array_key_exists() micro-optimization

IMHO fixing parts of core which are called often, this might make sense. Fixing it all over the place is a bit unfortunate because it reduces readability.

It is hard to judge, maybe we should start with a limit of places where we thin we might have something costly. Still, I doubt you can really easily measure a time difference.

So you show the time spent in array_key_exists() but yeah, isset() is essentially for free.

It is not only that there is less time spend, the amount of noise for profiling people is reduced, so there is another positive aspect on that side of things.

patch needs a reroll.

patch to follow shortly

This one should pass

This one should also pass, space indent style correction in core/modules/migrate/src/Plugin/migrate/process/SubProcess.php

I'll review and reroll shortly, I noticed a few more copy / paste issues.

unfortunately I don't have a test system ready with phpUnit installed (prerequisite for D8 testing suite), so relying on d.o for tests.

@joelpittet ? the IGNORE patch testing issue, is that for ignoring PHP Unit so that I can run tests locally without PHP Unit?
there's little description in that issue. ? please let me know

Great work joel_pittet , now if we can get this committed we won't have to keep rerolling it.

isset() is nearly free, but it's not entirely free. So if the isset() checks fail more often than not, it's not necessarily an improvement.

I agree with catch, we should use the isset() || array_key_exists() approach where it make sense. Probably the best candidates in core for this are:
Drupal\Core\Utility\ThemeRegistry::has
Drupal\Core\KeyValueStore\MemoryStorage::has
Drupal\Core\KeyValueStore\MemoryStorage::get
Drupal\Component\Utility\NestedArray::getValue
Drupal\Component\Utility\NestedArray::unsetValue

For views already exists an RTBC issue #2903843: views get_option() micro optimization

Anyway I think the the performance gain whit this patch for d8 is minimal, specially if you run php 7+.. Could make more sense for d7 as proposed here #2863786: D7 ThemeRegistry array_key_exists() micro-optimization

ok, sounds good, so reroll a smaller patch doing this just for:

Drupal\Core\Utility\ThemeRegistry::has
Drupal\Core\KeyValueStore\MemoryStorage::has
Drupal\Core\KeyValueStore\MemoryStorage::get
Drupal\Component\Utility\NestedArray::getValue
Drupal\Component\Utility\NestedArray::unsetValue

Drupal\Core\KeyValueStore\MemoryStorage::has
Drupal\Core\KeyValueStore\MemoryStorage::get

I would vote for skipping those as well, as you would not use a memory storage in the critical path.

The requested/suggested changes have been made according to #2770065-43: array_key_exists() micro-optimization and additional suggestion at #2770065-44: array_key_exists() micro-optimization . So yes I agree with #44. see new patch

For background information and justification of this performance patch please read: #2903843-13: views get_option() micro optimization

This gives us the simplest patch with the biggest positive impact on performance (as noted the biggest gains will be on php 5.6.x however still gains in php 7.x as well). Side note: php 5.6 will get security updates until Dec 31 2018 whereas php 7.0 will only get security updates until Dec 1 2018
http://php.net/supported-versions.php

All green, RTBC anyone?

I made exactly the changes to the patch as recommended by the core maintainers.

For more information on this performance improvement please see php.net array_key_exists AND also this detailed comparison between php versions

If you want to take the performance advantage of isset() while keeping the NULL element correctly detected, use this:

if (isset(..) || array_key_exists(...))
{
...
}

Benchmark (100000 runs):
array_key_exists() : 205 ms
is_set() : 35ms
isset() || array_key_exists() : 48ms

php version             5.6.12  7.0.11
isset() 	        1.186 	0.626
array_key_exists() 	3.375 	1.112

I crunched the numbers myself and came with a conservative estimate of performance improvement for the code in question and resulted in:
in php 5.6.12 expect a 134 % improvement, meaning more than twice as fast!
in php 7.0.11 expect a 56% improvement, meaning over 50% faster!

See this website for detailed Performance statistics between the various PHP versions.

So, the numbers don't lie, based on the stats alone there is a very strong performance argument in favor of this performance patch regardless of the chosen php version.

As per policy (respect thy authori-tie). The D7 performance fix is pending this D8 fix. I've marked the D7 issue as postponed until this issue is fixed.
#2863786: D7 ThemeRegistry array_key_exists() micro-optimization

I looked at the patch and agree that it would be a performance improvement and does not affect the logic. However, if it turned out that the value being tested is NULL most of the time, then there may not be much of a performance improvement. I'd go with it.

Although you were only making changes core maintainers asked, you can't RTBC your own patch.

Can someone else give it one final look over?

I crunched the numbers myself and came with a conservative estimate of performance improvement for the code in question and resulted in:
in php 5.6.12 expect a 134 % improvement, meaning more than twice as fast!
in php 7.0.11 expect a 56% improvement, meaning over 50% faster!

Note: while microopts might sound interesting the total time spend there could still be 1ms. I think looking at an entire page request and see how often these methods are called is abetter way to judge the effectiveness of an optimization.

Nevertheless, these 3 places are probably in the path of a typical rendered page.

According to the xhprof run that joel_pittet logged the few lines of code that this patch touches was executed over 500 times.

so yes, a few milliseconds improvement on a vanilla test environment, however on bigger complex installations of Drupal the improvement will be greater.

it's fine and dandy to see a vanilla site perform fast, however on a serious Drupal site with with hundreds of blocks, millions of nodes, hundreds of taxonomies, a few hundred contrib modules, several content types, several entity types with many fields, a theme with a lot of different twigs/templates and throw in special sauce, then this type of micro optimization becomes more important.

it's fine and dandy to see a vanilla site perform fast, however on a serious Drupal site with with hundreds of blocks, millions of nodes, hundreds of taxonomies, a few hundred contrib modules, several content types, several entity types with many fields, a theme with a lot of different twigs/templates and throw in special sauce, then this type of micro optimization becomes more important.

To play the devil's advocate here, don't these optimisations scale linearly with all the other costs?

To play the devil's advocate here, don't these optimisations scale linearly with all the other costs?

That would seem like a plausible assumption. However given the the almost limitless ways Drupal can be used I'd disagree because some modules might excerpt a heavier load on these core functions than others making this a non-linear optimisation.

Drupal can be used I'd disagree because some modules might excerpt a heavier load on these core functions than others making this a non-linear optimisation.

It'd be happy if this would be true :)

Adding review credits

Committed as aaefc3f and pushed to 8.5.x.

Can these cases please be reverted, for Drupal 9+ at least?

It was obviously a performance bug in PHP, and unsurprisingly 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.

I haven't tested in PHP 7.3, but https://www.drupal.org/docs/system-requirements/php-requirements#php_req... says that 7.3 is not recommended for D9, so I feel it's valid to target a minimum of PHP 7.4 for performance micro-optimisations (and change absurd-looking code back to sensible-looking code in the process).

@jweowu good to know that this has been fixed in php 7.4.20

with that said, please create a new issue and a patch and link the issue to this one.
we can continue this development in a new issue.