#57502 closed enhancement (fixed)
Add object cache to wp_get_global_settings()
Reported by: | oandregal | Owned by: | 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.
22 months ago
#1
#2
@
22 months ago
- Keywords gutenberg-merge added
Using an experimental keyword gutenberg-merge
to identify Gutenberg backports Trac tickets.
@oandregal commented on PR #3789:
22 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:
22 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)
22 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:
22 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
@
22 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:
22 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:
↓ 11
@
22 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
@
22 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
@
22 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:
22 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:
22 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:
22 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:
22 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.
@flixos90 commented on PR #3789:
22 months ago
#18
Committed in https://core.trac.wordpress.org/changeset/55155 🎉
@oandregal commented on PR #3789:
22 months ago
#19
Thanks everyone for helping to land all these PRs.
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:
theme.json
(from 876.05ms to 520.80ms, a total of -355.25ms)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 withouttheme.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
add_theme_support( 'editor-color-palette', $palette )
work as expected.add_theme_support( 'disable-custom-colors' );
to thefunctions.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:
e27c5a38e371e1db85efeea239accf535227ed39
(svn 55005)theme.json
and TwentyTwenty for a theme with notheme.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:
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_XDEBUG=false
+LOCAL_PHP_XDEBUG=true
@@ -31,7 +31,7 @@ LOCAL_PHP_XDEBUG=false
-LOCAL_PHP_XDEBUG_MODE=develop,debug
+LOCAL_PHP_XDEBUG_MODE=profile
@@ -54,7 +54,7 @@ LOCAL_DB_TYPE=mysql
-LOCAL_WP_DEBUG=true
+LOCAL_WP_DEBUG=false
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:
+ - XDEBUG_CONFIG=profiler_output_name=cachegrind.out.%R
}}}
npm install && npm run build:dev
.npm run env:start && npm run env:install
.{{{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/}}}
35 0442 02 * 10 = 350 442 020ns
, which amounts to350ms
.