Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
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
stloyd commentedSep 1, 2013
This is wrong as some services can be |
catch56 commentedSep 1, 2013
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. |
lsmith77 commentedSep 1, 2013
didn't we go back and forth here before? |
catch56 commentedSep 1, 2013
Well remembered. I did a search and found it discussed here:#7007 (diff) But isset() || array_key_exists() wasn't mentioned in that exchange. |
fabpot commentedSep 1, 2013
@catch56 Does it bring a noticeable performance improvement on a full page? |
catch56 commentedSep 1, 2013
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). |
catch56 commentedSep 2, 2013
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. |
fabpot commentedSep 2, 2013
@catch56 As this is also a performance optimization, feel free to do everything in this PR. That makes things easier for everyone. Thanks. |
catch56 commentedSep 2, 2013
OK sounds good, thanks! |
catch56 commentedSep 2, 2013
Pushed that commit. Here's before after xhprof UI screenshots showing the calls removed. |
catch56 commentedSep 3, 2013
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
fabpot commentedSep 3, 2013
merged in 2.3, thanks. |
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.