Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Add "container.hot_path" tag to flag the hot path and inline related services#24872
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fcb8603 tob028e59Comparelyrixx commentedNov 8, 2017
I like it. But i think the name IIUC, here it inlines the factory in the dumped container + it pre-load the PHP Class. |
nicolas-grekas commentedNov 8, 2017
container.hot_path? |
lyrixx commentedNov 8, 2017
Yes it's better. And like that the name of the tag will be the same as the CompilerPass ;)
It depends. We already use some inlining feature in Blackfire. It could be useful anyway. |
| KernelEvents::CONTROLLER_ARGUMENTS, | ||
| KernelEvents::RESPONSE, | ||
| KernelEvents::FINISH_REQUEST, | ||
| KernelEvents::TERMINATE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think terminate needs to be in the hot path. When using PHP-FPM, this code runs after sending the request.
| return; | ||
| } | ||
| try { | ||
| $r =new \ReflectionClass($class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this might break if a parent class does not exist (ContainerBuilder is using a special autoloader to cover this when getting a tracked ReflectionClass)
| return; | ||
| } | ||
| $file =$r->getFileName(); | ||
| if (!$file ||$this->doExport($file) ===$file =$this->export($file)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reassigning$file in the middle of an expression using it on both side makes the code quite hard to read. Please use a different variable name instead.
| <serviceid="request_stack"class="Symfony\Component\HttpFoundation\RequestStack"public="true" /> | ||
| <serviceid="request_stack"class="Symfony\Component\HttpFoundation\RequestStack"public="true"> | ||
| <tagname="container.hot_path" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do we actually need this ? HttpKernel propagates it anyway (and if HttpKernel does not use the stack anymore, it is not hot anymore)
| $this->subscriberTag =$subscriberTag; | ||
| } | ||
| publicfunctionsetHotPathListeners(array$hotPathListeners,$tagName ='container.hot_path') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you are not setting listeners, you are setting events
| { | ||
| parent::build($container); | ||
| $hotPathListeners =array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
these are events, not listeners
stof commentedNov 8, 2017
Tests are missing, for both the compiler pass and the new dumper behavior |
nicolas-grekas commentedNov 8, 2017
Now with a new @stof thanks, comments addressed. |
nicolas-grekas commentedNov 8, 2017
Just proposed to enable the parameter by default insymfony/recipes#241 |
| if (isset($lineage[$class])) { | ||
| return; | ||
| } | ||
| if (!$r =$this->container->getReflectionClass($class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof this still throws, but in a more controlled way. Since this is running late, all services should be fine xor the container is broken already. Do we want really to not throw? WDYT?
beberlei commentedNov 8, 2017
I feel |
| $code .=sprintf(" require_once %s;\n",$file); | ||
| } | ||
| if ("\n" ===$code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
how about ternary operator with return?
| 'namespace' =>'', | ||
| 'as_files' =>false, | ||
| 'debug' =>true, | ||
| 'hot_path_tag' =>null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why does it not have a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't want it to be enabled by default, so this is opt-in.
This is the way to do it.
| privatefunctionaddInlineRequires() | ||
| { | ||
| if (!$this->hotPathTag || !$this->inlineRequires) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why have two ways to disable this? Cant you just make hotPathTag required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because these are two independent settings (that play well together.)
Tobion commentedNov 8, 2017
So the |
nicolas-grekas commentedNov 8, 2017
The routerclasses are always loaded yes (not the objects). We don't care about the related CLI perf impact IMHO, it's negligible. |
Tobion commentedNov 8, 2017
Can't we preload the CLI classes as well then to speed up the CLI? If the impact of preloading unused classes is negligible, this shouldn't hurt webrequest as well. |
nicolas-grekas commentedNov 8, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There's nothing to optimize for the CLI: the CLI is not called 700 times per second as the web is. Just forking the CLI process is an order of magnitude slower than these optimizations. |
dmaicher commentedNov 8, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
On my biggest monolith app I can also measure a slight performance benefit for one html page that I tested 👍
your branch HEAD~1 (8cd2193): So for me its roughly 2% faster. Not quite as significant as in your benchmarks. Under which conditions did you benchmark@nicolas-grekas ? |
nicolas-grekas commentedNov 9, 2017
Now with tests, PR ready for a last (hopefully) round of review. @dmaicher thanks for the confirmation! My test was on an annotated controller with a Twig-rendered Hello World, and using Blackfire to profile. Using |
00e0c23 toe20e523Comparenicolas-grekas commentedNov 9, 2017
(failures are false-positives) |
| 'as_files' =>false, | ||
| 'debug' =>true, | ||
| 'hot_path_tag' =>null, | ||
| 'inline_class_loader_parameter' =>'container.dumper.inline_class_loader', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is it configurable? Usually we don't want to add new options. IMHO, this is useless here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't like hardcoding a parameter name into the dumper
fabpot commentedNov 9, 2017
Can we make fabbot happy? Avoiding false-positives in the future would be great. |
nicolas-grekas commentedNov 9, 2017
green it is now :) |
fabpot commentedNov 9, 2017
Thank you@nicolas-grekas. |
…nd inline related services (nicolas-grekas)This PR was merged into the 3.4 branch.Discussion----------[DI] Add "container.hot_path" tag to flag the hot path and inline related services| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This PR is the result of my quest to squeeze some performance out of 3.4/4.0.It builds on two ideas:- a new `container.inline` tag that identifies the services that are *always* needed. This tag is only applied to a very short list of bootstrapping services (`router`, `event_dispatcher`, `http_kernel` and `request_stack` only). Then, it is propagated to all dependencies of these services, with a special case for event listeners, where only listed events are propagated to their related listeners.- replacing the PHP autoloader by plain inlined `require_once` in generated service factories, with the benefit of completely bypassing the autoloader for services and their class hierarchy.The end result is significant, even on a simple Hello World.Here is the Blackfire profile, results are consistent with `ab` benchmarks:https://blackfire.io/profiles/compare/b5fa5ef0-755c-4967-b990-572305f8f381/graphCommits-------f7cb559 [DI] Add "container.hot_path" tag to flag the hot path and inline related services
This PR was merged into the 3.4 branch.Discussion----------Add container.hot_path to built-in service tagsRelated to [symfony/symfony#24872](symfony/symfony#24872)<!--If your pull request fixes a BUG, use the oldest maintained branch that containsthe bug (seehttps://symfony.com/roadmap for the list of maintained branches).If your pull request documents a NEW FEATURE, use the same Symfony branch wherethe feature was introduced (and `master` for features of unreleased versions).-->Commits-------1647b9a Add container.hot_path to built-in service tags
Uh oh!
There was an error while loading.Please reload this page.
This PR is the result of my quest to squeeze some performance out of 3.4/4.0.
It builds on two ideas:
container.hot_pathtag that identifies the services that arealways needed. This tag is only applied to a very short list of bootstrapping services (router,event_dispatcher,http_kernelandrequest_stackonly). Then, it is propagated to all dependencies of these services, with a special case for event listeners, where only listed events are propagated to their related listeners.require_oncein generated service factories, with the benefit of completely bypassing the autoloader for services and their class hierarchy.The end result is significant, even on a simple Hello World.
Here is the Blackfire profile, results are consistent with
abbenchmarks:https://blackfire.io/profiles/compare/b5fa5ef0-755c-4967-b990-572305f8f381/graph