Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Optimize ReplaceAliasByActualDefinitionPass#18265
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
ajb-in commentedMar 22, 2016
Note: patch applies to master with minor modification (tested). |
lstrojny commentedMar 22, 2016
Improves container dump times from ~40s to ~16s in real world applications. |
| } | ||
| privatefunctionupdateFactoryServiceReference($factoryService,$currentId,$newId) | ||
| privatefunctionupdateFactoryReferenceId($replacements,$referenceId) |
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 about adding a doc block 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.
On it 👍
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.
Done
GuilhemN commentedMar 22, 2016
This looks great 👍 |
ajb-in commentedMar 22, 2016
Thanks @Ener-Getick ^_^ |
| $aliasId = (string)$alias; | ||
| if (isset($replacements[$aliasId])) { | ||
| $container->setAlias($defId,$replacements[$aliasId]); |
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 must respect the visibility of the existing alias (allowing it to be removed in case of a private alias not being referenced anywhere anymore after other optimizations)
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 tried to stick to the original behaviour, namely: remove the definition of the target, point the alias to the definition, and point every subsequent alias with the same target to the first alias.
ajb-in commentedMar 23, 2016
@stof re chains: I played with the existing testcase for 2.3 to see what happens with chained aliases. I ran this code: and it 'splodedon 2.3: There was 1 error:1) Symfony\Component\DependencyInjection\Tests\Compiler\ReplaceAliasByActualDefinitionPassTest::testProcessSymfony\Component\DependencyInjection\Exception\InvalidArgumentException: Unable to replace alias "d_alias" with actual definition "c_alias"./code/symfony/src/Symfony/Component/DependencyInjection/Compiler/ReplaceAliasByActualDefinitionPass.php:48/code/symfony/src/Symfony/Component/DependencyInjection/Compiler/ReplaceAliasByActualDefinitionPass.php:63/code/symfony/src/Symfony/Component/DependencyInjection/Tests/Compiler/ReplaceAliasByActualDefinitionPassTest.php:69/code/symfony/src/Symfony/Component/DependencyInjection/Tests/Compiler/ReplaceAliasByActualDefinitionPassTest.php:40Caused bySymfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "c_alias". |
ajb-in commentedMar 23, 2016
It explodes also if I change the extra part to this: |
tadcka commentedMar 23, 2016
👍 |
egils commentedMar 23, 2016
Lovely! 👍 |
xabbuh commentedMar 25, 2016
@ajb-in Aliases that refer to other aliases should have been resolved in the |
| * | ||
| * @return string | ||
| */ | ||
| privatefunctionupdateFactoryReferenceId($replacements,$referenceId) |
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.
missing typehint array for$replacements
Tobion commentedMar 27, 2016
👍 apart from those small doc fixes. |
| $seenAliasTargets =array(); | ||
| $replacements =array(); | ||
| foreach ($container->getAliases()as$definitionID =>$target) { |
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.
should be$definitionId as$targetId is used later
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.
?
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.
id is upper-cased (ID instead ofId), i think it should be changed for consistency.
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 see, true
| $definition =$container->getDefinition($targetId); | ||
| }catch (InvalidArgumentException$e) { | ||
| thrownewInvalidArgumentException(sprintf('Unable to replace alias "%s" with actual definition "%s".',$id,$alias),null,$e); | ||
| thrownewInvalidArgumentException(sprintf('Unable to replace alias "%s" with actual definition "%s".',$definitionID,$target),null,$e); |
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 not using$targetId 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.
agree
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.
No idea :3 That's what the old code was doing though (using$alias instead of$aliasId). I'll change it.
ajb-in commentedMar 27, 2016
@Tobion @Ener-Getick Did doc and variable name fixes; looks better? |
ajb-in commentedMar 27, 2016
fabpot commentedMar 27, 2016
As this is not a bug fix, it should be merged in master, not 2.3 |
ajb-in commentedMar 27, 2016
@fabpot When I asked in #symfony I was told that it could count as a performance bug, so that's why I went with 2.3;@javiereguiluz seemed to agree since he labeled this PR as 'bug'. |
fabpot commentedMar 27, 2016
Sorry about the confusion. It's true that we've been more willing to merge PRs that do not strictly fix bugs in maintained branches (and I probably merged some myself), but I think this is a mistake. First, we want old branches to be as stable as possible, and merging non-bug fixes can introduce subtle regressions (happened in the past). Then, improving performance in new versions is a great incentive for people to upgrade, which is always a good idea. /cc @symfony/deciders |
Tobion commentedMar 28, 2016
I'd prefer to merge refactorings like this one in maitainance branches as it will make maintaining branches easier when they are more similar and do not diverge that much just because of refactorings. |
Tobion commentedMar 28, 2016
And introducing suble behavior changes should not be an argument. It just means we do not have enough tests (or it's really an edge case that wasn't meant to be like this). E.g. the refactoring in the templating parser actually revealed that people use absolute paths that wasn't intended that way. |
fabpot commentedMar 30, 2016
@ajb-in Can you rebase on current 2.3? Thanks. |
weaverryan commentedMar 30, 2016
In general, I'd prefer things like this to be merged into master. If this causes a subtle behavior change that isn't caught by our tests, that should be our problem, not a problem that is discovered by users in a patch update. We need users to have very high confidence in upgrading patch versions (otherwise, they may stay on insecure versions!) But it's not black and white - a performance issue could be a bug, so we can do anything here. |
ajb-in commentedMar 31, 2016
Rebased on latest 2.3; also squashed, added a comment, and added a plug to this review on the commit message :3 |
fabpot commentedMar 31, 2016
@ajb-in Can you remove the reference in the commit message? We already have everything in place to automatically get that in Git notes. |
ajb-in commentedMar 31, 2016
@fabpot Done; just wanted to give some due credit :3 |
fabpot commentedMar 31, 2016
Tests are broken apparently. |
Previous implementation passed over every alias and every definition, for everyalias (n*(n+m) runtime). New implementation passes once over all aliases andonce over all definitions (n+m).Also removing needless "restart" logic --- it has no real effect in either case.
ajb-in commentedMar 31, 2016
@fabpot I forgot to rename a variable =P |
fabpot commentedMar 31, 2016
Thank you@ajb-in. |
This PR was merged into the 2.3 branch.Discussion----------Optimize ReplaceAliasByActualDefinitionPass| Q | A| ------------- | ---| Branch? | 2.3| Bug fix? | yes (performance)| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Previous implementation passed over every definition for every alias (n*m runtime).New implementation passes once over all aliases and once over all definitions (n+m).Also removing needless "restart" logic.Commits-------ab8dc0c Optimize ReplaceAliasByActualDefinitionPass
ajb-in commentedMar 31, 2016
🍻 Thanks to everyone! |
* Test were broken because of this PR introduced in symfony 2.3:symfony/symfony#18265* We were passing an object instead of a string.
* Tests were broken because of this PR introduced in symfony 2.3:symfony/symfony#18265* We were passing an object instead of a string.
Previous implementation passed over every definition for every alias (n*m runtime).
New implementation passes once over all aliases and once over all definitions (n+m).
Also removing needless "restart" logic.