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] fine tune dumped factories#27268
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
ogizanagi commentedMay 15, 2018
Can't we merge this in lower branches too? |
nicolas-grekas commentedMay 15, 2018 • 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.
that's not a bugfix but an improvement, so that's for master |
| continue; | ||
| } | ||
| $ids =array(); | ||
| foreach ($node->getInEdges()as$edge) { |
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'd recommend to extract that whole logic into a new private method for clarity.
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
| private$inlineRequires; | ||
| private$inlinedRequires =array(); | ||
| private$circularReferences =array(); | ||
| private$singlePrivateIds =array(); |
javiereguiluzMay 15, 2018 • edited by stof
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by stof
Uh oh!
There was an error while loading.Please reload this page.
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.
Minor comment. I didn't like the$singlePrivateIds var name, so I looked forEnglish words that mean "used once" and maybe it could be better to rename this to$singleUsePrivateIds
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.
updated
| return$this->hotPathTag &&$definition->hasTag($this->hotPathTag) && !$definition->isDeprecated(); | ||
| } | ||
| privatefunctionisSingleUsePrivateNode(ServiceReferenceGraphNode$node) |
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.
bool return declaration?
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.
not on 3.4
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.
but OK on master :)
fabpot commentedMay 17, 2018
Thank you@nicolas-grekas. |
This PR was merged into the 4.2-dev branch.Discussion----------[DI] fine tune dumped factories| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Here is an optimization we forgot in the dumped container: when a private service is referenced once, there is no need to keep it in the internal registry as it will never be reused. This should help a bit the garbage collection process.Commits-------88ecd0d [DI] fine tune dumped factories
…services (chalasr)This PR was merged into the 3.4 branch.Discussion----------[DI] Fix dumping expressions accessing single-use private services| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29403| License | MIT| Doc PR | n/aIntroduced in#27268, see fixed ticketCommits-------d1e84aa [DI] Fix dumping expressions accessing single-use private services
Here is an optimization we forgot in the dumped container: when a private service is referenced once, there is no need to keep it in the internal registry as it will never be reused. This should help a bit the garbage collection process.