Avoid unnecessary calls to strtolower(). #14558
Conversation
FIx syntax error
| if ($strtolower) { | ||
| return; | ||
| } | ||
| else { |
There was a problem hiding this comment.
I think that it should be.
if ($strtolower && self::EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior) {
// code
} else if ($strtolower) {
return;
} else {
continue;
}|
How many service call you tested? |
|
So this cut 190 calls to strtolower() - i.e. Container::get() was called with a different service name 190 times during the request. Actual usage on real sites may well end up being higher. |
|
But the |
|
@dawehner you're right 👍 |
|
I think this can be merged in 2.3 |
| } | ||
|
|
||
| throw new ServiceNotFoundException($id, null, null, $alternatives); | ||
| } else if ($strtolower) { |
|
should be merged in 2.7. |
|
So we save a strtolower call, but on the other the following code is executed twice or am I missing something? So what is the actual net gain? |
|
That only gets run twice if the loop doesn't return before (i.e. no match without the normalisation). |
|
@Tobion there is an issue here: if the id passed in the method is not lowercased, the service will be built again, replacing the previous instance. This introduces a bug. This is precisely why the normalized version of the id is checked in instantiated services before going through the creation logic. so 👎 for this PR. It is broken for dumped containers (method names in PHP are case insensitive, so it will find the dumped factory method). The logic works fine for the ContainerBuilder because the Definition lookup is case sensitive in this case. |
|
See #14643 |
…wer() (nicolas-grekas) This PR was merged into the 2.3 branch. Discussion ---------- [DependencyInjection] Avoid unnecessary calls to strtolower() | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14558 | License | MIT | Doc PR | - Commits ------- c27f564 [DependencyInjection] Avoid unnecessary calls to strtolower()
DependencyInjection - remove unnecessary calls to strtolower()
I noticed when profiling Drupal 8, that each service requested via container::get() requires a strtolower() call. This was previously saved for multiple calls to the same service with e4b3039, but we can also avoid the initial call too but slightly re-organising the code. Most of the diff is indentation.
Drupal.org issue here which shows the (very modest, about 0.5ms in my profiling) performance improvement: https://www.drupal.org/node/2480943