Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DependencyInjection] Improve performance of the generated code.#9432
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
@fabpot@realityking what do you think of addinghttps://github.com/polyfractal/athletic performance tests to verify this speedup thing and make sure it won't regress? |
| if ($callable[0] instanceof Definition) { | ||
| if ($this->definitionVariables->contains($callable[0])) { | ||
| return sprintf(" %s->%s(\$%s);\n", $this->dumpValue($callable[0]), $callable[1], $variableName); | ||
| } elseif (version_compare(PHP_VERSION, '5.4.0') >= 0) { |
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 is enough as the previousif returns
This PR is actually doing 2 unrelated things:
These 2 things should be done in separate PRs, which can be reviewed and merged separately. The bug fix is probably far quicker to review and merge than the performance improvements (especially given that I find them suspicious in my first quick review). Thus, the bugfix should probably be merged in 2.2 while the improvements can only go in master. |
@staabm +1 for integration of such tool likehttps://github.com/polyfractal/athletic |
Took me a bit to get back to this. I'd be happy to split them, but I'm a bit unsure whether the configurator patch should be done to 2.3. The fix is a bit more involved and there's no bug in the sense that something breaks - it's just a missed optimization opportunity. Thoughts? |
Optimizations must be done on master only (except if the patch is simple enough to understand and it the performance gain is very significant). |
ghost commentedDec 16, 2013
ubot detected some small issues in this pull request. You can make the modifications by hand or run the following command from the repository root directory: diff -ru src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php--- src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php 2013-12-16 16:30:17.631122873 +0000+++ src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php 2013-12-16 16:30:18.803122873 +0000@@ -710,6 +710,7 @@ if (strpos($class, "'") === 0) { return sprintf(" $return{$instantiation}%s::%s(%s);\n", substr($class, 1, -1), $definition->getFactoryMethod(), $arguments ? implode(', ', $arguments) : ''); }+ return sprintf(" $return{$instantiation}call_user_func(array(%s, '%s')%s);\n", $class, $definition->getFactoryMethod(), $arguments ? ', '.implode(', ', $arguments) : ''); } |
I'll spin the second commit off after the first is merged since it depends on the first. |
…urators (realityking)This PR was merged into the 2.5-dev branch.Discussion----------[DependencyInjection] added support for inlining Configurators| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This is one commit from#9432.As mentioned in#3758 configurators can not be private (it's just ignored). This pull changes that and allows them to be inlined. It it also creates better code if a configurator is used multiple times for one service (i.e. to both inject it and configure the same service, or to configure multiple inlined services) but this should be very rare.Commits-------4e9aa07 [DependencyInjection] added support for inlining Configurators
#9791 has been merged now |
I'm closing this one as I've opened a pull request with the other code. I excluded the different path that only works on PHP 5.4 as that would be difficult to test for very little gain, maybe something to consider for 3.0. @stof I'd appreciate it if you'd look over the new pull request as well. |
…ainers. (realityking)This PR was merged into the 2.5-dev branch.Discussion----------[DependencyInjection] Avoid call_user_func in dumped containers.| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This is the second commit from#9432.When using static methods for either the factory or as the configurator we can avoid using call_user_func and directly use the class:method notation. This is faster (about 5 times, but we're talking milliseconds here) but I think the resulting code is also much easier to read.The code to use call_user_func has to remain in PhpDumper because in the uncompiled container they still get used.Commits-------be1eaaa [DependencyInjection] Avoid call_user_func in dumped containers.
… more cases (realityking)This PR was submitted for the master branch but it was merged into the 3.0 branch instead (closes#18682).Discussion----------[DependencyInjection] Avoid generating call_user_func in more cases| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This is a follow up to#9807 to add the PHP 5.4 only coded I excluded when I split it off#9432.Commits-------2718a6c [DependencyInjection] Avoid generating call_user_func in more cases
I noticed a couple of ways the code generated for the compiler could be improved:
Note that this patch generates different code on 5.4 to prevent a call to call_user_func. If that's not desired I'll revert that part.
TODO: