[DependencyInjection] made some perf improvements #10241
Conversation
| @@ -105,6 +106,8 @@ public function dump(array $options = array()) | |||
| 'namespace' => '', | |||
| ), $options); | |||
|
|
|||
| $this->parameterArgName = 'p'.md5($options['class']); | |||
There was a problem hiding this comment.
does it need to be a different name depending on the container class name ?
There was a problem hiding this comment.
yes, it needs to if you simulate 2 clients for instance in the same process, each client/container must have its own parameters.
There was a problem hiding this comment.
https://github.com/symfony/symfony/pull/10241/files#diff-f7b23d463cba27ac5e4cb677f2be7623R813 Ah because its a static :)
There was a problem hiding this comment.
@fabpot If the 2 clients have the same container class name, they will still share the same parameters. If they have different container class names, they will also have different static properties even if the name is not a hash
|
Any benchmark which shows the "drastic" improvements? |
|
My quick test shows (removed container and pressed refresh a few times after it was rebuilt) about 300k lower memory usage (25,2M vs 25,5M) and about 10ms less load time in dev environment.. With this new commit. |
|
@fabpot any news about this ? |
|
@fabpot any news ? |
7ad7a11 to
cf0fc08
Compare
cf0fc08 to
e1a3ef8
Compare
|
Ready for me. ping @symfony/deciders |
|
Looks ok to me. But I don't see the drastic improvment. Can somebody explain please why this improves performance? |
|
|
||
| $code = <<<EOF | ||
|
|
||
| private static \$parameters = $parameters; |
There was a problem hiding this comment.
is the static property needed in case of non-frozen containers ? would it improve perf in this case ? The static array will need to be copied as soon as you manipulate parameters.
I know that this is kind of an edge case given that Symfony always freezes the container before dumping it (I'm not even sure there is valid use case for dumping a non-frozen container, given that parameters used in service definitions will always be inlined, so changing them in the ParameterBag will be useless for most of them)
There was a problem hiding this comment.
Indeed, without a frozen container, the optimization won't work but it won't be worse than now, so as that should never happen, it does not really matters.
There was a problem hiding this comment.
should we prevent dumping a container which is not frozen in 3.0 given that the fact of dumping will freeze it partially ?
|
👍 |
This PR was merged into the 2.6-dev branch. Discussion ---------- [DependencyInjection] made some perf improvements | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR optimizes the creation of dumped containers drastically (thanks @jpauli for the hint). The Container class generated by the PHP dumper does not include the `getDefaultParameters()` method anymore. It does not seem like a big deal to me as I fail to see a use case where someone would override this method. Commits ------- e1a3ef8 [DependencyInjection] made some perf improvements
This PR was merged into the 2.6 branch. Discussion ---------- [DependencyInjection] fix PhpDumper | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #12815 | License | MIT | Doc PR | none Commits ------- 7887a46 [DependencyInjection] keep some of the reverted perf optim 2f0b355 Revert "minor #10241 [DependencyInjection] made some perf improvements (fabpot)"
* 2.6: [DependencyInjection] keep some of the reverted perf optim Revert "minor #10241 [DependencyInjection] made some perf improvements (fabpot)" Remove deprecated class
* 2.7: [DependencyInjection] keep some of the reverted perf optim Revert "minor #10241 [DependencyInjection] made some perf improvements (fabpot)" Remove deprecated class
This PR optimizes the creation of dumped containers drastically (thanks @jpauli for the hint).
The Container class generated by the PHP dumper does not include the
getDefaultParameters()method anymore. It does not seem like a big deal to me as I fail to see a use case where someone would override this method.