Make WordPress Core

Opened 11 months ago

Closed 10 months ago

Last modified 9 months ago

#57502 closed enhancement (fixed)

Add object cache to wp_get_global_settings()

Reported by: oandregal's profile oandregal Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch has-unit-tests gutenberg-merge add-to-field-guide
Focuses: performance Cc:

Description

To improve performance of a request, this ticket proposes adding object cache to wp_get_global_settings().

Why?

To improve the performance of a request by:

  • 40% for themes with theme.json (from 876.05ms to 520.80ms, a total of -355.25ms)
  • 10% for themes without theme.json (from 500.26ms to 450.66ms, a total of -49.60ms)

With the backport, the time a theme with theme.json takes to process a request is almost the same (70ms slower) than a theme without theme.json. See [full dataset](https://docs.google.com/spreadsheets/d/1ulnIA7EDun3vlMrDEcIowT2MwskU1VfScwQ33Ezbows/edit#gid=0).

The work has already been done in Gutenberg and is ready for backporting to Core:

The strategy is similar to what was done in #56975 / [55086].

This work is part of a larger strategy. See https://github.com/WordPress/gutenberg/issues/45171 for a detailed description of initiatives and actionable tasks to work on.

Change History (20)

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


11 months ago
#1

Trac ticket: https://core.trac.wordpress.org/ticket/57502
Related to https://github.com/WordPress/gutenberg/issues/45171

This PR backports:

Props @felixarntz, @aristath, @ramonjd, @spacedmonkey, @anton-vlasenko, @jorgefilipecosta, @mmtr, @mcsf

## What

This PR adds object cache to the existing wp_get_global_settings.

## Why

This PR improves the performance of a request by:

  • 40% for themes with theme.json (from 876.05ms to 520.80ms, a total of -355.25ms)
  • 10% for themes without theme.json (from 500.26ms to 450.66ms, a total of -49.60ms)

After this PR, the time a theme with theme.json takes to process a request is almost the same (70ms slower) than a theme without theme.json. See full dataset.

## How

Adds a cache to the high-level public method that consumers use to query the settings coming from theme.json APIs (WordPress, block, theme, and user data).

## Test

  • Make sure automated test pass.
  • Test block theme (TwentyTwentyThree)
    • Create new colors as user from the GS sidebar and verify they are shown in the color components in the block sidebar (site editor, post editor).
    • Modify theme.json settings of the theme
  • Test classic theme (TwentyTwenty)
    • Verify colors defined by the theme via add_theme_support( 'editor-color-palette', $palette ) work as expected.
    • Disable custom colors by adding add_theme_support( 'disable-custom-colors' ); to the functions.php of the theme. Verify that users cannot add custom colors.

## Performance analysis

### How performance was measured

I've tested this using the same mechanism I use for all performance work I've been doing, so the impact of each PR can be compared against each other:

  • I have loaded the "Hello World!" post as a logged-out user. Conducted the test 9 times and then calculated the median (see full dataset).
  • Compared this PR vs trunk at e27c5a38e371e1db85efeea239accf535227ed39 (svn 55005)
  • Used TwentyTwentyThree for theme with theme.json and TwentyTwenty for a theme with no theme.json.

I'm also sharing the raw data I extracted (cachegrind files) so anyone can open them with their tool of choice and inspect the results. Feel free to DM me if you are interested in setting this up for running the test yourself or anything.

### How to reproduce the experiment yourself

You don't need to, as I've made available the cachegrind data (see section above). Though you may still want to run this yourself for peer review:

  1. Configure docker and enviroment by applying this patch. This tells docker to enable the xdebug profiler, output the performance files by URL, and set WP_DEBUG to false:

{{{diff
diff --git a/.env b/.env
index 63a8169f64..f3a87fbb08 100644
--- a/.env
+++ b/.env
@@ -18,7 +18,7 @@ LOCAL_DIR=src

LOCAL_PHP=latest


# Whether or not to enable Xdebug.

-LOCAL_PHP_XDEBUG=false
+LOCAL_PHP_XDEBUG=true

##
# The Xdebug features to enable.

@@ -31,7 +31,7 @@ LOCAL_PHP_XDEBUG=false

#
# For a full list of accepted values, see https://xdebug.org/docs/all_settings#mode.
##

-LOCAL_PHP_XDEBUG_MODE=develop,debug
+LOCAL_PHP_XDEBUG_MODE=profile

# Whether or not to enable Memcached.
LOCAL_PHP_MEMCACHED=false

@@ -54,7 +54,7 @@ LOCAL_DB_TYPE=mysql

LOCAL_DB_VERSION=5.7


# The debug settings to add to wp-config.php.

-LOCAL_WP_DEBUG=true
+LOCAL_WP_DEBUG=false

LOCAL_WP_DEBUG_LOG=true
LOCAL_WP_DEBUG_DISPLAY=true
LOCAL_SCRIPT_DEBUG=true

diff --git a/docker-compose.yml b/docker-compose.yml
index 739c65f4a8..b781c3fe94 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -39,6 +39,7 @@ services:

environment:

  • LOCAL_PHP_XDEBUG=${LOCAL_PHP_XDEBUG-false}
  • XDEBUG_MODE=${LOCAL_PHP_XDEBUG_MODE-develop,debug}

+ - XDEBUG_CONFIG=profiler_output_name=cachegrind.out.%R

  • LOCAL_PHP_MEMCACHED=${LOCAL_PHP_MEMCACHED-false}
  • PHP_FPM_UID=${PHP_FPM_UID-1000}
  • PHP_FPM_GID=${PHP_FPM_GID-1000}

}}}

  1. Build the environment: npm install && npm run build:dev.
  2. Start docker: npm run env:start && npm run env:install.
  3. Visit the site and load the "Hello World!" post. It should be something along the lines of http://localhost:8889/2022/12/19/hello-world/
  4. Take the cachegrind data generated by xdebug out of docker:

{{{sh
# List and find the latest timestamp of cachegrind.out.* files
docker exec -it docker ps -f name=wordpress-develop_php -q ls -lat --full-time /tmp/
# Copy from docker to manipulate with any tool in your computer
docker cp docker ps -f name=wordpress-develop_php -q:'/tmp/cachegrind.out._2022_12_19_hello-world_.gz' ~/Desktop/
}}}

  1. Inspect the time it takes with any tool of your choice. I use kcachegrind, but there are alternatives such as qcachegrind, webgrind, etc. Note that kcachegrind shows times aggregated by 10ns, so the total included time of the request in the following screenshot would be 35 0442 02 * 10 = 350 442 020ns, which amounts to 350ms.

https://i0.wp.com/user-images.githubusercontent.com/583546/198574623-d1c02a25-1a63-4bf2-808a-f4b7e2f16cbb.png

  1. Repeat 9 times and calculate the median.

#2 @hellofromTonya
11 months ago

  • Keywords gutenberg-merge added

Using an experimental keyword gutenberg-merge to identify Gutenberg backports Trac tickets.

#3 follow-up: @spacedmonkey
11 months ago

Isnt't this a depulicate of #56910?

@oandregal commented on PR #3789:


11 months ago
#4

Rebased from trunk and removed the no longer relevant changes. I didn't have the time to test this manually after the rebase, I'll share here when I do if nobody gets to it first.

@oandregal commented on PR #3789:


11 months ago
#5

I've shared elsewhere that I've been frustrated with the high variability of XDebug results. I know that XDebug are no real numbers, as instrumenting the PHP code adds noise. However, up until this point, I presumed the percentage of variation (whether we improve or not and the order of magnitude) would be similar to production. I no longer think this is true. I'd recommend stop taking numbers directly from cachegrind files for reporting purposes and find ways to measure a production environment (either in PHP or the reported Time To First Byte data).

So I've run the same experiment (hello world post) but taking the times differently:

  • make sure you use a site in production (WP_DEBUG false and XDEBUG disabled)
  • load the "hello world" post
  • take the time to first byte from the network panel

Run the experiment 9 times and calculated the median for these results:

  • 25% improvement for themes with theme.json (from 51.98ms to 38.54ms, a total of -13.44ms)
  • 10% improvement for themes without theme.json (from 17.79ms to 16.12ms, a total of -1.67ms)

@azaozz commented on PR #3789:


10 months ago
#6

I've run the same experiment (hello world post) but taking the times differently

Yep, that seems quite better way to test, not just this change but anything that's expected to be more significant. One thing I'd like to suggest is to:

make sure you use a site in production (WP_DEBUG false and XDEBUG disabled), and it is running from /build, not /src. (Grunt watch can be used if making PHP changes).

Running from /build will make it as close to "production" as possible, i.e. minified js and css, etc.

@hellofromTonya commented on PR #3789:


10 months ago
#7

Now that wp_theme_has_theme_json() and wp_clean_theme_json_cache() landed in Core in https://core.trac.wordpress.org/ticket/56975 with the static cache variable approach, it unblocks the work here.

This code is a clean backport from the collective work done in Gutenberg, with the merge conflicts and code requests resolved ✅

@felixarntz you previously blocked the PR. Can you please re-review and share your thoughts on if this achieves the goals and is ready for commit?

#8 in reply to: ↑ 3 @hellofromTonya
10 months ago

Replying to spacedmonkey:

Isnt't this a depulicate of #56910?

They do touch the same function to improve the caching.

Hey @oandregal is this Trac ticket the same as the intent of #56910?

Also there are differences between PR 3712 and PR 3789 both of which are adding caching to the same function.

Can or should these be consolidated?

@hellofromTonya commented on PR #3789:


10 months ago
#9

Also noting there's another PR https://github.com/WordPress/wordpress-develop/pull/3712 opened in Trac 56910 which is doing something similar to the wp_get_global_settings() function.

#10 follow-up: @oandregal
10 months ago

Hey @oandregal is this Trac ticket the same as the intent of #56910?

I didn't know that existed, though it is focused on stylesheet and SVG filters? Either using this one or consolidating works for me.

Also there are differences between PR 3712 and PR 3789 both of which are adding caching to the same function.

3712 is for the wp_get_global_stylesheet and 3789 is for the wp_get_global_settings function, so they touch different functions. However, they both need to add back the theme_json group to the non-persistent groups. Whatever lands first, we can remove the changes from the other PR. Does that help?

#11 in reply to: ↑ 10 @hellofromTonya
10 months ago

Replying to oandregal:

I didn't know that existed, though it is focused on stylesheet and SVG filters? Either using this one or consolidating works for me.

3712 is for the wp_get_global_stylesheet and 3789 is for the wp_get_global_settings function, so they touch different functions. However, they both need to add back the theme_json group to the non-persistent groups. Whatever lands first, we can remove the changes from the other PR. Does that help?

Imagine me doing :facepalm: right now. I completely read it wrong. These 2 tickets and their PRs are for different functions and backports. Sorry for the confusion.

#12 @flixos90
10 months ago

PR https://github.com/WordPress/wordpress-develop/pull/3789 is almost good to go, so I just gave this another quick performance benchmark based on the median of 20 requests:

  • wp_get_global_settings() is called 77 times per page load, which we may separately want to improve.
  • Before this change, the total wp_get_global_settings() execution time was 12.5ms (total WP time 149.8ms).
  • With this change, the total wp_get_global_settings() execution time is 1.5ms (total WP time 137.8ms).

This is a massive win, not only for this function, but even for the entire WordPress page load this change reduces execution time by about 8%. \o/

@flixos90 commented on PR #3789:


10 months ago
#13

Cross-posting here: https://core.trac.wordpress.org/ticket/57502#comment:12

This change results in an ~8% improvement in _total_ WordPress server response time, which is amazing for a single change! 🎉

Excited to commit this once the last quirk above is addressed 😄

@oandregal commented on PR #3789:


10 months ago
#14

Also note my concern voiced in https://github.com/WordPress/wordpress-develop/pull/3712#pullrequestreview-1271487670 that may apply here too.

I tested following Tonya's instructions and no error happens. I verified that the load-styles.php is loaded and it has a 200 HTTP status code.

@oandregal commented on PR #3789:


10 months ago
#15

This change results in an ~8% improvement in total WordPress server response time, which is amazing for a single change! tada

@felixarntz In my tests, classic themes (TT1) improve by 10% and block themes (TT3) improve by 25% or 40%, depending on the test (see issue description & second test with TTFB). 8% is quite a bit lower than I expected! Could you share how did you measure?

@flixos90 commented on PR #3789:


10 months ago
#16

@oandregal Thanks for raising this. I am measuring Server-Timing of WordPress's own execution, which I typically do using the approach I outlined in this Gist: https://gist.github.com/felixarntz/de5c697a1a16c2b892634b70216eb6c7

So it is expected that those metrics are vastly different from TTFB. I would say both types of tests have their own justification, for different reasons:

  • Server-Timing metrics are far more stable between requests, so they are better to rely on to get an idea on performance impact, especially for _whether_ something is expected to bring a performance impact or not.
  • TTFB is a "closer to the real world" metric though, which is a benefit, but it also shows more variance, so harder to rely on it, particularly if the performance difference is very little. At the same time, _once_ we have developed an idea of the impact (e.g. via Server-Timing), I think TTFB is a much closer representation to how that difference will play out in the wild.

So I think those methodologies complement each other well.

#17 @flixos90
10 months ago

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

In 55155:

Editor: Use a non-persistent object cache in wp_get_global_settings().

This changeset is part of a greater effort to enhance the caching strategy for theme.json based data. Similar to [55138] and [55148], the cache is currently ignored when WP_DEBUG is on to avoid interrupting the theme developer's workflow.

This addition of a non-persistent cache results in a significant performance improvement for the overall load time of WordPress, with the Server-Timing load metric being ~8% faster and Time to First Byte being 25+% faster than before.

Props oandregal, spacedmonkey, hellofromtonya, flixos90, azaozz, aristath.
Fixes #57502.

@oandregal commented on PR #3789:


10 months ago
#19

Thanks everyone for helping to land all these PRs.

#20 @flixos90
9 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.