Performance improvement: Reduce the use of the _wp_array_get function

[aristath]

What?

_wp_array_get() is an expensive function, and it's called thousands of times on each page-view on the frontend. In https://core.trac.wordpress.org/ticket/58376 we improved slightly the performance of that function, but the fact remains that it's called more times than it should.

Why?

Because performance & sustainability.

How?

Inspiration came from a discussion with @jrfnl

In many cases, we can replace _wp_array_get() with a much simpler and faster isset() check.
The isset() function is capable of checking nested arrays, so isset( $foo['a']['b']['c'] ) will return false even if $foo['a'] is unset, without throwing any errors/warnings etc.
When _wp_array_get() cannot be directly replaced with isset(), it would be good practice to wrap it in an isset function so _wp_array_get will only run when it needs to.

In order to simplify the syntax, instead of using isset() checks, we can use the Null coalescing operator, which further simplifies the whole implementation.
So we can replace for example this line:

$has_font_family_support = _wp_array_get( $typography_supports, array( '__experimentalFontFamily' ), false );

with this:

$has_font_family_support = $typography_supports['__experimentalFontFamily'] ?? false;

which would be the equivalent of this:

$has_font_family_support = isset( $typography_supports['__experimentalFontFamily'] )
    ? $typography_supports['__experimentalFontFamily']
    : false;

The resulting code is shorter, easier to understand, and faster.

Of course the above modifications can only work when we know the path we're looking for. If we don't, then _wp_array_get should still be used.

[github-actions]

Flaky tests detected in 48efd0aa7133b7273e663700030a09f67cb211b4.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6168673762
📝 Reported issues:

• [Flaky Test] should insert elements from multiple completers in a single block #47916 in /test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js

[spacedmonkey]

@aristath This looks good. I have one bit of feedback. This PR needs a rebase. Is there a plan for how this will be backported?

[felixarntz]

@aristath This looks mostly good, though I have one point that applies in several parts of the PR where I wonder why it's not optimized further.

If we already use isset, we should also avoid calling _wp_array_get() afterwards. Otherwise, the performance may technically be worse than before since we just add an isset call basically.

[felixarntz]

Related: https://core.trac.wordpress.org/ticket/58532

[youknowriad]

I'm curious, do we have any data that proves that this kind of refactoring improve some important metric. (Not talking about number of function calls or things like that but more LCP, TTFB or similar) I'm personally very suspicious of refactorings like have the potential of degrading the readability of code of unclear performance gains.

To clarify I'm all for this change if we have data that proves that it's effective.

[spacedmonkey]

@youknowriad Using XHProf, I did a profile on home page of tt3 themed site.

This function gets called ALOT. Resulting in around 2% of page load. More complex / blocks on a page is more than it is called. So trying to avoid this function will improve performance. Using an isset, an in built php function is always going to be faster. I would argue that isset is more readable, as it is a core function.

[youknowriad]

@spacedmonkey Can we compare the results of the current PR with trunk and see the results. The number of function calls is not important to me, the real metric is whether it makes the server response faster.

Using an isset, an in built php function is always going to be faster

I don't doubt that. Is there a way to rewrite the function internally to use isset?

I would argue that isset is more readable, as it is a core function.

I guess that's a bit subjective, but it's fine, I don't argue code style.

---

It may be fine for this particular change but what I'm trying to convey is that we should attempt refactoring code for performance without really testing before/after important metrics. IMO, the number of function calls is not a real metric.

[aristath]

It may be fine for this particular change but what I'm trying to convey is that we should attempt refactoring code for performance without really testing before/after important metrics. IMO, the number of function calls is not a real metric.

I agree... Unfortunately my testing environment for PHP profiling is rather basic and unreliable (XDebug + Webgrind) so results vary from page-load to page-load and I can't accurately compare the results, there's a large margin of error. It's an OK environment for big changes, but not for smallish ones.
I know instinctively that this is a good performance improvement, but I can't provide accurate numbers. If someone has a more reliable profiling setup that would be great ❤️

[youknowriad]

Excuse my devil's advocate tone, I'm just trying to make sure that we all collectively work on the right things and don't spend time on things that are not that important.

So, if we can't measure the improvement in a meaningful metric like TTFB, even if there's an improvement within the margin of error, is it really worth the hassle? I'm pretty sure you can take any function in our code base, unit test it and improve its performance by checking the unit test time for instance, but is it worth it?

Thanks for doing this work @aristath I really value performance work as you know, I just want to add to the discussion so we can get some common understanding on how we should approach performance work.

[aristath]

I completely understand!
Even though my environment is not 100% accurate (and there's also an overhead from XDebug itself), it consistently shows an improvement in load-time.

Before (trunk):

After (this PR):

The 1st column is the function calls, the 2nd column is time in milliseconds.
There's an improvement of ~10ms on my localhost. I ran the tests multiple times and though the numbers vary slightly, the difference is there and ranges from 7 to 12ms.
Due to XDebug's overhead, the total page-load is ~3s, so in theory I'd expect on a normal production site this PR would improve performance by ~1-2ms.

I really value performance work as you know, I just want to add to the discussion so we can get some common understanding on how we should approach performance work.

It would be really nice if we come up with a proper methodology to handle performance improvements... Right now we just test things and whatever catches our attention is what we work on 😅

[jrfnl]

It would be really nice if we come up with a proper methodology to handle performance improvements... Right now we just test things and whatever catches our attention is what we work on

Might be worth it to look into getting a BlackFire account ?

[youknowriad]

We already have https://www.codevitals.run/project/wordpress with some metrics, that's a good start. We should add good metrics that represent meaningful bottlenecks for user interactions.

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/block-supports/background.php
❔ lib/block-supports/border.php
❔ lib/block-supports/colors.php
❔ lib/block-supports/dimensions.php
❔ lib/block-supports/layout.php
❔ lib/block-supports/position.php
❔ lib/block-supports/settings.php
❔ lib/block-supports/spacing.php
❔ lib/block-supports/typography.php
❔ lib/class-wp-duotone-gutenberg.php
❔ lib/class-wp-theme-json-gutenberg.php
❔ lib/class-wp-theme-json-resolver-gutenberg.php
❔ lib/experimental/blocks.php

[godismyjudge95]

I don't mean to barge in on a random PR, but now that WordPress 6.3+ only supports PHP 7.0+ a lot of this could be cleaned up even further with the null coalescing operator.

[aristath]

Thank you for the comments and the review @andrewserong!
I addressed the feedback 👍

[andrewserong]

Thanks for the follow-ups @aristath, those lines in the colors block support are much more readable now, that looks great 👍
Nice work landing this change! 🎉

[tellthemachines]

@aristath are you able to create a Trac ticket and open a PR in wordpress-develop to sync these changes to core?

[aristath]

@tellthemachines I already started the PR yesterday in WordPress/wordpress-develop#5244, as soon as tests pass I'll create the ticket on trac and submit the patch 👍