The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
isset() is fast but doesn't take in account NULL values, we can use it ahead of an array_key_exists() check to glean some of it's speed but PHP's short circuit ||
http://thinkofdev.com/php-fast-way-to-determine-a-key-elements-existance...
Proposed resolution
Before
if (array_key_exists($key, $array))
After
if (isset($array[$key]) || array_key_exists($key, $array))
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3006123-2.patch | 592 bytes | joelpittet |
Comments
Comment #2
joelpittetComment #3
joelpittetHere are the XHProf results on a a really slow and large permissions page
/admin/people/permissionsComment #4
joelpittetSwapped out the referenced link as it's broken.
Comment #5
joseph.olstadNice work Joel.
Comment #6
joseph.olstadComment #7
joseph.olstadComment #8
joseph.olstadMaintain the RTBC:
1) Has sufficient test coverage
2) To increase our confidence in the test results for the above issue, these two should go in first so we can re-queue php 7.3 and php 5.3 tests.
#3047844: [Regression] Tests fail on PHP 5.3
#3025335: session_id() cannot be changed after session is started
Comment #9
joseph.olstadPending Drupal 7 commit - based on FabianX's comment here on a related micro-optimization:
#2863786-18: D7 ThemeRegistry array_key_exists() micro-optimization
Comment #10
joseph.olstadMaintain RTBC
1) Is already in D8 (similar fix)
2) we already put in performance optimisations similar to this.
3) passes tests
4) has been performance profiled to prove gains
Significant gains in performance here!
Comment #11
mustanggb commentedComment #12
mustanggb commentedComment #13
joseph.olstadComment #14
joseph.olstadComment #15
marcelovaniThis seem to be a very low risk fix, I wonder why it takes 2 years to commit...
Comment #16
joelpittetComment #17
ressaThis issue is included in #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77, so tagging with "Drupal 7.xx target" probably isn't necessary. A new meta issue for a planned D7 release around February 2021 should be created soon, transferring the remaining issues there.
Comment #18
mcdruid commentedLGTM - we'll try to get this into the next release.
Comment #19
fabianx commentedRTBC + 1
Comment #21
mcdruid commentedThanks!
Comment #23
jweowu commentedQuoting #10:
> Is already in D8 (similar fix)
Was this really done in D8?
It was obviously a performance bug in PHP, and it's been fixed. I'm using PHP 7.4.20 and this awkward combo of isset and array_key_exists is slower than array_key_exists on its own (according to the benchmark used in http://thinkofdev.com/php-fast-way-to-determine-a-key-elements-existance... which seems to be where this hack originated), so any such code should be reverted to the simpler and now-faster method.
Comment #24
jweowu commentedAlso, hacks like this should always include comments to explain them, because the otherwise the code looks ludicrous, and is liable to be reverted in future by someone noticing the redundancy.
Comment #25
joseph.olstadYes we got these optimisations into D8 first
Not a hack. Well known performance of isset vs other functions.