Skip to content

[11.x] Memoize requiring of view files to improve loop performance #51139

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

Closed
wants to merge 5 commits into from

Conversation

riasvdv
Copy link
Contributor

@riasvdv riasvdv commented Apr 19, 2024

Spent some time on this as a result of Taylor's tweet: https://twitter.com/taylorotwell/status/1781039378376146970

This reduces the main overhead of require calls when looping over 1000 components by storing the code, and running it through eval.

This seems to be something that Twig does as well when I was researching how evil calling eval in this case is https://github.com/twigphp/Twig/blob/12625c0c050db5a71d94ab8d599bf711673a41c3/src/Environment.php#L384 though I'm not that familiar with Twig.

This lowers the rendering in a testcase for me from 25ms to 11ms, though I'm not sure we'd want to use eval

Thanks to @mpociot mentioning it, it also skips an extra file_exists check.

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.

@mpociot
Copy link
Contributor

mpociot commented Apr 19, 2024

Nice!
I did some benchmarks on this, comparing the getRequire and getRequireCallback performance, using a simple component that gets rendered 1000 times:

$view = <<<BLADE
@for (\$i=0; \$i < 1000; \$i++)
    <x-avatar :name="'Taylor'" class="mt-4" />
@endfor
BLADE;

$result = \Illuminate\Support\Benchmark::measure(function () use ($view) {
    Blade::render($view);
}, 1000);

CleanShot 2024-04-19 at 15 40 36

@taylorotwell
Copy link
Member

taylorotwell commented Apr 19, 2024

Thanks for the PR! Laravel has previously used eval() to render views back in the old days. There are a few caveats:

  1. Eval'd code is never opcached, and runs at normal, non-opcache speed.

  2. I'm curious how error handling via Flare / debug pages would be affected, since any errors in the view would generate errors like "error in eval'd code line 76", or something similar.

I would also be curious if enabling opcache negates this change essentially. On a test server, enabling opcache cuts the rendering time of 10,000 components in half - roughly similar to this PR. If you use this PR and also enable opcache, do you see even further benefits or negligible improvements? 🤔

@taylorotwell
Copy link
Member

taylorotwell commented Apr 19, 2024

Update: after doing some testing on a Forge provisioned server, with Opcache enabled this PR is actually slower than the code we have now. Likely due to evaluated code not being Opcache and also Opcache negating the IO penalties this seeks to avoid in the first place.

@riasvdv
Copy link
Contributor Author

riasvdv commented Apr 19, 2024

Makes sense with opcache, maybe looking more into what I had in the first commit c325cee would be more interesting then, if we can differentiate between components that can be wrapped in a callable and not.

That commit had issues with components that had arbitrary PHP code and/or use statements in the file, but otherwise had the same performance improvements.

I see @amir9480 has just opened a PR that seems to be doing the same but already has those checks #51141 so I'll be closing this then.

@riasvdv riasvdv closed this Apr 19, 2024
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.

3 participants