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

[HttpKernel][DI] allow bundles to declare classes that should be preloaded #33689

Open
wants to merge 1 commit into
base: 4.4
from

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 24, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets #29105
License MIT
Doc PR -

This adds new methods to allow apps and bundles to declare classes that should be preloaded (when using PHP 7.4's opcache.preload).

Since #32032, the DI component is already able to generate a preloading script, based on services with the container.hot_path tag. Yet, based on my local experiments, not all classes can be discovered using this strategy. Also, it appears that while preloading all classes in vendor/ is said to be a bad idea by Dmitry itself, not preloading a class has a measurable impact.

Thus we should seek to preload all actually used classes. A bit more than that amount is better than a bit less.

As such, this PR now preloads all service classes and adds the infrastructure needed to list more classes when appropriate.

Listing more classes is done either via:

  • Kernel::getClassesToPreload(), which by default loads everything in the namespace of the app's kernel (usually the App\ namespace) + all bundle classes
  • Extension::addClassesToPreload(), so that each bundle's DI extension can add to the list. FrameworkBundle, SecurityBundle, and TwigBundle already contain the core they need.

This works nice on a skeleton app but segfaults on a website-skeleton for now, so I won't give numbers.

This is nonetheless ready (merging would actually help php-internal to reproduce the segfault.)

PS: this requires dumping the autoloader, i.e. composer install -o

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:classes-to-preload branch from 61acb29 to c539899 Sep 24, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:classes-to-preload branch from c539899 to 90ba34d Sep 25, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Sep 25, 2019

The var/cache/ folder contains additional files to preload - especially twig/ and validation.php, maybe others. We might need an extensibility mechanism to preload them too.

@Flyingmana Flyingmana mentioned this pull request Sep 26, 2019
0 of 4 tasks complete
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@nicolas-grekas nicolas-grekas modified the milestones: 4.4, next Nov 9, 2019
Copy link
Member

stof left a comment

We also need a way to force some classes to stay out of preloading IMO.
We had this issue in the past with Swiftmailer doing some magic during autoloading to load the swiftmailer initialization code, where hot-loading the file i the compiled container would break things by skipping the autoloader. SwiftmailerBundle could fix that by forcing to remove the hot tag on all its services after the DI component marked services as hot (as marking services as hot is separate from performing optimization for the). But here, I don't see an opt-out system.

'Symfony\Component\Cache\Adapter\ApcuAdapter',
'Symfony\Component\Cache\Adapter\ArrayAdapter',
'Symfony\Component\Cache\Adapter\PhpArrayAdapter',
'Symfony\Component\Cache\Adapter\PhpFilesAdapter',

This comment has been minimized.

Copy link
@stof

stof Nov 15, 2019

Member

Aren't these discovered thanks to services already ?

'Symfony\Component\HttpFoundation\ServerBag',
'Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent',
'Symfony\Component\HttpKernel\Event\ControllerEvent',
'Symfony\Component\HttpKernel\Event\TerminateEvent',

This comment has been minimized.

Copy link
@stof

stof Nov 15, 2019

Member

what about RequestEvent and ResponseEvent ?

@@ -170,6 +170,18 @@ public function load(array $configs, ContainerBuilder $container)
$container->removeDefinition('twig.cache_warmer');
$container->removeDefinition('twig.template_cache_warmer');
}

$this->addClassesToPreload([
'Symfony\Component\Stopwatch\Section',

This comment has been minimized.

Copy link
@stof

stof Nov 15, 2019

Member

shouldn't it be added by FrameworkBundle when defining the stopwatch service instead ? Thus, I don't think we use it in prod code.

@stof

This comment has been minimized.

Copy link
Member

stof commented Nov 15, 2019

Note that preloading hot services rather than all services would already provide such opt-out.

@rvanlaak

This comment has been minimized.

Copy link
Contributor

rvanlaak commented Jan 6, 2020

As preloading might have some overlap with the already existing addAnnotatedClassesToCompile method, it might be good to document the similarities / differences.

Will compiled classes get preloaded as well?

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.