Replace the internal `WP_Theme_JSON_Resolver::theme_has_support()` with a public function

[oandregal]

The WP_Theme_JSON_Resolver::theme_has_support() method is used to check if a theme or its parent has a theme.json file. However, this class is an internal Core-only class.

The proposal is to add a global public function called wp_theme_has_theme_json() making it available to extenders for public consumption.

In addition, for performance, the results should be cached using a non-persistent cache. Why? To make the derived data from theme.json is always fresh from the potential modifications done via hooks that can use dynamic data (modify the stylesheet depending on some option, settings depending on user permissions, etc.).

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

• ​Gutenberg PR 45168 Add wp_theme_has_theme_json as a public API to know whether a theme has a theme.json.
• ​Gutenberg PR 45380 Deprecate WP_Theme_JSON_Resolver:theme_has_support().
• ​Gutenberg PR 46150 Make theme.json object caches non-persistent.
• ​Gutenberg PR 45979 Don't check if constants set by wp_initial_constants() are defined.
• ​Gutenberg PR 45950 Cleaner logic in wp_theme_has_theme_json.

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.

Trac ticket https://core.trac.wordpress.org/ticket/56975#ticket

This PR backports ​https://github.com/WordPress/gutenberg/pull/45168 and ​https://github.com/WordPress/gutenberg/pull/45380

## What

Adds a new public method wp_theme_has_theme_json to check whether a theme has a theme.json file.

## Why

Part of ​https://github.com/WordPress/gutenberg/issues/45171

This offers consumers a public sanctioned method they can use instead of relying on private code.

## How

Creates the new public method, substitutes usage, and deprecates the old one.

## Performance analysis

• Enabled xdebug in the env.
• Used TwentyTwentyThree theme in trunk at 54747 and this PR.
• As a logged-out user, load the "Hello World" post.
• Run the test 9 times and calculate the median.

This PR reduces 13.5ms the time to response, a 1.67% performance increase (from 809.81ms to 796.29ms).

Sharing the two fastest cachegrind files: ​this PR and ​trunk.

## Test

• Verify that automated tests pass.
• Using TwentyTwentyThree (a theme with a theme.json) and then a TwentyTwentyOne (a theme without theme.json) dfo the following:
  • Load the frontend and verify that styles load as expected.
  • Load the post editor and verify that the presets (e.g.: color) are properly set in the block inspector.

Demo wp_theme_has_theme_json function and cleaner dux.

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

[spacedmonkey]

I created a ​PR that improves the api and adds caching.

​https://github.com/WordPress/gutenberg/pull/45543 is updating a bit the code, I need to port those changes here once they land in Gutenberg.

I think we should look at and focus on my PR here - ​https://github.com/WordPress/wordpress-develop/pull/3604

I've ported the last changes done in Gutenberg to this PR, so it's up to date and ready to review. Marked the old conversations as done as they no longer reflected the state of this code (nor the conversations over at Gutenberg).

Because its relationship with ​this other PR (both use the theme_json cache group: they need to make it persistent and offer a way to clean the data), I've ported the same changes over here at ​https://github.com/WordPress/wordpress-develop/pull/3556/commits/073766ed9300eb5be7b752c1c23a3b411173aa35 Whatever PR lands first, some changes will not be longer need it. I'm documenting them as PR comments.

cc @felixarntz @spacedmonkey @hellofromtonya @azaozz @anton-vlasenko @costdev

@costdev Thanks for the suggestion to use a data provider in the tests, they're much clearer now! All your feedback was addressed.

Rebased from latest trunk to fix some conflicts. I guess this is ready for merging if a core committer finds the time :)

Blocking this commit until we resolve ​WordPress/gutenberg#45955.

This PR changes the cache invalidation.

One PR is a backport of a function introduced and validated in Gutenberg. The other is a potential improvement still in debate. Both rely on WP_Object_Cache, differing only in the details. I don't see how one should block the other.

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

This PR backports:

• ​https://github.com/WordPress/gutenberg/pull/45971
• ​https://github.com/WordPress/gutenberg/pull/45372
• ​https://github.com/WordPress/gutenberg/pull/46150

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 (450.66ms to 500.26ms, a total of -49.60ms)

## 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.
• 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.

• Data for TwentyTwenty:
• Data for TwentyTwentyThree:

### 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=develop,debug,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}

}}}

2. Build the environment: npm install && npm run build:dev.
3. Start docker: npm run env:start && npm run env:install.
4. 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/
5. 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/
}}}

6. 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.

7. Repeat 9 times and calculate the median.

cc @hellofromtonya @azaozz this is the other PR I mentioned I'd backport as early in the cycle as possible.

I'm confused why this backport PR includes caching changes as these seem out-of-scope for this initiative. Adding wp_theme_has_theme_json() and applying its changes are a specific scope of work for a single commit.

Does adding wp_theme_has_theme_json() not work without the caching changes? Or are these truly separate scopes of work?

I'm confused why this backport PR includes caching changes as these seem out-of-scope for this initiative. Adding wp_theme_has_theme_json() and applying its changes are a specific scope of work for a single commit.

Does adding wp_theme_has_theme_json() not work without the caching changes? Or are these truly separate scopes of work?

That's a great question. The more important thing for me is to have this method available. Not making performance worse is also something I tried to keep in mind, that's why I thought it should have _some_ cache, as the older one also had.

However! I also want to provide a more nuanced answer based on data. I ran some numbers and this is what I found (see ​raw data extracted from the cachegrind files linked below):

Approach | Cost in the full request (ms) | Cachegrind files
--- | --- | ---
Current status: WP_Theme_JSON_Resolver::has_theme_support | 0.05ms | ​cachegrind.out.hello-world.trunk.zip
This PR: wp_theme_has_theme_json with object cache | 2.7ms | ​cachegrind.out.hello-world.pr.zip
wp_theme_has_theme_json using static variables as cache | 0.05ms | ​cachegrind.out.hello-world.cachestatic.zip
wp_theme_has_theme_json with no cache (no object cache or static variables) | 1.13ms | ​cachegrind.out.hello-world.nocache.zip:

When looking at the numbers, you can say that using static variables is faster, and the wp_theme_has_theme_json without any cache performs faster than using the object cache :upside_down_face:

However, that's a partial picture. This is the other part of the picture:

• Not using any cache is vulnerable to grow conditions (the method can be used in more places, hence the performance impact will have a linear growth). Besides, the data I've shared only shows the performance _in my machine_ (some servers may have slower file access, different PHP versions, etc.).
• Using static variables in the method for caching comes with downsides in terms of API ergonomics: the cache leaks through the method parameters. In other words, you need to have this: wp_theme_has_theme_json( $cache_clear ). I actually started with this approach (​see thread).
• Using the object cache has the advantage that you can swap it for a faster implementation (plugins, etc.). Also, in the future, if/when we make the theme_json group persistent across requests, the cost is minimized (~1ms less at a minimum given if would not have to recalculate support).

All things considered, even the fact that we're talking 1ms here while there are PRs such as ​https://github.com/WordPress/wordpress-develop/pull/3789 that reduce +350ms, I think the current approach is the more future-proof.

With what I've learned, I wouldn't mind removing the cache altogether if that is preferable for core folks and provided it helps to ship this faster: it is no big impact either way. I wouldn't recommend going back to caching using static variables in the method, as it compromises the API.

[hellofromTonya]

Pulling into 6.2 cycle and self-assigning for commit review.

Everyone reviewing this PR should consider this alternative ​https://github.com/WordPress/wordpress-develop/pull/3604.

This has number of key benefits.

• You can see if ANY theme support theme json.
• Reuses the theme cache group.
• Cleaner cache invalidation.

Which Gutenberg PR(s) is/are covered by this?

Which Gutenberg PR(s) is/are covered by this?

Thank you everyone. I'm preparing the commit.

[hellofromTonya]

In 55086:

Themes: Introduce wp_theme_has_theme_json() for public consumption.

Adds wp_theme_has_theme_json() for public consumption, to replace the private internal Core-only WP_Theme_JSON_Resolver::theme_has_support() method. This new global function checks if a theme or its parent has a theme.json file.

For performance, results are cached as an integer 1 or 0 in the 'theme_json' group with 'wp_theme_has_theme_json' key. This is a non-persistent cache. Why? To make the derived data from theme.json is always fresh from the potential modifications done via hooks that can use dynamic data (modify the stylesheet depending on some option, settings depending on user permissions, etc.).

Also adds a new public function wp_clean_theme_json_cache() to clear the cache on 'switch_theme' and start_previewing_theme'.

References:

• ​Gutenberg PR 45168 Add wp_theme_has_theme_json as a public API to know whether a theme has a theme.json.
• ​Gutenberg PR 45380 Deprecate WP_Theme_JSON_Resolver:theme_has_support().
• ​Gutenberg PR 46150 Make theme.json object caches non-persistent.
• ​Gutenberg PR 45979 Don't check if constants set by wp_initial_constants() are defined.
• ​Gutenberg PR 45950 Cleaner logic in wp_theme_has_theme_json.

Follow-up to [54493], [53282], [52744], [52049], [50959].

Props oandregal, afragen, alexstine, aristath, azaozz, costdev, flixos90, hellofromTonya, mamaduka, mcsf, ocean90, spacedmonkey.
Fixes #56975.

[hellofromTonya]

Thank you everyone for your contributions!

Switching this ticket to Themes component.

Committed via changeset https://core.trac.wordpress.org/changeset/55086.

Which Gutenberg PR(s) is/are covered by this?

@azaozz the PRs are listed in the description in the "This PR backports" section.

[hellofromTonya]

Reopening for wp_get_global_settings() work in ​PR 3789.

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

[spacedmonkey]

Created a PR for an api to allow core to check for ANY theme having a theme json file.

​https://github.com/WordPress/wordpress-develop/pull/3860

[spacedmonkey]

Replying to hellofromTonya:

Reopening for wp_get_global_settings() work in ​PR 3789.

@hellofromTonya Will #56910 be handle as part of this ticket we all?

[hellofromTonya]

Replying to spacedmonkey:

Replying to hellofromTonya:

Reopening for wp_get_global_settings() work in ​PR 3789.

@hellofromTonya Will #56910 be handle as part of this ticket we all?

No, that's a separate Trac ticket.

Updated:

​PR 3789 is a backport from Gutenberg. Is #56910 then iterating on top of that? Or is something different?

I'm asking because may want to point PR 3789 to that ticket too.

[hellofromTonya]

Ah wait, duh on my part. 2 different scopes of work. Aha please disregard my questions @spacedmonkey. I think the work in #56910 is separate.

Likely the scope of this Trac ticket is too big. The goal is 1 Trac ticket per backport for better commit and ticket history. So thinking this Trac ticket can be reclosed with the description updated to make it specific to the previous commit. Then move PR 3789 to its own Trac ticket.

[hellofromTonya]

Revising to focus this Trac ticket on wp_theme_has_theme_json() rather than a larger body of work. Reclosing as the work is committed.

[Otto42]

Fatal error: Uncaught Error: Call to undefined function wp_cache_get() in /wp-includes/global-styles-and-settings.php on line 285

This breaks the call to load-styles.php, and broke the admin screens of wordpress.org.

[costdev]

wp_cache_get() is defined either in wp-includes/cache.php, or in an external object cache. wp_start_object_cache() determines which of these should be loaded.

This is called in two places: wp-settings.php, and wp-includes/ms-settings.php.

For wp-includes/global-styles-and-settings.php, this is loaded in:

• wp-settings.php which, as mentioned above, already calls wp_start_object_cache(), so this is safe.
• wp-admin/load-styles.php, which doesn't load wp-includes/load.php and call wp_start_object_cache().

@Otto42 has confirmed to me that wp-admin/load-styles.php was in the stacktrace, so it seems that we need to load wp-includes/load.php and call wp_start_object_cache() in wp-admin/load-styles.php.

Thoughts?

[Otto42]

@costdev I doubt it's going to be quite that simple.

wp_start_object_cache uses the filter functions to determine whether to load an external cache. But those functions are noop'd here, by the wp-admin/includes/noop.php. So it won't load the cache, if the cache is external.

The purpose of load-styles and load-scripts is to get the styles and scripts output without loading the overhead of loading all of WordPress. To get the cache functions working properly, you have to load a hell of a lot of WordPress.

So the question is, is this overhead worth it? Or the alternative is to simply remove the use of caching functions from this change.

[costdev]

Thanks for the extra info @Otto42!

[Otto42]

Alternatively, alternatively, wrap the cache calls in function_exists calls. Then, for the purposes of this, it can be simply uncached.

I'm not seeing a lot of benefits to the caching here anyway, since really it's just checking for if a couple files exist.

[hellofromTonya]

The caching is important part of the overall performance strategy work that's being backported function by function from ​https://github.com/WordPress/gutenberg/issues/45171. There are other functions that will receive the same code. So it's important to get it right in this one first before it's backported to the others.

As it's currently impacting sites running on trunk, let's revert the caching part of [55086] for now. This will give @flixos90 and others time to discuss, test, and make adjustments.

The caching wp_cache_* functions are not available when wp-admin/load-styles.php is loaded. The caching introduced in r55086 caused fatal errors and broken styles.

This PR removes all of the caching code to restore functionality for sites and local development using trunk.

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

[hellofromTonya]

​PR 3864 is a revert of the caching portion of [55086], ie the parts that use wp_cache_*() functions.

With a clean install of trunk, I was not able to reproduce the problem in Chrome, Edge, or Firefox. The admin area renders properly for me before and after PR 3864.

@dmsnell reached out to me in Slack noting:

I don't get any enqueued CSS, so everything inside of wp-admin renders broken and I'm not seeing any HTTP requests for any CSS files

Dennis tested PR 3864 for me and confirmed that it does resolve the issues.

Marking for commit.

[hellofromTonya]

In 55092:

Themes: Revert caching from r55086.

Calls to wp-admin/load-styles.php do not include the loading of wp_cache_*() functions. With [55086], this caused a fatal error:

Fatal error: Uncaught Error: Call to undefined function wp_cache_get() in /wp-includes/global-styles-and-settings.php on line 285

In some production and local environments running trunk, the admin area looked broken as the styling was not loaded as there were no HTTP requests.

This commit reverts the caching from [55086] to restore sites running trunk until a solution is found.

Follow-up to [55086].

Props Otto42, dmsnell, costdev.
See #56975.

[hellofromTonya]

The caching portion has been reverted. Updating the keywords.

Committed via https://core.trac.wordpress.org/changeset/55092.

[hellofromTonya]

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

[dmsnell]

To share more about the testing I provided, in case it helps someone figure out what broke, I was running PHP 8.2.0, running WordPress from its directory with php -S 0.0.0.0, on a fresh site install, on a fresh database, with the trunk Gutenberg symlinked as a plugin.

Before the patch, with WP_DEBUG set (also with it unset too) I get no CSS requests from wp-admin pages (except wp-login) and no errors on the backend. as a fresh install the theme was twenty-twenty three, but changing the theme didn't affect anything. page renders on the site frontend were fine and enqueued and requested the appropriate CSS.

After the patch, everything works again.

Found this commit via git bisect.

I'm curious how this affects wp-admin CSS, maybe a bit unnerved that theme-JSON related code can have this effect, but maybe I'm overlooking something on the caching? or some error is being eaten up by a loose try/catch somewhere? I initially assumed some hook/filter chain was broken which stoped enqueue_script, but I didn't look far.

Thanks everyone for fixing this!

[Otto42]

I'm curious how this affects wp-admin CSS, maybe a bit unnerved that theme-JSON related code can have this effect, but maybe I'm overlooking something on the caching?

This has nothing to do with theme related JSON or anything else. This is straight PHP code that was not working properly, because it was not including required files.

[azaozz]

Sorry I'm late to the party.

Fatal error: Uncaught Error: Call to undefined function wp_cache_get() in /wp-includes/global-styles-and-settings.php on line 285

wp_cache_get() is defined either in wp-includes/cache.php, or in an external object cache. wp_start_object_cache() determines which of these should be loaded.

This is called in two places: wp-settings.php, and wp-includes/ms-settings.php.

@Otto42 @costdev @hellofromTonya The problem is with how load-styles.php and load-scripts.php work. They do not load WP, only load script-loader.php and use it as a whitelist to get the URLs to the scripts and stylesheets. The dependencies and load order have already been "calculated" beforehand. load-styles.php and load-scripts.php only concatenate and output (stream) the actual js and css files whose names are passed in $_GET .

Because WP is not loaded, any functions that are called from script-loader are stubbed. None of these functions is needed, only the URLs to scripts and stylesheets. The stubs are in https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/noop.php.

Looking at [55086], seems wp_theme_has_theme_json() needs to be stubbed too, and the revert in [55092] added again?

This is a new attempt for #3556, which after commit resulted in a fatal error due to load-styles.php only loading a very small subset of WordPress, thus not having the wp_cache_*() functions available.

This PR addresses the problem by introducing a concrete check for whether to use the cache (the previous code was doing a cache lookup before even checking whether to use the cache (based on ! WP_DEBUG), and by including a new condition ! is_admin() in that check. This way the load-styles.php request succeeds.

To test this fix, set the CONCATENATE_SCRIPTS constant to true, and load WP Admin, which will then load load-styles.php as a CSS file. Without the ! is_admin() condition, you get the fatal error, and therefore styles are not loading. With the condition, it is working as expected.

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

[flixos90]

@azaozz That's a great pointer, thanks! I took a closer look at that code. Unfortunately, your suggestion of stubbing wp_theme_has_theme_json() would not work since the global-styles-and-settings.php file that includes it is loaded directly from load-styles.php. So we could only stub that function if it was wrapped in a function_exists() check, which seems a bit fragile to me.

What we could stub is all the wp_cache_*() functions, but that would be a lot, so I'm not sure we want to do that.

For now, I went with the approach of not using the cache if ! is_admin(), which fixes the problem in a simpler way. I implemented that solution in ​https://github.com/WordPress/wordpress-develop/pull/3867.

If you prefer to stub the wp_cache_*() functions, that would make it a bit more resilient (no need to remember to check for is_admin() in other similar functions in the future), but I'm also unsure about that because of potential side effects. So to me the ! is_admin() check seems the safest solution.

[hellofromTonya]

Summarizing solution options

Option 1: Adding a is_admin() check as a wrapper

Implemented in ​PR 3867

• Pros:
  • Simple approach in this specific use case as it's also checking ! WP_DEBUG.
• Cons:
  • Adds a function call.
  • Adds another conditional.
  • Could cause confusion later for why is_admin() is being used.

Option 2: Stubbing the functions in wp-admin/includes/noop.php

Implemented in ​PR 3872

function wp_cache_get() {
        return false;
}
function wp_cache_set() {
        return false;
}

• Pros:
  • More resilient.
    • Less code.
    • Less chance of breaking sites when forgetting to add ! is_admin() wrapper.
  • Centralized solution.
• Cons:
  • Unknown side effects?

Are there instances of function_exists( 'wp_cache_get' ) or function_exists( 'wp_cache_set' )?

• Core: No
• Gutenberg: No
• Plugins in the wp.org plugin repo? Yes ​https://wpdirectory.net/search/01GQ539955T9K8X8RZ0XY3YDXH. Only Ezoic plugin might be impacted. The others shouldn't be due to how their code is using it.
• Themes in the wp.org theme repo? No ​https://wpdirectory.net/search/01GQ53PX6X59ZP5M34VR33W4K5

Mitigation:
Outreach can be done to the impacted plugin author. A dev note published to alert developers.

Other potential unknown side effects?
These could be discovered throughout the 6.2 cycle through reports from users and developers.

Pausing commit for the option of stubbing in noop.php. Waiting for consensus.

Option 2: Stubbing the functions in wp-admin/includes/noop.php:

As detailed in the trac ticket, this PR implements option 2 to prevent the fatal error due to load-styles.php only loading a very small subset of WordPress, thus not having the wp_cache_*() functions available.

This PR:

• Stubs wp_cache_get() and wp_cache_set() returning their failure state.
• Restores the reverted caching code from PR #3556 (reverted in r55092.

How to test:

• Add the following to wp-config.php:

{{{php
define( 'CONCATENATE_SCRIPTS', true );
define( 'WP_DEBUG', false );
define( 'SCRIPT_DEBUG', false );
}}}

• Load WP Admin, which will then load load-styles.php as a CSS file.

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

[hellofromTonya]

There are 2 different solutions (detailed in comment 51 above. Each has a PR to implement.

Which solution should be implemented?

I think Option 2 (stubbing in noop) is the better option for the long term.

@azaozz @flixos90 Which option do you prefer?

[Otto42]

Call me crazy, but.. what does this entire change actually do?

Because as near I can see, the only purpose for the wp_theme_has_theme_json function, is to add the use of caching around a couple of file_exists calls. And that's fine, fair enough.

But then you go to the trouble of ensuring that the cache is non-persistent and cleared a fair amount. And now we're talking about stubbing out the cache functions, ensuring that this won't be cached, on every wp-admin screen that uses load-styles (which is all of them).

So, one has to wonder, why all the effort to do the caching in the first place? What am I missing?

[azaozz]

Replying to Otto42:

And now we're talking about stubbing out the cache functions...

Stubbing is how script-loader is used (for better or worse) in load-styles.php and load-scripts.php. I.e. only the list of file URLs is used, everything else is ignored. This is unrelated to how caching may work, or any other functionality there.

[Otto42]

Replying to azaozz:

Stubbing is how script-loader is used (for better or worse) in load-styles.php and load-scripts.php. I.e. only the list of file URLs is used, everything else is ignored.

Hey, I'm with you on that part. But it seems like we should stub out this function entirely, since it's not needed to create a list of file URLs. Frankly, it doesn't seem like this function belongs in this file.

Something fundamentally is wrong here, and it needs to be addressed before any further changes happen.

[azaozz]

Replying to flixos90:

For now, I went with the approach of not using the cache if ! is_admin(), which fixes the problem in a simpler way.

Yep, it fixes it but using is_admin() may seem a bit like going around it?

If you prefer to stub the wp_cache_*() functions, that would make it a bit more resilient...

Think so too. In addition it would make it the same as the existing way of handling such cases, and perhaps a bit more readable? So perhaps ​https://github.com/WordPress/wordpress-develop/pull/3872 is the better approach.

[azaozz]

Replying to Otto42:

But it seems like we should stub out this function entirely, since it's not needed to create a list of file URLs. Frankly, it doesn't seem like this function belongs in this file.

Yea, it doesn't seem to belong in script-loader...

Something fundamentally is wrong here, and it needs to be addressed before any further changes happen.

Seems there were few recent additions to script-loader that depart from it's main purpose. As far as I see maybe a script-loader-functions.php file is needed for all the additional things there.

On the the other hand it seems it's time to retire load-scripts.php and load-styles.php? load-scripts.php has been broken for quite a while now, only works for some of the old scripts. Also when HTTP/2 is used, it seems better to have separate tags (better network and browser caching), not to have different concatenated subsets of the css files.

[Otto42]

Replying to azaozz:

On the the other hand it seems it's time to retire load-scripts.php and load-styles.php?

You're not wrong there. Probably a good addition for 6.3.

+1 for stubbing, LGTM.

Pausing commit for the option of stubbing in noop.php.

Imho stubbing is the better option. Same as existing code, and perhaps a bit more readable/easier to understand.

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.

[flixos90]

I am also leaning towards the noop option, however given @Otto42's feedback I wanted to see what the real performance impact of this caching is. So I did a quick test, making 20 requests and recording median performance metrics.

Results are in this spreadsheet: ​https://docs.google.com/spreadsheets/d/1lTw0d4iyNJ91Jg0Ru3RXCMvSASRaV5vFIsWIkgOjBTU/edit

TLDR: The function itself is indeed relevant for performance. It is a single function in WordPress core that, because it is called so often (137 times), makes for more than 1% of overall WordPress load time. 1% seems little, but for a single simple-looking function, it is quite a lot.

That said, it looks like caching did not help with performance at all based on this data. It seems the overhead of cache lookup is somewhat comparable to the filesystem checks. And this is even without object-cache.php, so no external connection that would potentially be a problem. So for real performance impact, caching does not appear to help.

Please review. My current advise based on this would be to discard this effort entirely. What I think would make more sense is to try to reduce the excessive number of calls to wp_theme_has_theme_json() or find other ways to simplify.

I created a Gist with how I got the data, may be useful for other similar quick performance tests too: ​https://gist.github.com/felixarntz/de5c697a1a16c2b892634b70216eb6c7

Curious what you think @hellofromTonya @azaozz @spacedmonkey @oandregal.

@hellofromtonya This option makes sense to me, however I have an overarching concern now, see https://core.trac.wordpress.org/ticket/56975#comment:65.

[oandregal]

@flixos90 those numbers are in line with what I shared in the issue a month ago ​https://github.com/WordPress/wordpress-develop/pull/3556#issuecomment-1361689398

With what I've learned, I wouldn't mind removing the cache altogether if that is preferable for core folks and provided it helps to ship this faster: it is no big impact either way.

[flixos90]

@oandregal How could I miss that? Apologies.

FWIW my results are still a tiny bit different, as for me it looks like all results are pretty much the same. But that is enough to show that using the WP cache functions does not help here.

I will try getting numbers for the static variable cache on my analysis as well, since that looks quite promising in yours.

[oandregal]

I will try getting numbers for the static variable cache on my analysis as well, since that looks quite promising in yours.

Yeah, though, I wouldn't recommend going back to that point. The fact that it leaks into the API (parameter in the function to clean the cache) is not great. The performance improvement is negligible compared to other much bigger impact we can have with changes in other areas (upcoming PRs, etc.).

[Otto42]

Replying to oandregal:

I will try getting numbers for the static variable cache on my analysis as well, since that looks quite promising in yours.

Yeah, though, I wouldn't recommend going back to that point. The fact that it leaks into the API (parameter in the function to clean the cache) is not great. The performance improvement is negligible compared to other much bigger impact we can have with changes in other areas (upcoming PRs, etc.).

Can you please explain what "leaks into the API" means?

Also, for static variable, you wouldn't need any function to clean the cache. In fact, you seem to have gone to so much effort to make it a non-persistent cache that you shouldn't need to clean it at all in the first place.

And the use of a static variable for such a thing is quite likely to be much, much faster than using the wp_cache functions.

[spacedmonkey]

My recommendation is the following.

• Removing caching from wp_theme_has_theme_json function.
• Limit the usage of wp_theme_has_theme_json where possible, possibly just to bootstrap functions.
• Implement a has_theme_json method on WP_Theme class and use existing caching built ​into the theme class. This class already has a way of ​opt-in into make the cache persistent. Cache invalidation is ​already in place.
• has_theme_json in places where have a WP_Theme instance in use already. ​Example 1, ​example 2.

Use get_theme_file_path function to stop replicated code.

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

[flixos90]

@oandregal I agree with @Otto42 here. It is a non-persistent cache, no need to clear it. I did a quick follow up test for a static variable cache and in that, the total wp_theme_has_theme_json() time was cut in half. I would need to look a bit more to get a better assessment, but it is safe to say that the static variable cache does bring a notable improvement.

I would suggest to go with that. Also a lot simpler as it does not involve the cache clearing code. And given the WP cache API usage doesn't bring a benefit here, at least for this function we don't even need to consider using a persistent cache later - no notable benefits. We can create a new PR where we introduce a static variable, and keep the WP cache API (including the non-persistent cache group) out of it. For the other related tickets where we have been considering using the API, we need to separately consider and measure whether it's worth it.

@spacedmonkey I agree with you mostly, however I don't think we should use the existing caching from WP_Theme in WP_Theme::has_theme_json(). Doing so would bring just as little benefit as using the WP cache API does here, since it is effectively similar logic. This is a good lesson for us that using WP cache API is not always solution (though it often is).

[spacedmonkey]

Replying to flixos90:

Doing so would bring just as little benefit as using the WP cache API does here, since it is effectively similar logic.

It is different as the cache group is persistent, if opted in. Which is different and would ( I believe ) add benefit.

Related #57114

[oandregal]

@oandregal I agree with @Otto42 here. It is a non-persistent cache, no need to clear it

Isn't it the goal to make this cache and the other ones persistent? ​https://github.com/WordPress/wordpress-develop/pull/3712#issuecomment-1354459826 Why go to the trouble of adding something we plan to remove if it is not an issue?

I can't help but feeling that we're running in circles. When I first created this PR, I was asked to implement it using object cache in a persistent way, which I did. At which point, I was asked to make it non-persistent, which I also did. And now we removed it altogether. We're back to what we were 3 months ago, but with the feature freeze for 6.2 a lot closer.

At this point, I'd feel more comfortable with the following next steps:

• Do not add any kind of cache to the wp_theme_has_theme_json method for now. We can revisit after 6.2 feature freeze if it proves to be a real issue in terms of performance (it's a bugfix we can ship).

• Focus our time in shipping the two PRs we've been working on that fix long-standing issues ​https://github.com/WordPress/wordpress-develop/pull/3712 and add big performance improvements ​https://github.com/WordPress/wordpress-develop/pull/3789 These cannot land after feature freeze.

Would you think that would a viable way forward?

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

This PR implements caching to wp_theme_has_theme_json using a static variable.

[oandregal]

We can create a new PR where we introduce a static variable, and keep the WP cache API (including the non-persistent cache group) out of it. For the other related tickets where we have been considering using the API, we need to separately consider and measure whether it's worth it.

I found myself in a layover with a half an hour to kill and re-checked the conversations in this ticket. Prepared ​https://github.com/WordPress/wordpress-develop/pull/3887 I won't be fully available until Wednesday, so feel free to address any feedback and merge. I didn't remove the existing non-persistent group or the function wp_clean_theme_json_cache, as it's going to be necessary for the wp_get_global_settings anyway. If it is not, it can be removed in a separate PR as well.

My advice would still be not using the static cache and follow the approach at https://core.trac.wordpress.org/ticket/56975?cnum_edit=77#comment:75 However, it's more important to me to wrap up this conversation and focus on shipping the other two tickets. If most folks think this is the best way forward, let's ship and move forward.

@otto42 Note how using the static cache leaks into the API via parameter to clean it upon certain events:

<?php
wp_theme_has_theme_json( $can_use_cache = true ) {

[Otto42]

Replying to oandregal:

@otto42 Note how using the static cache leaks into the API via parameter to clean it upon certain events:

<?php
wp_theme_has_theme_json( $can_use_cache = true ) {

A static variable doesn't persist between runs. You don't need an extra parameter to clear it.

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)

[oandregal]

A static variable doesn't persist between runs. You don't need an extra parameter to clear it.

What if the variable is initialized before a "change of theme" such as the switch_theme or start_previewing_theme hook? An easy way to see how it fails is by removing the last commit or the parameter in the PR and observing the dozens of PHP unit tests failures.

[Otto42]

Replying to oandregal:

A static variable doesn't persist between runs. You don't need an extra parameter to clear it.

What if the variable is initialized before a "change of theme" such as the switch_theme or start_previewing_theme hook? An easy way to see how it fails is by removing the last commit or the parameter in the PR and observing the dozens of PHP unit tests failures.

I would say to change the run ordering so that it happens after those theme hooks. Why would it run before those hooks if you're changing the theme? Do you need to know of the theme.json files existence on a theme you're switching away from?

[oandregal]

I would say to change the run ordering so that it happens after those theme hooks. Why would it run before those hooks if you're changing the theme? Do you need to know of the theme.json files existence on a theme you're switching away from?

From memory, a few use cases were the customizer and the theme directory. These were some that prompted people to add hooks to clear the cache (there were none at the beginning), so I'm not sure how feasible is that suggestion. I also don't feel comfortable having a cached function that requires people to know when they can use it and when they cannot. I'd rather have a reliable one that works in all scenarios.

[flixos90]

@oandregal I understand the theoretical concern you're referring to, and it may indeed be a problem. But I think @Otto42 is right in that if this function was ever called in a situation where the theme is being switched, before it is switched, this would be a problem in the consuming code.

Maybe this is happening somewhere in WordPress - maybe it is not. We don't know for sure, and as long as we don't know for sure I don't think it's worth optimizing for this "bug".

If it is indeed a problem somewhere, we could easily fix it by making the static variable a map of $theme_dir => $has_theme_json, always relying on get_stylesheet() to make sure we act on the current theme. This should be straightforward, but I still think it is overkill in the current situation where we are just considering this as a theoretical concern. We still have all the betas ahead of us, so I think it's worth committing the simple solution, and if anything comes up we can see how to best approach a fix for that.

My concluded suggestion here would be to proceed with the basic static variable cache and commit that. cc @hellofromTonya

[flixos90]

@oandregal Replying to your concern from further up:

Isn't it the goal to make this cache and the other ones persistent? [...] Why go to the trouble of adding something we plan to remove if it is not an issue?

Yes it potentially is, though we will have to measure how that impacts performance to decide whether it is worth it. At this point, it is safe to say that a static variable cache is faster than a non-persistent cache using WP cache API, so we should go with that IMO, since we have no timeline on making the cache persistent anyway - I doubt we realistically can get that into 6.2, and we shouldn't ship with an approach that we could easily achieve and we know is worse performance-wise than another one.

How it is for a persistent cache using WP cache API, we will have to measure. I would expect it to be faster, since the actual logic would not need to run at all in most requests. But it could be that the actual filesystem check here is so cheap that it the persistent cache overhead is worse than the logic itself - let's see. All of this is assumptions until we measure the performance impact.

[flixos90]

Replying to spacedmonkey:

Replying to flixos90:

Doing so would bring just as little benefit as using the WP cache API does here, since it is effectively similar logic.

It is different as the cache group is persistent, if opted in. Which is different and would ( I believe ) add benefit.

Right, it may be different since the cache there is persistent. But we need to keep in mind, this only matters for sites that use a persistent object cache. Tons of sites do not, and for those, the behavior is the same as a non-persistent cache.

We need to measure performance of this approach to make a decision, not base it on assumptions.

[hellofromTonya]

Replying to flixos90:

At this point, it is safe to say that a static variable cache is faster than a non-persistent cache using WP cache API, so we should go with that IMO, since we have no timeline on making the cache persistent anyway

I agree. It's also less code and complexity.

Replying to flixos90:

Maybe this is happening somewhere in WordPress - maybe it is not. We don't know for sure, and as long as we don't know for sure I don't think it's worth optimizing for this "bug".

I agree with the lean approach.

The "use cache" param is handy for resetting the static variable cache between automated tests (ie one of the ways to accomplish it).

Replying to flixos90:

My concluded suggestion here would be to proceed with the basic static variable cache and commit that.

Sounds good. Thanks Felix. I'll take look at the PR for commit consideration.

Per discussions in the Trac ticket, ​https://github.com/WordPress/wordpress-develop/pull/3887/commits/f8d09bcbc8a030e4050e4517586bacc8e6b0ef0b removes the cache reset and $can_use_cache logic in wp_theme_has_theme_json(). The removal of the function param is also for BC considerations should this param not be needed.

However, a reset of some sort is still needed between automated tests. Else, tests will fail (as seen by the commit ❌ ).

+1 to using WP_RUN_CORE_TESTS to invalidate/reset the cache when running tests. That's the primary use for this constant, right?

Regarding use of WP_DEBUG to invalidate the cache while developing. Like @felixarntz thinking this is okay for now and should be replaced once core has a (well established) way of setting development mode. There's a trac ticket for that: https://core.trac.wordpress.org/ticket/57487.

The only case that seems a bit unclear when using a static is whether invalidating the cache may be needed while PHP is running. Reading through the newer comments on https://core.trac.wordpress.org/ticket/56975, seems it will not be?

[spacedmonkey]

It seems to me, that is there is a valid use case that the cache needs to be invalidated. A couple of cases off the top of my head.

• Unit test runs.
• A plugin that switches theme depending on request, like mobile theme if the request is coming a phone.
• Customizor previewing new theme.

Just guessing that theme is the same is the same request might be a problem.

It is worth noting, there is a performance hit of requesting if a file existing and is readable 170+ times per page.

Maybe instead of a static variable or object cache, maybe a global. Something I explored ​here.

[Otto42]

Replying to spacedmonkey:

It seems to me, that is there is a valid use case that the cache needs to be invalidated.

As @flixos90 mentioned, all those cases are easily solved with a slightly modified static variable idea, where the theme name is used a key. Easy to write, cheap to implement.

Maybe instead of a static variable or object cache, maybe a global. Something I explored here.

There's no real case where using a global makes sense, because if the goal is to centralize it in one place, then having a global just allows you to split that up and decentralize it. Anytime you check the global value instead of calling the function to get the value, you're decentralizing the function.

It is worth noting, there is a performance hit of requesting if a file existing and is readable 170+ times per page.

I have to question, why are we doing this that many times per page? It seems like this is a case of optimization needed. I can see maybe having it happen like 10 times, but 100 or more is insane. Is it used in some kind of loop structure somewhere that is unnecessary? Maybe this whole argument could be easily solved by simply eliminating the actual problem.

[hellofromTonya]

Replying to spacedmonkey:

It seems to me, that is there is a valid use case that the cache needs to be invalidated. A couple of cases off the top of my head.

• Unit test runs.
• A plugin that switches theme depending on request, like mobile theme if the request is coming a phone.
• Customizor previewing new theme.

For the unit test runs, the ​PR 3887 ignores the cache between tests.

For theme switching and previewing, @flixos90 addressed theme switching and previewing as an unknown.

I understand the theoretical concern you're referring to, and it may indeed be a problem. But I think @Otto42 is right in that if this function was ever called in a situation where the theme is being switched, before it is switched, this would be a problem in the consuming code.

Maybe this is happening somewhere in WordPress - maybe it is not. We don't know for sure, and as long as we don't know for sure I don't think it's worth optimizing for this "bug".

Rather than optimizing the function now for the unknown, the thought is to commit a lean approach, test and validate if it truly is a bug or not. If it is a bug that needs addressing, then follow-up with a bugfix.

Do you agree with this lean static variable approach @spacedmonkey?

[spacedmonkey]

Do you agree with this lean static variable approach @spacedmonkey?

I am not 100% sure about static variable. I don't like the DUX of cache clearing with a param on a function. That feels ugly.

I think maybe we leave it as is and maybe create a new ticket to discuss caching in my detail. There are lots of options.

• Transients.
• Object cache.
• Static variables
• Global variables

All of which have their pros and cons. I think the end goal here is end up in a preseient cache across requests, but this has never been defined as the goal.

I could get behind any solution that makes the internals changable later. We need a wp_theme_has_theme_json function and a wp_theme_has_theme_json_cache_clear function ( named however you want). If developers call, wp_theme_has_theme_json_cache_clear if clears that cache. That way we can change it however we want later. Any solution where we forcing ourselves to use a param, is just making bad DUX IMO.

[hellofromTonya]

Replying to spacedmonkey:

I am not 100% sure about static variable. I don't like the DUX of cache clearing with a param on a function. That feels ugly.

The $can_use_cache is removed, i.e. due to BC concerns and unknowns if it's truly needed

wp_theme_has_theme_json( $can_use_cache = true ) {

I could get behind any solution that makes the internals changable later. We need a wp_theme_has_theme_json function and a wp_theme_has_theme_json_cache_clear function ( named however you want). If developers call, wp_theme_has_theme_json_cache_clear if clears that cache. That way we can change it however we want later. Any solution where we forcing ourselves to use a param, is just making bad DUX IMO.

@spacedmonkey I think ​PR 3887 fits your request of what is known today and what can be done in time for WP 6.2.

• There's no reset param.
• Cache is ignored when automated tests are running.
• The internals are changeable without BC concerns.

All of which have their pros and cons. I think the end goal here is end up in a preseient cache across requests, but this has never been defined as the goal.

If persistent caching is the goal, that will likely need to be in future, maybe 6.3. I doubt there's time to revamp all of the caching strategies being backported and get them committed in time for 6.2.

## 2021 theme.

### Before

### After

This saves 40+ calls to is_readable which saves 48ms per page request. This is worth doing.

Awesome! Thanks @spacedmonkey for running this before and after analysis ​https://github.com/WordPress/wordpress-develop/pull/3887#issuecomment-1402055695 👏

[dmsnell]

This saves 40+ calls to is_readable which saves 48ms per page request. This is worth doing.

Is this 48ms of a profiler run? Or is that 48ms of normal production runtime? I'm curious if we were to flip the tests and instead of measuring ms we measured req/s using something like httperf or siege. 48ms for stat calls to determine if a file exists seems off, even if it's called 170 times. I think these tools might give more reliable readings than checking TTFB a few times and averaging, since they will hit a page thousands of times.

A cursory search noted that repeated calls to stat should return data directly from memory and shouldn't require disk I/O after the first call, so it's possible that the measured impact of this in an artificial environment (such as a profiler) will appear much worse than it is in reality (not to mention what @oandregal pointed out in that the profiler substantially inflates reported runtimes across the board).

Some ideas for getting out of the circle:

Has anyone complained about this code slowing down their sites? If we don't know for sure that performance is a problem, we can follow what @Otto42 suggested and ship this without a cache. Wait until we know it's a problem to fix it.

Ship without a cache so we can better gather data. A follow-up patch to add caching to an existing working system is a smaller problem than shipping a cache in advance. No caching comes without its own cost and more often than not, the costs of caching without a clear understanding of the implications of the cache result in defects, which are generally harsher than performance hits. We can take a patch adding a cache and apply it to existing sites with all their plugins and data and see in realistic settings what the impact is. As with the Gutenberg CI workflows, many if not most additive "optimizations" (such as adding caches) that have been applied to speed things up (and which had positive local micro-benchmark wins) have ended up not helping when situated within a bigger and more complicated system, but they all introduce latent bugs and maintenance burden.

The fact that we're all recognizing a wide array of places where improper cache invalidation could triggers visible breakage in sites speaks to me that there's a high risk in caching this function the way we're talking about. I think this current discussion started when this same subsystem blocked all wp-admin CSS; if we're talking about similar levels of consequences, or worse (by handing out the wrong CSS for rendered frontend pages) then I'd definitely prefer some performance overhead.

I'd be curious to see how "170+ times per page" really pans out against the context of all the other existing calls in a typical WP page render to know if it's a major problem or a drop in the bucket. Once we're looking at sites with a page cache in place (which hopefully is basically any and all actual production sites) how much does this cache matter since most requests will be served without ever contacting PHP? (talking about non-auth public page renders handled by something else, like nginx).

Just some thoughts. Thanks for your continued diligence on this, and desire to avoid needless overhead.

[flixos90]

@dmsnell I measured performance of the latest static cache approach in ​https://docs.google.com/spreadsheets/d/1lTw0d4iyNJ91Jg0Ru3RXCMvSASRaV5vFIsWIkgOjBTU/edit#gid=679033418: The relative performance win for the function's overall execution time is significant (0.04ms compared to 4.34ms). Overall these values are small, but it is critical that we keep optimize performance of WordPress across the board, and those improvements add up over time. The data we have gathered clearly shows this is worthwhile.

Has anyone complained about this code slowing down their sites?

I don't think this is how we should approach performance optimization. The average WordPress end user has no idea why their site is slow; they wouldn't be able to identify a concrete function in WordPress core to be slow, so we cannot rely on someone complaining.

Ship without a cache so we can better gather data.

Gather data how? We don't have a way to gather data on how a concrete function performs on production sites, so we need to do our due diligence on assessing performance as we are developing it.

The fact that we're all recognizing a wide array of places where improper cache invalidation could triggers visible breakage in sites speaks to me that there's a high risk in caching this function the way we're talking about. I think this current discussion started when this same subsystem blocked all wp-admin CSS

There has not been outlined any concrete breakage that occurred during testing of this performance enhancement. This is a non-persistent cache, so it would only need to be invalidated if the current theme switched within the same request and if the function would be called before and after the theme switch.

The wp-admin breakage was a critical problem, but that was unrelated to the performance implications of cache usage, and we have already found a way to solve this problem.

[dmsnell]

Thanks @flixos90 - I'm definitely not suggesting we ignore performance problems. My comment was supposed to say something more like "can we establish that the metrics we've collected amount to the scale of the problem we think they do?"

To that end I suspect measuring real page renders with req/s might be more illustrative of the real performance impact, and also that we consider the hit-rate of the cache when thinking about standard recommended setups, including HTTP-server page caches that eliminate all of the code in question for the overwhelming majority of page renders.

Again, the 48ms value seems wildly unexpected, and if a call to file_exists() can amount to that much time then something seems very wrong somewhere. A value of 4ms seems less surprising, though.

Did anyone already mention or discuss how ​is_readable() is already cached by PHP? I thought the discussion centered around the filesystem access, but maybe it's more about get_template_directory()?

[hellofromTonya]

Replying to dmsnell:

Again, the 48ms value seems wildly unexpected, and if a call to file_exists() can amount to that much time then something seems very wrong somewhere. A value of 4ms seems less surprising, though.

Did anyone already mention or discuss how ​is_readable() is already cached by PHP? I thought the discussion centered around the filesystem access, but maybe it's more about get_template_directory()?

It don't think it's about is_readable(). It's about the calls to get_stylesheet_directory() and get_template_directory() and each of their call stacks.

I think in this case, the memoization (static cache variable) approach implemented in ​PR 3887 is a win. Why?

• lean
• non-persistent
• low risk
• encapsulated
• micro-optimization gains
• no BC concerns
• easily changeable
• not uncommon in Core

What do you think @dmsnell? Any concerns for committing it?

[dmsnell]

@hellofromTonya the non-persistent cache seems minimal in this case, which is a win. I have no stake in this so I don't have strong opinions unless we're talking about adding layers of caching and complexity to try and solve a problem we don't fully understand 😉

[hellofromTonya]

Replying to dmsnell:

@hellofromTonya the non-persistent cache seems minimal in this case, which is a win. I have no stake in this so I don't have strong opinions unless we're talking about adding layers of caching and complexity to try and solve a problem we don't fully understand 😉

Agreed. Keeping it simple until there's data to show something more complex is needed.

Thanks @dmsnell! Appreciate your input.

[hellofromTonya]

Seems there's alignment and consensus to move forward committing ​PR 3887 as it implements the memoization (static cache variable) lean approach. Will let this sit until tomorrow just to make sure before committing.

[hellofromTonya]

Thank you everyone for helping in this effort!

With 4 committer approvals on ​PR 3887 and the concerns resolved, I'm now in the process of preparing the commit.

[hellofromTonya]

In 55138:

Themes: Add static cache variable to wp_theme_has_theme_json().

For performance, a static variable is added to wp_theme_has_theme_json() to cache the boolean result of determining if a theme (or its parent) has a theme.json file.

This cache avoids the overhead of calling get_stylesheet_directory() and get_template_directory() each time wp_theme_has_theme_json() is invoked.

The cache is lean, non-persistent, and encapsulated within the function (i.e. function scope and not available externally).

The cache is ignored when:

• WP_DEBUG is on to avoid interrupting theme developer's workflow and for extender automated test suites.
• WP_RUN_CORE_TESTS is on to ensure each Core test exercises the checking code.

Follow-up to [55092], [55086].

Props oandregal, azaozz, costdev, dmsnell, flixos90, hellofromTonya, Otto42, spacedmonkey.
Fixes #56975.

Thank you everyone for your contributions to land on this solution. Committed via changeset https://core.trac.wordpress.org/changeset/55138.

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.

Closing in favor of #3887, which was committed as the solution 🎉

Closing in favor of ​https://github.com/WordPress/wordpress-develop/pull/3887, which was committed as the solution 🎉

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?

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.

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 😄

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.

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?

@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.

Committed in https://core.trac.wordpress.org/changeset/55155 🎉

Thanks everyone for helping to land all these PRs.

[spacedmonkey]

This PR is still awaiting review ​#3876 @hellofromTonya @flixos90 @oandregal

Closing for now

[flixos90]

Replying to spacedmonkey:

This PR is still awaiting review ​#3876 @hellofromTonya @flixos90 @oandregal

This is a separate enhancement to the new function, so it should rather be handled in a separate ticket. Only if there was a related bug we would need to reopen this one. I opened #57629 for this purpose and assigned to you for now.

@mukeshpanchal27 @felixarntz

I have include another commit, to also use the theme_file_path filter in theme json resolver. If we are going to implement using this filter in one place, it needs to be done in all.

think of this use case.

• Developer, has a filter that filters the location of the theme.json file.
• Calls wp_theme_has_theme_json, get return true.
• But then, get_theme_data in theme json resolver, use get_file_path_from_theme that does not respect that.

Why is_readable() vs file_exists()?

is_readable — Tells whether a file exists and is readable
Returns true if the file or directory specified by filename exists and is readable, false otherwise.

​https://www.php.net/manual/en/function.is-readable.php

file_exists — Checks whether a file or directory exists
Returns true if the file or directory specified by filename exists; false otherwise.

​https://www.php.net/manual/en/function.file-exists.php

is_readable() then is an extra safety measure to ensure the file is usable for reading it into memory.

[spacedmonkey]

Now that we have wp_theme_has_theme_json in core, can we use in _wp_theme_json_webfonts_handler and return early and skip this logic? CC @hellofromTonya

[hellofromTonya]

Replying to spacedmonkey:

Now that we have wp_theme_has_theme_json in core, can we use in _wp_theme_json_webfonts_handler and return early and skip this logic? CC @hellofromTonya

@spacedmonkey @flixos90 if modifying _wp_theme_json_webfonts_handler() provides a significant performance boost, then please open a new Trac ticket for further discussion and analysis.

That said, I'm hesitant to touch this function. Why?

• The function is a temporary stopgap.
• The function will be deprecated and add_action( 'plugins_loaded', '_wp_theme_json_webfonts_handler' ); removed when the Fonts API is introduced in Core, which is targeted for 6.3.
• The code is a backport from Gutenberg during 6.0.0.

Unless there's a big big performance boost, I'd prefer not to touch _wp_theme_json_webfonts_handler().

[spacedmonkey]

Added dev note keyword. I believe this change needs a dev note. Is a dev note in the works? @hellofromTonya

[oandregal]

I left the note at ​https://github.com/WordPress/gutenberg/issues/47771#issuecomment-1420609398

wp_theme_has_theme_json

WordPress 6.2 introduces a new method called wp_theme_has_theme_json(), that returns whether the active theme or its parent has a theme.json file. The goal is to provide 3rd parties with a public API they can use to query the active theme for this information.

Being so small, I suggested it could go as part of other devnote: either a "miscellaneous changes" note or a theme.json focused one. I don't have good visibility of what those changes would be, so I mentioned I'd leave space for others with more information to make that call. cc @bph for awareness of this conversation as we talked about this.

For reference, this is how we dealt with related dev notes in the past:

• ​https://make.wordpress.org/core/2021/06/25/introducing-theme-json-in-wordpress-5-8/
• ​https://make.wordpress.org/core/2022/01/08/updates-for-settings-styles-and-theme-json/
• ​https://make.wordpress.org/core/2022/01/04/new-api-to-access-global-settings-styles/
• ​https://make.wordpress.org/core/2022/10/10/filters-for-theme-json-data/

[spacedmonkey]

Replying to hellofromTonya:

Unless there's a big big performance boost, I'd prefer not to touch _wp_theme_json_webfonts_handler().

See #57814.

## 2021 theme.
### Before

### After

This saves 40+ calls to is_readable which saves 48ms per page request. This is worth doing.

Sorry for not directly relevant question, but could you let me know which tool you used for this profiling?

is_readable() then is an extra safety measure to ensure the file is usable for reading it into memory.

I recommend using is_readable, as this what is correctly in core.

I think using the theme_file_path is extremely useful for developers.

I would like to unblock this, can I get a review of this.

@hellofromtonya @oandregal @felixarntz

My main comment is that these changes should happen in Gutenberg first. This is something I have recurrently mentioned, and it takes time and energy for everyone. I feel I sound like a broken record. I'm sorry about that. Is there something I can do to clarify this, so nobody needs to waste energy/time on this again?

For full transparency: other than I already commented ​back in February, I don't fully understand how this is impactful (performance, users, etc.). I feel others would be better equipped than I am to help figure this out.

I feel I sound like a broken record. I'm sorry about that.

Can you point me in the direction of the documented process for this? @oandregal

Can you point me in the direction of the documented process for this?

As far as I know, it has been good etiquette among people that work together, and it has been transmitted PR to PR in comments. I have been personally doing this for months, if not a year. I know @azaozz and @hellofromtonya have been sharing this as well in other channels, they will have more context as to where.

I don't know that there is a handbook page anywhere that states this. I'm happy to help writing one down if that's useful to help us focus on solving user's problems.

Following on from the above, is this process documented in a handbook yet? If there are files in core that need to be modified first in Gutenberg then ideally there should be a test that fails when they're modified in a core PR.