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://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existanc...

Proposed resolution

Before
if (array_key_exists($key, $array))

After
if (isset($array[$key]) || array_key_exists($key, $array))

Performance profiling

Scenario: 50 nodes generated on the homepage.

With warm cache but caches turned off with example.settings.local.php used.

With warm cache but caches turned off

After drush cr with the same.
After drush cr with the same

Exceptions

If the array variable can potentially be an object without array access, array_key_exists() will ignore that but isset() will throw an Fatal Error in trying to use it as an array.
Example

// drush scr test.php
use Symfony\Component\HttpFoundation\ParameterBag;
$name = 'test';
$raw_parameters = new ParameterBag([$name => $name]);
if (isset($raw_parameters[$name]) && array_key_exists($name, $raw_parameters)) {
  print 'test';
}
CommentFileSizeAuthor
#46 isset_high_performance_with_array_key_exists-2770065-46.patch1.81 KBjoseph.olstad
#35 array_key_exists-2770065-35.patch41.64 KBjoseph.olstad
#34 array_key_exists-2770065-34.patch41.64 KBjoseph.olstad
#30 array_key_exists-2770065-30.patch42.39 KBjoseph.olstad
#29 array_key_exists-2770065-29.patch42.38 KBjoseph.olstad
#27 array_key_exists-2770065-27.patch42.38 KBjoseph.olstad
#25 array_key_exists-2770065-25.patch42.38 KBjoseph.olstad
#18 XHProf__Hierarchical_Profiler_Report.png486.6 KBjoelpittet
#18 XHProf__Hierarchical_Profiler_Report.png340.52 KBjoelpittet
#17 interdiff.txt636 bytesjoelpittet
#17 array_key_exists-2770065-17.patch43.72 KBjoelpittet
#12 array_key_exists-2770065-12.patch43.72 KBjoelpittet
#10 interdiff.txt1.74 KBjoelpittet
#10 array_key_exists-2770065-10.patch30.12 MBjoelpittet
#3 array_key_exists-2770065-3.patch44.79 KBjoelpittet
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Status: Active » Needs review
FileSize
44.79 KB

Status: Needs review » Needs work

The last submitted patch, 3: array_key_exists-2770065-3.patch, failed testing.

dawehner’s picture

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.

joelpittet’s picture

May have found bugs if the logic is correct(and I didn't make typos).

But yeah, I agree @dawehner. The ones that are called lots will have the largest impact. Knowing that is another problem though.

dawehner’s picture

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.

joelpittet’s picture

Issue tags: +Needs profiling

We can profile this to see what kind of overall time difference. Need to fix the fails (and find out why the fails) first, then we can profile to see. Following that I think we can start profiling.

This one could be a not of work for little gain, but I'm hoping to either learn something new that I didn't know before about that PHP optimization pattern or make some performance gains:)

joelpittet’s picture

Issue summary: View changes

( ! ) Fatal error: Cannot use object of type Symfony\Component\HttpFoundation\ParameterBag as array in core/lib/Drupal/Core/Controller/ControllerResolver.php on line 139

That's an interesting one:) ParameterBag is implementing IteratorAggregate, Countable. So array_key_exists() ignores that caveat.

Added to issue summary!

joelpittet’s picture

Status: Needs work » Needs review
FileSize
30.12 MB
1.74 KB

Most of the errors were on that, didn't check them all locally but a couple I did checked passed. So let's try this without that ControllerResolver change. (we could put a is_array() check in that which may be idea actually because is_ type checks are quite fast too)

Status: Needs review » Needs work

The last submitted patch, 10: array_key_exists-2770065-10.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
43.72 KB

Lol diffed against 8.0.x. Well if someone wants to upgrade, that's the patch.

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 12: array_key_exists-2770065-12.patch, failed testing.

joelpittet’s picture

The last submitted patch, 12: array_key_exists-2770065-12.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
43.72 KB
636 bytes

Sweet one more test to pass and that was a logic error I introduced to the diff. Much simpler to do !(isset() || array_key_exists) in parens for the negation

joelpittet’s picture

Scenario: 50 nodes generated on the homepage.

With warm cache but caches turned off with example.settings.local.php used.

After drush cr with the same.

This is totally minor, but maybe we can do the top ones that made a difference?

Suggesting these ones if any:

Drupal\Component\Utility\NestedArray::getValue
Drupal\Core\Utility\ThemeRegistry::has
Drupal\views\Plugin\views\display\DisplayPluginBase::getOption
Drupal\views\Plugin\views\display\DisplayPluginBase::getOption
Drupal\Core\Field\PluginSettingsBase::getSetting
Drupal\Component\Utility\NestedArray::unsetValue
Drupal\field\Entity\FieldStorageConfig::getSetting
Drupal\Core\Field\FieldConfigBase::getSetting
Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::setValue
template_preprocess_toolbar
dawehner’s picture

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joseph.olstad’s picture

Status: Needs review » Needs work

patch needs a reroll.

joseph.olstad’s picture

Assigned: Unassigned » joseph.olstad
Status: Needs work » Active

patch to follow shortly

joseph.olstad’s picture

Status: Needs review » Needs work

The last submitted patch, 25: array_key_exists-2770065-25.patch, failed testing. View results

joseph.olstad’s picture

Status: Needs work » Needs review
FileSize
42.38 KB

Status: Needs review » Needs work

The last submitted patch, 27: array_key_exists-2770065-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

Status: Needs work » Needs review
FileSize
42.38 KB

This one should pass

joseph.olstad’s picture

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

The last submitted patch, 29: array_key_exists-2770065-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 30: array_key_exists-2770065-30.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

Status: Needs work » Active

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

joseph.olstad’s picture

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’s picture

You can use this issue if you want.
#2465399: IGNORE: Patch testing issue

joseph.olstad’s picture

Issue summary: View changes
joseph.olstad’s picture

@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

joelpittet’s picture

It's for when a change may affect many tests and may fail a lot before it's ready, to avoid spamming the original issue with experiments.

It's a normal issue with a name that others have used for this purpose.

joseph.olstad’s picture

Assigned: joseph.olstad » Unassigned
Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

willzyx’s picture

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

joseph.olstad’s picture

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
dawehner’s picture

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.

dawehner’s picture

Status: Needs review » Needs work
joseph.olstad’s picture

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

joseph.olstad’s picture

Status: Needs work » Needs review
joseph.olstad’s picture

All green, RTBC anyone?

joelpittet’s picture

Issue tags: +Vienna2017, +Novice

RTBC +1 from me but I made a patch in this, so not worthy;)

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

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.

joseph.olstad’s picture

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

dsutter’s picture

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.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

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?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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.

joseph.olstad’s picture

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.

dawehner’s picture

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?

joseph.olstad’s picture

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.

dawehner’s picture

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 :)

larowlan’s picture

Adding review credits

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as aaefc3f and pushed to 8.5.x.

  • larowlan committed aaefc3f on 8.5.x
    Issue #2770065 by joseph.olstad, joelpittet, dawehner, catch, willzyx:...
joelpittet’s picture

Thank you 🎉🎉

Status: Fixed » Closed (fixed)

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