Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Use isset() instead of array_key_exists() in DIC#8907
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
This is wrong as some services can be |
Yes I'm sure about xdebug being disabled (although it looks like you deleted your comment?). isset() || array_key_exists() handles the case of a null service, and still gives me significant reduction in calls. |
didn't we go back and forth here before? |
Well remembered. I did a search and found it discussed here:#7007 (diff) But isset() || array_key_exists() wasn't mentioned in that exchange. |
@catch56 Does it bring a noticeable performance improvement on a full page? |
It's 0.8 of a millisecond on my local install and 1/45th of the total function calls on the page. That's not huge but yes it's a measurable improvement (i.e. ten similar changes and they'd add up to 8ms). |
I think we can likely save some of the 1,200 or so strtolower() calls here as well - by checking for the string as passed in first in the common cases, then falling back to strtolower() later. That's roughly a millisecond on my machine as well - will put it in a new pull request but it'll need to depend on this one since it's the same lines of code. |
@catch56 As this is also a performance optimization, feel free to do everything in this PR. That makes things easier for everyone. Thanks. |
OK sounds good, thanks! |
Pushed that commit. Here's before after xhprof UI screenshots showing the calls removed. |
Because that's Drupal coding standards which my brain defaults to. Updated. |
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes#8907).Discussion----------Use isset() instead of array_key_exists() in DICIt doesn't look like the array_key_exists() in Container::get() and Container::has() is necessary. Changing this to isset() removed over 1,000 calls on a default Drupal 8 install - attaching before/after xhprof screenshots with this change.Commits-------3c01ae6 Use isset() instead of array_key_exists() in DIC
merged in 2.3, thanks. |
| // available services. Service IDs are case insensitive, however since | ||
| // this method can be called thousands of times during a request, avoid | ||
| // calling strotolower() unless necessary. | ||
| foreach (array(false, true) as $strtolower) { |
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.
Does flatten code would be more efficient than this loop ?
It doesn't look like the array_key_exists() in Container::get() and Container::has() is necessary. Changing this to isset() removed over 1,000 calls on a default Drupal 8 install - attaching before/after xhprof screenshots with this change.