Problem/Motivation

In old versions of PHP, array_key_exists($key, $arr) was (much) slower than isset($arr[$key]) and consequently the following pattern, although functionally redundant, was adopted for performance reasons in cases where the first test was likely to succeed:

isset($arr[$key]) || array_key_exists($key, $arr)

This is now an anti-pattern, as the performance issue with array_key_exists() has already been resolved in all versions of PHP that we care about, and consequently the inclusion of the redundant isset() will now be having a negative net effect on performance, rather than a positive one. Equally importantly, we can get rid of code which looks wrong.

Refer to https://bugs.php.net/bug.php?id=71239 for upstream confirmation:

array_key_exists() has an optimized opcode implementation since PHP 7.4.

Heed comment #5 below, however -- in namespaced code, it's necessary to either call \array_key_exists() or else use function array_key_exists; otherwise the opcode will not be used.

CommentFileSizeAuthor
#8 drupal-10.1.x-core-isset_array_key_exists-3346645-8.patch7.95 KBjweowu
#2 drupal-10.1.x-core-isset_array_key_exists-3346645-2.patch7.93 KBjweowu

Comments

jweowu created an issue. See original summary.

jweowu’s picture

jweowu commented
Status: Active » Needs review
StatusFileSize
new drupal-10.1.x-core-isset_array_key_exists-3346645-2.patch7.93 KB

This patch omits the instance in drupal_static() which is accounted for already in the patch for #3136041: drupal_static() edge case bug and inconsistent comments (which inspired this new issue).

catch’s picture

catch
he/him
Primary language English
commented 
+++ b/core/lib/Drupal/Component/DependencyInjection/Container.php
@@ -320,7 +320,7 @@ public function has($id): bool {
   public function getParameter($name): array|bool|string|int|float|NULL {
-    if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters))) {
+    if (!array_key_exists($name, $this->parameters)) {
       if (!$name) {

Should we change this to \array_key_exists() in namespaced code at the same time?

jweowu’s picture

jweowu commented

I'm not seeing anything in the Drupal coding standards indicating that function calls in the global name-space should have a backslash prefix. It does say this about global classes, but that's all; and a cursory grep is only showing a tiny number of such function calls anywhere in the codebase.

I don't mind making that change if need be, but it seems out of scope?

jweowu’s picture

jweowu commented
Status: Needs review » Needs work

I was wrong -- I see now that this tweak is actually required for namespaced code, so I'll go ahead and re-roll the patch.

https://www.php.net/manual/en/migration74.other-changes.php says:

A specialized VM opcode for the array_key_exists() function has been added, which improves performance of this function if it can be statically resolved. In namespaced code, this may require writing \array_key_exists() or explicitly importing the function.

https://bugs.php.net/bug.php?id=79675 indicates use function array_key_exists; is the correct way to explicitly import the function, and following the cues from that bug I can confirm the other approach practically, courtesy of pecl install vld-beta:

$ php -dvld.active=1 -dvld.execute=0 -r 'array_key_exists(1, []);'

#* E I O op                           operands
-----------------------------------------------
0  E >   ARRAY_KEY_EXISTS             1, <array>
1        FREE                         ~0
2      > RETURN                       null

$ php -dvld.active=1 -dvld.execute=0 -r 'namespace a; array_key_exists(1, []);'

#* E I O op                           operands
-----------------------------------------------
0  E >   INIT_NS_FCALL_BY_NAME        'a%5Carray_key_exists'
1        SEND_VAL_EX                  1
2        SEND_VAL_EX                  <array>
3        DO_FCALL
4      > RETURN                       null

$ php -dvld.active=1 -dvld.execute=0 -r 'namespace a; \array_key_exists(1, []);'

#* E I O op                           operands
-----------------------------------------------
0  E >   ARRAY_KEY_EXISTS             1, <array>
1        FREE                         ~0
2      > RETURN                       null
jweowu’s picture

jweowu commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented

#5 is right, we probably should have a coding standard, but until then using the explicit namespace as a performance optimisation is fine - sorry could have been more explicit why we needed to do it but glad you found it, and hadn't come across vld before - looks handy!

jweowu’s picture

jweowu commented
Status: Needs work » Needs review
StatusFileSize
new drupal-10.1.x-core-isset_array_key_exists-3346645-8.patch7.95 KB

I've added the backslash prefix irrespective of namespacing, as I feel that this opcode/no-opcode performance gotcha is ridiculously esoteric, and so it's probably better if the backslash is employed in all cases (there should be a follow-up issue for code not affected by this patch) so that that it'll look wrong to ever see this function called without a backslash.

(And perhaps after that, the test bot could also be made to check for un-backslashed calls.)

catch’s picture

catch
he/him
Primary language English
commented

Opened #3346987: Call PHP native functions fully qualified (like \array_key_exists()).

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

@jweowu, thanks for this initiative. It's good that PHP fixed this performance bug and will allow us to have code which is simpler. A good initiative for symfony based Drupals that require the very latest PHP.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Applied patch #8

Searched for ") || array_key_exists('" and got 0 results in the repo.

  • catch committed e48191a9 on 10.1.x 
    Issue #3346645 by jweowu, catch, smustgrave: Eliminate anti-pattern...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

This is good for a readability improvement and now micro-optimization now that PHP has the opcode implementation. We can try to make fully qualified functions a coding standard in #3346987: Call PHP native functions fully qualified (like \array_key_exists()).

Committed/pushed to 10.1.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.