Opened 6 weeks ago
Last modified 7 days ago
#62126 reviewing enhancement
Cache duplicate calls to WP_Theme_JSON::sanitize
Reported by: | josephscott | Owned by: | joemcgill |
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch changes-requested |
Focuses: | performance | Cc: |
Description
A fresh out of the box WordPress site with the 2024 theme will make 53 calls to WP_Theme_JSON::sanitize()
during a single request to the default home page. We can see the progress of duplicate $input
calls with a simple addition to wp-includes/class-wp-theme-json.php
:
protected static function sanitize( $input, $valid_block_names, $valid_element_names, $valid_variations ) { static $inputs = array(); $input_key = md5( serialize( $input ) ); if ( ! isset( $inputs[ $input_key ] ) ) { $inputs[ $input_key ] = 1; } else { $inputs[ $input_key ]++; } error_log( print_r( $inputs, true ) ); $output = array();
The final log entry:
Array ( [c81ffa6dbea2454497cd6bd1bb5328a0] => 2 [c5ed022349e4a64a02171deb00a3d988] => 23 [cb150b99ce54ad170f5d795b65f6a651] => 2 [7aef88bb839b090be13dc01ab67a52e1] => 2 [a277ebc729ba3b9e28c492d103043a2e] => 21 [f8c99016466edd5a5377fdf68d6fa04d] => 1 [4ec129bfa3c2182a1037990a0aad6232] => 1 [6f5c486d28c1d96906e5237180daed22] => 1 )
Of the 53 calls, there are only 8 unique $input
values. All but 3 have duplicates. With that ratio it we'd likely benefit from caching the results and skipping the work on cache hits. This would be a classic trade off of doing less work by using a bit more memory.
I'm including a patch to cache processed $input
s with a simple static variable. This allows for multiple calls in a single page view to benefit from the reduced amount of work. The exact implementation is not my main concern as much as reducing the amount of time spent processing WP_Theme_JSON::sanitize()
calls.
Doing 1,000 sequential HTTP GET requests for the front page, here is the TTFB p75 before and after. While TTFB can vary because of any number of things taking longer, I'm including it to show even at that scale it can potentially move the needle. To show the difference in isolation for just this method call I'm including the exclusive processing time for a single home page view for WP_Theme_JSON::sanitize()
as measured by php-spx.
Before patch
- TTFB p75: 44.28ms
- exclusive time: 1.74ms
After patch
- TTFB p75: 43.601ms
- exclusive time: 0.779ms
Change
- TTFB p75: 0.679ms ( 1.5% )
- exclusive time: 1.061ms ( 61% )
Setup:
- WP 6.7-alpha-59092
- PHP 8.3.10
- Theme: Twenty Twenty-Four
- No active plugins
- Default home page with 2024 theme
Attachments (1)
Change History (12)
This ticket was mentioned in Slack in #core-performance by josephscott. View the logs.
5 weeks ago
This ticket was mentioned in PR #7529 on WordPress/wordpress-develop by @joemcgill.
5 weeks ago
#2
- Keywords has-patch added
#3
@
5 weeks ago
Thanks @josephscott. I've opened a PR from your diff in order to run CI against the proposed changes.
#4
@
5 weeks ago
It looks like this is causing several unit tests to fail: https://github.com/WordPress/wordpress-develop/actions/runs/11220806461/job/31189817758?pr=7529
One thing to keep in mind is that WP_Theme_JSON
data gets sanitized at different times during the application lifecycle so it's possible that code that modifies one of the origins (e.g., registered blocks) runs after the first time this method is called, which could cause these caches to become stale.
#5
@
5 weeks ago
Looks like I need to expand it to consider the other parameters as well.
If things can be altered after that fact then yeah, that is something that could be a problem as well.
#6
@
5 weeks ago
Caching different aspects of the WP Theme JSON structure has been tricky for this very reason. If you're interested in a deep dive, you may find the discussion in #57789 useful.
#9
@
5 weeks ago
I've reverted setting #62187 as a duplicate since both issues seek to improve the performance of WP_Theme_JSON::sanitize
in slightly different ways that are not mutually exclusive.
#10
@
5 weeks ago
- Keywords changes-requested added
- Milestone changed from Future Release to 6.8
- Owner set to joemcgill
- Status changed from new to reviewing
Going to set this, along with #62187 as issues to review for the 6.8 milestone. In the meantime, @josephscott do you want to investigate the PHPUnit test failures?
@mukesh27 commented on PR #7529:
7 days ago
#11
I also find it worth for some of the keys that are common in requested input when i profile the WP release.
This applies the diff supplied by @josephscott in https://core.trac.wordpress.org/attachment/ticket/62126/cache-wp-theme-json-sanitize-input.diff to run CI against the proposed changes.
Trac ticket: https://core.trac.wordpress.org/ticket/62126