Use `wp_theme_has_theme_json` in `_wp_theme_json_webfonts_handler`

[spacedmonkey]

Follow on ticket from #56975.

_wp_theme_json_webfonts_handler is used to register font is theme.json is present. However, not all themes have theme.json. Use wp_theme_has_theme_json function to end processing early if not theme.json is present.

[spacedmonkey]

Before

[spacedmonkey]

After

[spacedmonkey]

As you can see from the above graphs, marked render start much early if font system is not loaded. At 32ms mark instead of 38ms mark. Meaning part start to render faster if this is conditionally loaded.

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

[hellofromTonya]

Thanks for opening this ticket @spacedmonkey.

FYI: This code is temporary and a stopgap. The plan is to remove it in 6.3 and replace with the Fonts API.

What's the status on the API?
Currently being stabilized in the Gutenberg repo. I'm not seeing any blockers or concerns for it to land in 6.3.

Thinking the proposed fix for this ticket can stay in Awaiting Review just in case for some reason the API doesn't land in 6.3.

[spacedmonkey]

I still think we can ( and should ) land this change. It doesn't hurt making this very small change and their are benefits to it. We can even land it in a point release.

This fix should also be part of the final implementation.

[flixos90]

@spacedmonkey @hellofromTonya I have conducted a performance benchmark for ​https://github.com/WordPress/wordpress-develop/pull/4185 in ​this spreadsheet. I did two comparisons, with 250 runs each. For the first one I manually calculated the total load time from the two other metrics, which however is not entirely correct statistically, so then I also ran a second check where each of the runs had its own total load time.

Summary of the results:

• The fix here speeds up the time WordPress takes to bootstrap, before it starts template output between 11-16% at the median. The faster bootstrap time is expected, since by adding the extra condition here the logic in _wp_theme_json_webfonts_handler() is not run, which is hooked into plugins_loaded, i.e. runs very early.
• At the same time, the fix slows down the time WordPress takes to render the template between 9-16% at the median. This needs more investigation. I was first thinking that it might make sense because of the theme.json parsing happening later now, but on second thought I wonder why that is happening additionally. There may be something else that is normally calculated in here and now due to bailing in the logic still happens later.
• The total load time sees an improvement of ~1% at the moment based on the statistically correct wp-total median.

It is safe to say that this very small change can have wide performance implications, so I think we should continue to explore it. The fact that this is a private function that hopefully gets removed in 6.3 doesn't mean we cannot enhance it, particularly if there is a real performance impact to be made.

That said, right now the partial regression for the template rendering time means we need to figure out what is happening there and why. If we can do that and solve the remaining problem, this ticket has the potential to be a significant performance enhancement.

This deserves a closer look per https://core.trac.wordpress.org/ticket/57814#comment:5.

@spacedmonkey Please see my ticket comment and the performance data. Can you investigate what is going on there and open a new follow up PR? There must be something _after_ the template_include filter which together with this PR causes things to slow down. I think you're on the right track with this PR so far, but it appears that there is something else to it.

If we can avoid the regression in the wp-template metric, there's a chance that this can lead to a massive performance improvement for classic themes total load time.

[spacedmonkey]

@flixos90 Did you test other request types, like REST API calls, admin, RSS feeds etc. This change improves the bootstrap, if like in the case of REST API call, theme.json is very processed, it might need up REST API calls by 11-16% without the 9-16% lost in template.

[flixos90]

@spacedmonkey I did not, that's a fair point. However, frontend performance is probably the most important here. We need to investigate why the wp-template metric is seeing a slowdown. If we can figure that out, I think there is a massive opportunity here.

The performance win in wp-before-template is ~5ms, so if we can avoid the wp-template slowdown, this would mean a total performance win of ~7% (~5ms of ~69ms) which would be huge!

[spacedmonkey]

Current trunk.

[spacedmonkey]

Current PR

[spacedmonkey]

@flixos90 I don't think the FE performance is the only metric we judge things on. If this had no FE benefit, it would still be worth doing.

As you can see from the above screenshots are the the posts REST API endpoint, there is a nearly 4ms improvement.

[spacedmonkey]

This ticket would go in combo with #57864

[flixos90]

Replying to spacedmonkey:

@flixos90 I don't think the FE performance is the only metric we judge things on. If this had no FE benefit, it would still be worth doing.

As you can see from the above screenshots are the the posts REST API endpoint, there is a nearly 4ms improvement.

I didn't say that it was the only metric, I said it was the most important one. Frontend performance affects every WordPress experience a website visitor has, except for a small fraction of headless sites. The horizontal impact of performance in the REST API is far lower. I'm not saying it is not important, but it is less important.

Anyway, this is a very promising enhancement when it comes to performance. However, we shouldn't commit this without figuring out why rendering the template sees a regression. I would love for this change to land, but we need to take the time and figure out the one remaining issue here before we can reasonably commit it.

[spacedmonkey]

The expensive part of _wp_theme_json_webfonts_handler, is the call to WP_Theme_JSON_Resolver::get_merged_data(). If this is not called here, is called later in wp_get_global_settings / wp_enqueue_global_styles. If we can workout caching, wp_get_global_settings persistently then that could solve this problem. We are just moving that piece of heavy compute later in the process of the page.

I will see what I can do.

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

[spacedmonkey]

Here is a simplier fix at ​https://github.com/WordPress/wordpress-develop/pull/4218.

This just load theme.json data and not all other data. This seems to fix the issue.

[flixos90]

Replying to spacedmonkey:

The expensive part of _wp_theme_json_webfonts_handler, is the call to WP_Theme_JSON_Resolver::get_merged_data(). If this is not called here, is called later in wp_get_global_settings / wp_enqueue_global_styles. If we can workout caching, wp_get_global_settings persistently then that could solve this problem. We are just moving that piece of heavy compute later in the process of the page.

I see. Makes sense then that we're basically just deferring the call to a later point in time, which looks like wp-template becomes slower, but it's roughly the same amount that wp-before-template becomes faster. So that's expected then.

Here is a simplier fix at ​https://github.com/WordPress/wordpress-develop/pull/4218.

This just load theme.json data and not all other data. This seems to fix the issue.

I ran another test for your new PR (see ​this sheet), and it really doesn't make a difference. Your new fix improves wp-before-template by ~1ms and reduces wp-template by ~1ms, while the previous PR did the same by ~4ms for both metrics - eventually the outcome for total load time is similar: Both PRs bring a win of ~0.5ms (almost 1%).

I think your original fix from ​https://github.com/WordPress/wordpress-develop/pull/4185 is much simpler and less error-prone, so I would prefer committing that one. The other fix requires a much closer attention and better understanding of the theme.json parsing logic.

[spacedmonkey]

@flixos90 Did you run any tests again the REST API or other page types?

[spacedmonkey]

@flixos90 I run the profiles again for ​#4185

Trunk
Response Time (median),89.84
wp-load-alloptions-query (median),0.65
wp-before-template (median),32.92
wp-template (median),47.91

PR
Response Time (median),87.94
wp-load-alloptions-query (median),0.66
wp-before-template (median),29.56
wp-template (median),49.64

REST API request.

Trunk
Response Time (median),58.5

PR
Response Time (median),57.8

This is on 2021, 500 runs, I am seeing a positive impact on response time , what am I missing here.

[flixos90]

I ran another benchmark with the changes from ​https://github.com/WordPress/wordpress-develop/pull/4185 applied to the WP 6.2 stable release codebase: ​https://docs.google.com/spreadsheets/d/1PmPKzvKLtN4_Zf8EG0VX0X8icZpzMz-k1yBaMdxaHow/edit

I see a consistent improvement between 0.5-1ms in overall load time (wp-total) for classic themes, and together with the fact that running all this code conceptually is unnecessary for classic themes, I think this is good to commit. In absolute numbers that's little, but it comes to ~3% of the overall response time, which is quite a win for such a tiny change.

The original PR has been closed, but it is a one line change that I approve, so marking this for commit.

[flixos90]

This is not a bug, but a performance enhancement :)

[spacedmonkey]

In 55612:

Script Loader: Return early in _wp_theme_json_webfonts_handler if theme.json not present.

Check to see if the theme.json file exists before processing the rest of the code in _wp_theme_json_webfonts_handler. This improves performance by not trying to parse the theme.json, early in the bootstrap process.

Props spacedmonkey, hellofromTonya, flixos90.
Fixes #57814.

Committed https://core.trac.wordpress.org/changeset/55612

[spacedmonkey]

Reopenning as core unit tests are failing.

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

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

[spacedmonkey]

In 55613:

Tests: Fix tests introduced in [55612].

In [55612] a change was added that made two unit tests fail. Fix these test by forcing get_merged_data method to run.

Props spacedmonkey, mukesh27.
See #57814.

Committed