Make WordPress Core

Opened 18 months ago

Closed 18 months ago

Last modified 9 months ago

#56974 closed defect (bug) (fixed)

Improve performance of the WP_Theme_JSON class

Reported by: aristath's profile aristath Owned by: flixos90's profile flixos90
Milestone: 6.1.1 Priority: normal
Severity: normal Version: 6.2
Component: Editor Keywords: has-patch has-unit-tests fixed-major
Focuses: performance Cc:

Description

As pointed out in other tickets during the 6.1 release cycle, the WP_Theme_JSON is one of the biggest bottlenecks (if not the biggest) when loading a page using a block theme.
Some of the methods inside WP_Theme_JSON call a lot of expensive array functions - and their effect is compounded by the fact that they usually run hundreds of times as part of deeply-nested loops.
We can improve the performance of these methods by simplifying all these calls to faster, simpler, and usually more readable alternatives.

Change History (24)

This ticket was mentioned in PR #3555 on WordPress/wordpress-develop by @aristath.


18 months ago
#1

  • Keywords has-patch has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/56974

This PR simplifies expensive array-functions calls where appropriate.

Attaching some profiling results to showcase the impact.

Before (trunk):
https://i0.wp.com/user-images.githubusercontent.com/588688/199669150-1fcf3ccc-f0fc-49b7-b54d-9a4eb3635398.png

After (with this PR):
https://i0.wp.com/user-images.githubusercontent.com/588688/199669142-886ff31f-ab79-43fb-801a-94a9d61892d5.png

All numbers in the screenshots above use milliseconds, and the impact of these seemingly simple changes is bigger than expected, shaving off more than 200ms. Of course those numbers are a bit inflated due to the fact that profiling slows down a page significantly, but the overall improvement on the page-load is more than 3%.

#3 @matt
18 months ago

Cool!

#4 @SergeyBiryukov
18 months ago

  • Milestone changed from Awaiting Review to 6.2

#5 @aristath
18 months ago

  • Milestone changed from 6.2 to 6.1.1

@spacedmonkey commented on PR #3555:


18 months ago
#6

While doing some profiling, I noticed there are still calls to _wp_array_get. This calls array_key_exists, which is expensive. I wonder if we could avoid these somehow.

@spacedmonkey commented on PR #3555:


18 months ago
#7

I wonder if we remove some uses of _wp_array_get. This function calls array_key_exists internally. If we can remove a couple of uses of this, it would give us a performance win here.

@spacedmonkey commented on PR #3555:


18 months ago
#8

I wonder if we remove some uses of _wp_array_get. This function calls array_key_exists internally. If we can remove a couple of uses of this, it would give us a performance win here.

@jrf commented on PR #3555:


18 months ago
#9

While doing some profiling, I noticed there are still calls to _wp_array_get. This calls array_key_exists, which is expensive. I wonder if we could avoid these somehow.

I wouldn't worry (or overestimate) the impact of array_key_exists().

Note: isset() and array_key_exists() also have a _functional_ difference in how they treat a null value, so for array_key_exists() to be equivalent to isset(), it MUST be accompanied by a && $var !== null.

@aristath commented on PR #3555:


18 months ago
#10

Note: isset() and array_key_exists() also have a _functional_ difference in how they treat a null value, so for array_key_exists() to be equivalent to isset(), it MUST be accompanied by a && $var !== null.

Yeah, that's an important distinction!
In the comments I added for future reference to replace array_key_exists() with isset() (// Note: array_key_exists() should be replaced with an isset() check once WordPress drops support for PHP 5.6.), I checked the values in each case and only added that comment in cases where the value can not be null.
In one case the value _can_ be null, and I added a comment in that one that // We need to use array_key_exists() instead of isset() because the value can be null. 👍

@spacedmonkey commented on PR #3555:


18 months ago
#11

In trunk.
https://i0.wp.com/user-images.githubusercontent.com/237508/200950234-4d0c7513-3721-4640-ac26-a74b6cf02d69.png

In this brunch.
https://i0.wp.com/user-images.githubusercontent.com/237508/200950270-30fd48a7-ac96-4cb4-a541-ab28e5684e78.png

@flixos90 commented on PR #3555:


18 months ago
#12

@jrfnl Following up on array_key_exists: Makes sense not to overthink this in terms of performance impact compared to isset, however I had one follow up thought I wanted to at least raise here:

  • Most of the usage of array_key_exists here could be replaced with isset as the additional null consideration is irrelevant for most (if not all) of the code where it's used here.
  • Given that, there would be the alternative option of assigning the result to a variable and then calling isset on that variable.

For example, instead of:
{{{php
if ( array_key_exists( $element, static::EXPERIMENTAL_ELEMENT_CLASS_NAMES ) ) {
}
}}}

Do:
{{{php
$element_class_names = static::EXPERIMENTAL_ELEMENT_CLASS_NAMES;
if ( isset( $element_class_names[ $element ] ) {
}
}}}

Again, not saying this is necessarily worth doing (and certainly not a blocker), but I'd be curious if the first or the last is generally faster. :)

@jrf commented on PR #3555:


18 months ago
#13

There's bound to be more room for improvement, but yes, let's take this step by step and I do believe this is a valuable step in that.

Re: assign+isset vs array_key_exists: as most array_key_exists() are only in place for PHP 5.6, might be more effective to re-open the discussion on dropping support for PHP 5.6 ... 😇

On that note - PHP 8.3 will bring another very useful improvement `json_validate()` (validate a JSON input before decoding without running the risk of the JSON tying up all the memory), but that is no doubt still a bridge too far ;-)

#14 @flixos90
18 months ago

  • Owner set to flixos90
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


18 months ago

#16 @flixos90
18 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 54804:

Editor: Improve performance of WP_Theme_JSON class by reducing usage of expensive array functions.

In many scenarios array functions are more expensive than using simpler for or foreach loops.

This changeset results in roughly 4% faster wp_head execution time for both block themes and classic themes. While this may seem like a small win, it is a worthwhile enhancement and only one part of several other little performance tweaks which are being worked on to improve performance of theme.json parsing further.

Props aristath, desrosj, jrf, spacedmonkey.
Fixes #56974.
See #57067.

#18 @flixos90
18 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 6.1.1.

#19 @flixos90
18 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 54805:

Editor: Improve performance of WP_Theme_JSON class by reducing usage of expensive array functions.

In many scenarios array functions are more expensive than using simpler for or foreach loops.

This changeset results in roughly 4% faster wp_head execution time for both block themes and classic themes. While this may seem like a small win, it is a worthwhile enhancement and only one part of several other little performance tweaks which are being worked on to improve performance of theme.json parsing further.

Props aristath, desrosj, jrf, spacedmonkey.
Merges [54804] to the 6.1 branch.
Fixes #56974.
See #57067.

#20 @flixos90
15 months ago

In 55142:

Editor: Fix undefined variable following [54805].

Props mamaduka, costdev, mukesh27.
See #56974, #57067.

#21 @flixos90
15 months ago

@jeffpaul Do we need to backport the fix from [55142] to the 6.1 branch? If so, should this ticket be reopened for that purpose? Not sure what the timeline for a potential 6.1.2 is.

#22 @JeffPaul
15 months ago

No immediate 6.1.2 that I have planned / I know is planned. @priethor @desrosj @mikeschroder @davidbaumwald @czapla @bernhard-reiter @cbravobernal as other 6.1 RC/Core&Editor Tech Leads and @hellofromTonya @webcommsat as core team reps, are any of you aware of / planning a 6.1.2?

This ticket was mentioned in PR #4936 on WordPress/wordpress-develop by @Soean.


9 months ago
#23

This was raised as part of #56974, which reviewed several of these checks.isset is slightly preferable overarray_key_exists in terms of performance (with the caveat that the two behave differently when it comes to null values).

Trac ticket: https://core.trac.wordpress.org/ticket/57067

Gutenberg PR: https://github.com/WordPress/gutenberg/pull/53098

Note: See TracTickets for help on using tickets.