Problem/Motivation

isset() is much faster than array_key_exists().

Proposed resolution

Use isset() Use both with a shortcut || after isset()

CommentFileSizeAuthor
#14 get_option_micro-2760419-14.patch475 bytesjoseph.olstad
#2 get_option_micro-2760419-2.patch520 bytesjoelpittet
get_option.png99.9 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Active » Needs review
Issue tags: +Quick fix
FileSize
get_option_micro-2760419-2.patch520 bytes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Title: get_option micro optimization » get_option() micro optimization
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

100x faster or 3ms:P!

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Bravo! Great work @JoelPittet.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

We've been using this patch in production for a few weeks, and on our dev machines, it's excellent!

Thanks again.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs review » Reviewed & tested by the community
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Issue tags: +minor version target

joelpittet has done some great work here. Adding the 'minor version target' tag.
Please include this for 7.x-3.17

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Related issues: +#2653266: Avoid excess t() calls in field_views_field_default_views_data(), +#2760405: Reduce the calls to the same t() in views_fetch_fields(), +#2653214: Replace views_include() with include_once

These other performance fixes all play nice together. I recommend all of them for 7.x-3.17 please and thanks.

dsutter’s picture

dsutter CreditAttribution: dsutter as a volunteer commented

RTBC+ patch #2

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

What's the benefit of keeping the array_key_exists() logic? isset() on its own should suffice.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Have you looked at making this (and the other) performance improvement in D8?

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Reviewed & tested by the community » Needs work

reroll to follow , dropping array_key_exists

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs work » Needs review
FileSize
get_option_micro-2760419-14.patch475 bytes
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

D8 patch as been rolled
#2903843-1: views get_option() micro optimization

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Issue summary: View changes
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs review » Reviewed & tested by the community
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#2903843: views get_option() micro optimization
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Hmm, DamienMckenna Joel_Pittets patch is the way to go.

we need to check that the array_key_exists because isset when the array element value is null would return false (meaning the test would fail) but we want to know either way.

So back to Joel_Pittets patch. Otherwise we risk breaking something.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

according to Joel_Pittet , this is a 100x improvement
in relative terms, 3ms
We've been using his patch verbatim for quite a while now, it's holding up great.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

That crafty PHP! Thanks for digging into it, joseph.olstad.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Thanks to DSutter for the php in-depth analysis, he's sitting beside me here. A php wizard of wizards.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

Isset doesn't keep null values but array_key_exists does, I have a number of other d8 issues that make this optimization, don't have the reference off hand(on my phone) but easily googleable.

I didn't want to change logic as to make this change equivalent to what was there.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

yes agreed #23, basically what we were trying to say in #19, but helps explaining it in that way as well. PHP trickiness
nice work JoelPittet

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

Sorry, I didn't read the back scroll, you guys figured out that stuff within a span of a few hours. Nice:)

willzyx’s picture

willzyx CreditAttribution: willzyx commented

@joelpittet great work! maybe unrelated by I'm thinking that the same approach (isset() || array_key_exists()) could be used even in d7 core in ThemeRegistry::offsetExists. Not tested but probably could prevent a lot of calls to array_key_exists function

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

@willzyx, that's the one I was bumbling about in #23 on my phone:) Here's the link #2863786: D7 ThemeRegistry array_key_exists() micro-optimization #2770065: array_key_exists() micro-optimization

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

Here's a link to find others like this:)
https://www.drupal.org/project/issues/search?text=&projects=&assigned=&s...

willzyx’s picture

willzyx CreditAttribution: willzyx commented

@joelpittet good to know :)

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

FYI, we're ready.
This was committed to D8 views (in core 8.4.x and 8.5.x branches)
Thanks to Joel_pittet for the original patch.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Parent issue: » #2903944: Plan for Views 7.x-3.19 release

Probably patch in #14 #2 is a good candidate for 7.x-3.19 release

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

@willzyx, the original patch #2 is the way to go, patch #14 is NOT the way. #14 was hidden.

joel_pittet patch is what all the related patches are based on.

willzyx’s picture

willzyx CreditAttribution: willzyx commented

@joseph.olstad oops you are right, my mistake :). Patch #2 is the way to go

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

Status: Fixed » Closed (fixed)

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