Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Add in-memory cache support to Blade #51141

Closed
wants to merge 2 commits into from

Conversation

amir9480
Copy link
Contributor

@amir9480 amir9480 commented Apr 19, 2024

I working on a Filament based project and experiencing slow performance on pages including tables, and realized the issue is that Filament uses lots of components to render resources.
I have seen Taylor's recent tweet about the Blade component's performance and tried to optimize it by converting Blade compiled codes to returning functions and caching it inside memory instead of reading the component from disk on each render.

I tried to keep PHP engine working on non-callback returns to prevent any breaking changes while rendering old compiled views.

Performance results:
image
image

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@taylorotwell
Copy link
Member

taylorotwell commented Apr 19, 2024

I would suggest testing your solution on a server with and without opcache enabled... then test Laravel 11.x without your PR with opcache enabled and see if there is much of a difference. Other PRs I've seen attempt attempt to take a similar approach do not see any speed benefit once Opcache is enabled because it eliminates the file I/O penalty for you.

Please mark as ready for review again when you have those results.

@taylorotwell taylorotwell marked this pull request as draft April 19, 2024 18:42
@amir9480
Copy link
Contributor Author

@taylorotwell
I tried each one 10 times and captured the best time.

Without opcache With opcache
Without Blade Caching image image
With Blade Caching image image

I think calling a function has less overhead than using require on each render.

Code used to benchmark:

Route::get('/', function () {
    $a = array_map(
        fn () => Benchmark::measure(fn() => view('home')->render()),
        range(0, 10)
    );

    return 'OPCache Enabled: ' . (is_array(opcache_get_status()) ? 'Enabled' : 'Disabled') .
            '<br>With caching: ' . (array_sum($a) / count($a));
});

home.blade.php:

@foreach (range(1, 20000) as $id)
    <x-avatar :name="'Taylor'" class="mt-4" />
@endforeach

@amir9480 amir9480 marked this pull request as ready for review April 19, 2024 19:43
@taylorotwell
Copy link
Member

Got it - so 299ms vs 280ms. It's a small improvement but not quite as big of an impact as I would like to make on this part of the stack. I think most of the improvements we are looking for should probably be outside of file I/O since Opcache gets rid of that concern for the most part.

@amir9480
Copy link
Contributor Author

amir9480 commented Apr 19, 2024

@taylorotwell
Another overhead is making a new instance of View on each render. I tried to cache the view instance and just update data inside it in the next calls and here is the result but I'm not sure if it's a breaking change or not (Like nested components including self inside content).

return tap($this->viewInstance($view, $path, $data), function ($view) {

Without view instance caching:
image

With view instance caching:
image

I did a quick change just for testing:

public function make($view, $data = [], $mergeData = [])
    {
        $path = $this->finder->find(
            $view = $this->normalizeName($view)
        );

        // Next, we will create the view instance and call the view creator for the view
        // which can set any data, etc. Then we will return the view instance back to
        // the caller for rendering or performing other view manipulations on this.
        $data = array_merge($mergeData, $this->parseData($data));
        if ($view == 'components.avatar') {
            if ($this->cachedView) {
                return $this->cachedView->with($data);
            }
            $this->cachedView = $this->viewInstance($view, $path, $data);
            return $this->cachedView;
        }

        return tap($this->viewInstance($view, $path, $data), function ($view) {
            $this->callCreator($view);
        });
    }

@taylorotwell
Copy link
Member

That's fairly interesting. A 20% reduction isn't bad.

@taylorotwell
Copy link
Member

I think another area of improvement is we are calling callComposer and callCreator in ManagesEvents for every instance of a component we create, even if there are no composers or creators for that view.

@linaspasv
Copy link
Contributor

It's not Filament only but LiveWire applications in general would benefit as these are often view heavy, even if it's only ~7-10% improvement... it's still an improvement. Thank you for the PR and good luck in squeezing even more performance!

@amir9480
Copy link
Contributor Author

@taylorotwell
I opened another PR for view instance caching. #51152

@linaspasv
Copy link
Contributor

linaspasv commented Apr 21, 2024

I think another area of improvement is we are calling callComposer and callCreator in ManagesEvents for every instance of a component we create, even if there are no composers or creators for that view.

I do believe #51143 PR is trying to improve on these areas with some marginal gains.

@lonnylot
Copy link
Contributor

@taylorotwell Another overhead is making a new instance of View on each render. I tried to cache the view instance and just update data inside it in the next calls and here is the result but I'm not sure if it's a breaking change or not (Like nested components including self inside content).

return tap($this->viewInstance($view, $path, $data), function ($view) {

Without view instance caching: image

With view instance caching: image

I did a quick change just for testing:

public function make($view, $data = [], $mergeData = [])
    {
        $path = $this->finder->find(
            $view = $this->normalizeName($view)
        );

        // Next, we will create the view instance and call the view creator for the view
        // which can set any data, etc. Then we will return the view instance back to
        // the caller for rendering or performing other view manipulations on this.
        $data = array_merge($mergeData, $this->parseData($data));
        if ($view == 'components.avatar') {
            if ($this->cachedView) {
                return $this->cachedView->with($data);
            }
            $this->cachedView = $this->viewInstance($view, $path, $data);
            return $this->cachedView;
        }

        return tap($this->viewInstance($view, $path, $data), function ($view) {
            $this->callCreator($view);
        });
    }

Playing around with this: this improvement is 100% because of the call to $this->callCreator. If you comment out that line then the rest stays the same.

You also gain marginal improvements from anything else happening in loops. E.g. getEngineFromPath, normalizeName. Nothing as high as 20%, but if you do enough caching of these repeated processes then it'll shave off ms combined.

@taylorotwell
Copy link
Member

Closing for other PR you have open.

@taylorotwell
Copy link
Member

@amir9480 I'm not seeing any performance improvement here locally. 🤔

@amir9480
Copy link
Contributor Author

amir9480 commented Apr 23, 2024

Maybe I use an old laptop and a big range number (10 x 20,000) for testing 🤔.

Or maybe you tested on old compiled cached view and should clear cached views to recompile as a callback.

@mpociot could you please run your performance test with this PR too?

@amir9480 amir9480 force-pushed the add_view_require_caching branch from 60f638e to e8c993f Compare April 23, 2024 08:49
@taylorotwell
Copy link
Member

Going to close this one as I'm not seeing much benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants