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] Improve performance of removing/inlining passes#27471
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
5095d4f to2f0c343Comparenicolas-grekas commentedJun 3, 2018
@tarlepp since you reported a slowdown on Slack, can you please try this PR and report any results? |
staabm commentedJun 3, 2018
The blackfire link in the desc doesnt work |
nicolas-grekas commentedJun 3, 2018
@staabm updated thanks. Note that our test suite is pretty nice on this topic, coverage is pretty great. I also used this patch to compile Blackfire's container, and it produces exactly same output. Confidence is good on my side :) |
0bf53a9 toae832d5Compare21effb3 to75114e9Compare
chalasr left a comment
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.
UPGRADE notes are missing
| * | ||
| * @author Johannes M. Schmitt <schmittjoh@gmail.com> | ||
| * | ||
| * @deprecated since Symfony 4.1. |
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.
4.2 :)
| namespaceSymfony\Component\DependencyInjection\Compiler; | ||
| @trigger_error(sprintf('The "%s" class is deprecated since Symfony 4.1.', RepeatedPass::class),E_USER_DEPRECATED); |
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.
same
nicolas-grekas commentedJun 4, 2018
Do we need upgrade notes here? This is mostly internal thing IMHO. |
| foreach ($this->graph->getNode($id)->getInEdges()as$edge) { | ||
| $srcId =$edge->getSourceNode()->getId(); | ||
| $this->connectedIds[$srcId] =true; | ||
| if ($edge->isWeak()) { |
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.
if an edge is weak, other edges will not be marked as connected here. Is is expected ?
| } | ||
| if (count(array_unique($ids)) >1) { | ||
| if (1 !==\count($srcIds)) { |
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.
what is the difference between\count($srcIds) and$srcIdsCount ? If$srcIdsCount is the number of references (and so a single service referencing it multiple times is counted multiple times), the variable name is not clear.
| ); | ||
| if ($this->inExpression) { | ||
| $this->inExpression =false; |
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 making itfalse again ? Wouldn't this break in case an expression is referencing multiple services ?
| */ | ||
| publicfunctionprocess(ContainerBuilder$container) | ||
| { | ||
| $this->enableExpressionProcessing(); |
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.
Are you sure about that ? Replacing a reference inside the expression won't change the expression anyway. It will keep using the alias.
| */ | ||
| publicfunctionprocess(ContainerBuilder$container) | ||
| { | ||
| $this->enableExpressionProcessing(); |
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.
Are you sure about that ? Replacing a reference inside the expression won't change the expression anyway. It will keep using the alias.
nicolas-grekas commentedJun 5, 2018
@stof agreed, thanks for the review, comments addressed. |
src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/CachePoolClearerPassTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/Compiler/AnalyzeServiceReferencesPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Tobion commentedJun 5, 2018
Thank you@nicolas-grekas. |
…nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[DI] Improve performance of removing/inlining passes| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#27386| License | MIT| Doc PR | -Here is an optimization to reclaim some compilation time by optimizing the analysis of unused and inlined services.This PR removes any use case for `RepeatedPass`, instead:- `RemoveUnusedDefinitionsPass` works in one run, removing all private services that are unreachable from public services- `InlineServiceDefinitionsPass` reduces the number of nodes to analyze per iteration using `AnalyzeServiceReferencesPass` on a duplicated container internally.https://blackfire.io/profiles/compare/00723822-6c09-431c-b98d-4a4197d044fc/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=focused&settings%5BtabPane%5D=nodes&selected=Symfony%5CComponent%5CDependencyInjection%5CCompiler%5CRepeatedPass%3A%3Aprocess&callname=Symfony%5CComponent%5CDependencyInjection%5CCompiler%5CRepeatedPass%3A%3AprocessCommits-------cf375e5 [DI] Improve performance of removing/inlining passes
Uh oh!
There was an error while loading.Please reload this page.
Here is an optimization to reclaim some compilation time by optimizing the analysis of unused and inlined services.
This PR removes any use case for
RepeatedPass, instead:RemoveUnusedDefinitionsPassworks in one run, removing all private services that are unreachable from public servicesInlineServiceDefinitionsPassreduces the number of nodes to analyze per iteration usingAnalyzeServiceReferencesPasson a duplicated container internally.https://blackfire.io/profiles/compare/00723822-6c09-431c-b98d-4a4197d044fc/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=focused&settings%5BtabPane%5D=nodes&selected=Symfony%5CComponent%5CDependencyInjection%5CCompiler%5CRepeatedPass%3A%3Aprocess&callname=Symfony%5CComponent%5CDependencyInjection%5CCompiler%5CRepeatedPass%3A%3Aprocess