Make WordPress Core

Opened 6 weeks ago

Last modified 7 days ago

#62126 reviewing enhancement

Cache duplicate calls to WP_Theme_JSON::sanitize

Reported by: josephscott's profile josephscott Owned by: joemcgill's profile 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 $inputs 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)

cache-wp-theme-json-sanitize-input.diff (751 bytes) - added by josephscott 6 weeks ago.

Download all attachments as: .zip

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

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

#3 @joemcgill
5 weeks ago

Thanks @josephscott. I've opened a PR from your diff in order to run CI against the proposed changes.

#4 @joemcgill
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 @josephscott
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 @joemcgill
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.

#7 @joemcgill
5 weeks ago

  • Milestone changed from Awaiting Review to Future Release

#8 @joemcgill
5 weeks ago

#62187 was marked as a duplicate.

#9 @joemcgill
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 @joemcgill
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.

Note: See TracTickets for help on using tickets.