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

Preload RFC and Symfony #29105

Open
kejwmen opened this Issue Nov 6, 2018 · 22 comments

Comments

Projects
None yet
8 participants
@kejwmen
Contributor

kejwmen commented Nov 6, 2018

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

This comment has been minimized.

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.

@kejwmen

This comment has been minimized.

Contributor

kejwmen commented Nov 6, 2018

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

This comment has been minimized.

Contributor

apfelbox commented Nov 6, 2018

Maybe preloading the compiled container can provide some benefit?

@nicolas-grekas

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 7, 2018

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

@stof

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

DavidBadura commented Nov 7, 2018

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

@Toflar

This comment has been minimized.

Contributor

Toflar commented Nov 7, 2018

Ah sorry, I missed this!

@friizer

This comment has been minimized.

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

This comment has been minimized.

Member

stof commented Nov 7, 2018

the last comment says it should now work

@stof

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment