Make WordPress Core

Opened 15 months ago

Closed 14 months ago

Last modified 14 months ago

#57814 closed enhancement (fixed)

Use `wp_theme_has_theme_json` in `_wp_theme_json_webfonts_handler`

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.0
Component: Script Loader Keywords: has-patch commit has-unit-tests
Focuses: performance Cc:

Description

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.

Attachments (5)

Screenshot 2023-02-27 at 18.07.58.png (386.0 KB) - added by spacedmonkey 15 months ago.
Before
Screenshot 2023-02-27 at 18.07.05.png (357.3 KB) - added by spacedmonkey 15 months ago.
After
Screenshot 2023-03-10 at 22.35.42.png (17.9 KB) - added by spacedmonkey 15 months ago.
Current trunk.
Screenshot 2023-03-10 at 22.35.38.png (18.1 KB) - added by spacedmonkey 15 months ago.
Current PR
Screenshot 2023-03-11 at 00.01.14.png (28.1 KB) - added by spacedmonkey 15 months ago.

Download all attachments as: .zip

Change History (32)

#1 @spacedmonkey
15 months ago

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.

This ticket was mentioned in PR #4185 on WordPress/wordpress-develop by @spacedmonkey.


15 months ago
#2

  • Keywords has-patch added

#3 @hellofromTonya
15 months ago

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.

#4 @spacedmonkey
15 months ago

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.

#5 @flixos90
15 months ago

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

@flixos90 commented on PR #4185:


15 months ago
#6

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.

#7 @spacedmonkey
15 months ago

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

#8 @flixos90
15 months ago

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

#9 follow-up: @spacedmonkey
15 months ago

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

#10 @spacedmonkey
15 months ago

This ticket would go in combo with #57864

#11 in reply to: ↑ 9 @flixos90
15 months ago

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.

#12 follow-up: @spacedmonkey
15 months ago

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.

#14 @spacedmonkey
15 months ago

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.

#15 in reply to: ↑ 12 @flixos90
15 months ago

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.

#16 @spacedmonkey
15 months ago

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

#17 @spacedmonkey
15 months ago

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

#18 @flixos90
14 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to spacedmonkey
  • Status changed from new to assigned

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.

#19 @flixos90
14 months ago

  • Type changed from defect (bug) to enhancement

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

#20 @spacedmonkey
14 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#22 @spacedmonkey
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopenning as core unit tests are failing.

This ticket was mentioned in PR #4267 on WordPress/wordpress-develop by @spacedmonkey.


14 months ago
#23

  • Keywords has-unit-tests added

This ticket was mentioned in PR #4267 on WordPress/wordpress-develop by @spacedmonkey.


14 months ago
#24

  • Keywords has-unit-tests added

#25 @spacedmonkey
14 months ago

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.

#26 @spacedmonkey
14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

@spacedmonkey commented on PR #4267:


14 months ago
#27

Committed

Note: See TracTickets for help on using tickets.