8000 Preload RFC and Symfony · Issue #29105 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Preload RFC and Symfony #29105

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
mateuszsip opened this issue Nov 6, 2018 · 23 comments
Closed

Preload RFC and Symfony #29105

mateuszsip opened this issue Nov 6, 2018 · 23 comments

Comments

@mateuszsip
Copy link
Contributor

Hi,

Preload RFC is now in voting phase, so I thought this is a good time to find out how it works with Symfony and what changes can be made to improve performance even further.

Maybe someone already did that?

@jvasseur
Copy link
Contributor
jvasseur commented Nov 6, 2018

The preloading RFC is about preloading class and function definitions, it can't preload variables.

We could go back to something similar to the class cache we had at some point but IMO preloading is more something that should be resolved at the composer level (composer could provide a script that preload all classes of a project) not at the Symfony level.

@mateuszsip
Copy link
Contributor Author

Yes, I thought about class cache to reduce overhead per request.

About composer, Jordi has some ideas: https://externals.io/message/103333#103335

I wanted to say we can do some benchmarks and think about possible improvements on symfony side.

I asked about existing benchmarks on symfony slack today, and got a response from Marco - I don't know technical aspects of preloading well, but it looks like some changes can be discussed.

@apfelbox
Copy link
Contributor
apfelbox commented Nov 6, 2018

Maybe preloading the compiled container can provide some benefit?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 6, 2018

Sure it would; see php/php-src#3538 (comment) for some numbers using Symfony. I'm not sure yet what we should do in core. Having support for preloading at composer stage looks like the way to go to me also.

@jvasseur
Copy link
Contributor
jvasseur commented Nov 6, 2018

The only could be done IMO in Symfony (but I'm not even sure it would bring benefits) is give the possibility to revert to a single file container to allow having the full container code preloaded since included files would not be preloaded.

@stof
Copy link
Member
stof commented Nov 7, 2018

@jvasseur the extra container files are not defining any class or function, and so would not benefit from preloading.

@stof
Copy link
Member
stof commented Nov 7, 2018

hmm, maybe the bypass of the autoloader by using require_once for services of the hot path should be reverted though (unless require_once can deal with files required already during the preloading phase of course)

@friizer
Copy link
friizer commented Nov 7, 2018

I tried the php-preload branch yesterday with symfony 3.4

The preload script was generated by calling an endpoint, and extracting the loaded classes, from php, so not all the classes was preloaded from vendor

After the extraction, i removed the following classes from preloading to make it work currently

  • whole cache dir
  • vendor/twig
  • vendor/swiftmailer

There can be problems, where the code, is using if/else constructs for different class declarations or class_exists/interface_exists checks ...

@stof
Copy link
Member
stof commented Nov 7, 2018

Well, for swiftmailer, it relies on performing some special initializing when loading the library, so it indeed cannot be preloaded.

The cache should indeed not be preloaded in debug mode (you could maybe preload it in prod, where you can afford restarting PHP-FPM when you rebuild the cache (and it may still be complex to rebuild the cache in such case, so I would not advice doing it). But a preload script generated

For Twig, maybe preloading does not handle class_alias properly. What was the error you were getting ?

@jvasseur
Copy link
Contributor
jvasseur commented Nov 7, 2018

@stof what I meant is by including extra files in the container we have to hit the opcache to get the opcodes necessary to create the service, if the code was included in the container class, the opcodes would already be preloaded with the container class.

< 8000 include-fragment loading="lazy" src="/symfony/symfony/issue_comments/436562724/edit_form?textarea_id=issuecomment-436562724-body&comment_context=" data-nonce="v2:28b7d6ac-5de4-895f-a7fc-b643afb7a8ad" data-view-component="true" class="previewable-comment-form js-comment-edit-form-deferred-include-fragment">

@nicolas-grekas
Copy link
Member

$container->setParameter('container.dumper.inline_class_loader', false); would do the trick.

@stof
Copy link
Member
stof commented Nov 7, 2018

@jvasseur for services not part of the hot path (or for all services when disabling the inline_class_loader optimization), the extra files don't require anything, but rely on the autoloader. So preloading the classes should rather be done by extracting classes from the composer autoloading config.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 7, 2018

About require_once this should definitely be handled by the preloading logic. Something to check maybe. We could add a flag to generate a preloading file for "container.hot_path" services (and thus skip generating require_once for those.)

@jvasseur
Copy link
Contributor
jvasseur commented Nov 7, 2018

@stof this has nothing about defining new classes.

We split the container into multiple files because it removes the cost of copying opcodes from opcache to the symbol table. With preloading this cost is removed so we could go back to the old version to remove the cost of including another file.

This is probably a really small optimization but I could be useful to do a benchmark to see if it could bring some benefits.

@friizer
Copy link
friizer commented Nov 7, 2018

For Twig, maybe preloading does not handle class_alias properly

maybe

if i just preload vendor/twig/twig/lib/Twig/RuntimeLoaderInterface.php which contains an additional class_alias:
class_alias('Twig_RuntimeLoaderInterface', 'Twig\RuntimeLoader\RuntimeLoaderInterface', false);

var_dump(interface_exists('\Twig_RuntimeLoaderInterface'));
var_dump(interface_exists('\Twig\RuntimeLoader\RuntimeLoaderInterface'));

bool(true)
bool(false)

I will ask dstogov, on this

@DavidBadura
Copy link
Contributor

Here is the issue to implement it in composer: composer/composer#7777

@Toflar
Copy link
Contributor
Toflar commented Nov 7, 2018

Ah sorry, I missed this!

@friizer
Copy link
friizer commented Nov 7, 2018

php/php-src#3538 (comment)

I think his reply means the class_alias call will not create a peristent, immutable class alias during preload

@stof
Copy link
Member
stof commented Nov 7, 2018

the last comment says it should now work

@stof
Copy link
Member
stof commented Nov 7, 2018

We split the container into multiple files because it removes the cost of copying opcodes from opcache to the symbol table. With preloading this cost is removed so we could go back to the old version to remove the cost of including another file.

Preloading is not about avoiding the cost of copying opcodes. It is about avoiding the cost of copying class and function definitions (plus the resolving of parents, which is done at runtime when they live in separate files). So AFAIU, preloading won't change things for files not defining classes or functions.

@stof
Copy link
Member
stof commented Nov 7, 2018

We could add a flag to generate a preloading file for "container.hot_path" services (and thus skip generating require_once for those.)

that's an interesting idea @nicolas-grekas

@friizer
Copy link
friizer commented Nov 8, 2018

php/php-src#3538 (comment)

I think class_alias calls can remain in the original files,, if include is used instead of opcache_compile_file in preload file

@nicolas-grekas
Copy link
Member

Please check #32032, feedback and benchs appreciated.

@fabpot fabpot closed this as completed Sep 8, 2019
fabpot added a commit that referenced this issue Sep 8, 2019
…lder (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[DI] generate preload.php file for PHP 7.4 in cache folder

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29105
| License       | MIT
| Doc PR        | -

This PR makes the PhpDumper generate a preloading file suited for PHP 7.4.
On a skeleton app, the generated file is `var/cache/dev/srcApp_KernelDevDebugContainer.preload.php` (of course, this varies by env name + kernel class)

One missing thing is listing some classes that are always needed but are not related to services.

Typically: `Request` and `Response`. We might need a new mechanism to make this list extensible.

I did not measure the benefit of this on PHP 7.4. I would really appreciate if someone could give it a try on PHP 7.4 with preloadin
7071
g enabled.

Commits
-------

c4dad0d [DI] generate preload.php file for PHP 7.4 in cache folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants
0