Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#58528 closed defect (bug) (fixed)

Performance issue in register_block_style_handle

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: high
Severity: normal Version: 5.8
Component: Editor Keywords: has-patch
Focuses: performance Cc:

Description

The function register_block_style_handle is extremely resource intensive to run. While profiling, it is noted that this function is called 184 times on the average page load ( register styles for core blocks ). This function call alone can result in around 10% of server time to generate a page.

This is because realpath is called. Realpath can be an expensive function to run. Especially if the file does not exist. In [51102], core blocks are register with styles, even if the files does not exist, to allow developers to register inline styles.

This code should be refactored to check if file exists before running realpath.

This ticket was orignally created on the gutenberg repo - https://github.com/WordPress/gutenberg/issues/50889

Attachments (1)

Screenshot 2023-06-13 at 13.21.50.png (146.3 KB) - added by spacedmonkey 11 months ago.

Download all attachments as: .zip

Change History (19)

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


11 months ago
#1

  • Keywords has-patch added

This ticket fixes two issues with block registration.

  1. Fix an issue, introduced in https://github.com/WordPress/wordpress-develop/commit/716a99587c04e5b37b4ceacc3ccce6c72c842464. Basically, Register styles that the we know the file do not exist, results in calling realpath on a file that is not there. This means realpath caches and is expensive.
  2. Also had a simple check, for file_exists before realpath is called, for third party blocks that are also registering styles that are not there.

This can result in some really big server response time gains.

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

#2 @spacedmonkey
11 months ago

@aristath @flixos90 @joemcgill

So I have been doing some discovery on this ticket. I found the issue. realpath is being in register_block_style_handle, on styles that we know do not exist. Realpath on file that do not exist, are expensive and are not cached. In https://github.com/WordPress/wordpress-develop/commit/716a99587c04e5b37b4ceacc3ccce6c72c842464, styles are register for editor and fe, in a way that they are meant to register with URL false, so developer can register inline styles.

So there are two problems in the core.

  1. For core blocks we know do not have styles for FE or editor styles, skip calling register_block_style_handle and just call wp_register_style with false. This means no backwards compatibility break. This avoid, the call to realpath in register_block_style_handle and other logic that is not needed, as we know the exist does not exist.
  2. For third party blocks we know what are doing this as well, add a file_exist before calling realpath. This still takes up time and I/O, but it is much faster than realpath, that does symlink lookup etc when called.

I put together PR here.
Benchmarks on 5000 requests

Trunk PR
Response Time (median) 300.03 283.83
wp-load-alloptions-query (median) 0.71 0.71
wp-before-template (median) 34.27 29.61
wp-before-template-db-queries (median) 3.14 3.22
wp-template (median) 58.85 59.34
wp-total (median) 93.45 89.59
wp-template-db-queries (median) 6.42 6.57
Last edited 11 months ago by spacedmonkey (previous) (diff)

#4 @joemcgill
11 months ago

  • Milestone changed from Future Release to 6.3
  • Owner set to spacedmonkey
  • Status changed from new to assigned
  • Type changed from enhancement to defect (bug)

Thanks, @spacedmonkey. I agree with you that it would be good to avoid unnecessary realpath calls here when possible. This ticket seems very similar in intent to #56666, which was fixed in 6.1 as a bug. I'm going to recategorize this one for now and am happy to work with you to iterate on the right approach.

#6 @spacedmonkey
11 months ago

@flixos90 @joemcgill I have done up with a much simpler solution in #4621. Avoid any realpath and file exists. Simple register all the block styles along all default styles.

This shaves off 11ms off a 156ms page load in my local testing and is much simpler and less brittle fix.

@spacedmonkey commented on PR #4621:


11 months ago
#7

CC @aristath

#9 @flixos90
11 months ago

Amazing work so far @spacedmonkey! I left some feedback on the PR and there are a few minor iterations to make, but in terms of performance this is the highest server response time win I've seen in a single PR so far: For block themes (TT3), wp-total is improved by over 30%! 🎉

For classic themes, the change doesn't have any impact, which is expected. My benchmarks there are just a bit up and down which can be attributed to variance.

Here are the full results.

#10 @spacedmonkey
11 months ago

Thanks for the benchmarks @flixos90 . I am happy with the results.

I have updated the patch based on feedback. Hopefully we can get this merged for 6.3.

@mukesh27 commented on PR #4621:


11 months ago
#11

@spacedmonkey Re-run the failed job it show Navigation timeout of 30000 ms exceeded.

@spacedmonkey commented on PR #4621:


11 months ago
#12

@felixarntz @mukeshpanchal27 I went down a habbit hole. I fixed two more issues. First there is a fix for https://core.trac.wordpress.org/ticket/58605. Changing the glob, fixes this.
Secondly, looking at the function wp_common_block_scripts_and_styles. This too does a glob on css files as well. It doesn't make sense to do a glob in two places. So I moved this logic to this function as well. This has some really nice saves as well. To test this, you should switch to the tt2 theme.

This PR now needs some unit tests, I will work on the ASAP.

@flixos90 commented on PR #4621:


11 months ago
#13

@spacedmonkey I don't think it's a good idea to intertwine the two tickets in this pull request. Yes, they are related but we're also close to the 6.3 beta, and you now added lots of new logic that changed the PR notably from what it was before.

I would have approved it now in its original scope, but the new changes make this a whole lot more complicated and potentially risky since you moved logic from one place to the other. I would suggest that you revert those changes and open a separate PR to address https://core.trac.wordpress.org/ticket/58560. That ticket is much less critical, and it's not even in the 6.3 milestone. Let's tackle it separately. Then we can get this PR merged in its original scope.

@spacedmonkey commented on PR #4621:


11 months ago
#14

@felixarntz Fair point, adding back is simple enough. I will do it as part of that release. I have reverted the changes now, can you take a look again.

@spacedmonkey commented on PR #4621:


11 months ago
#16

@felixarntz @mukeshpanchal27 As requested, I have added unit tests. The unit tests have highlighted that the incorrectly value was being return. I moved the some the logic in register_block_style_handle around. This does two things, one skips the need to call wp_should_load_separate_core_block_assets, which resulted in a 1ms of load time. Two, calls generate_block_asset_handle and gets the correct style handle name early, so it can be used with wp_style_is.

This change now is even better of a performance improvement.

#17 @spacedmonkey
11 months ago

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

In 56044:

Editor: Register core block styles in one place.

Register all core blocks in a new function called register_core_block_style_handles. This mirrors the function wp_default_styles where all core styles are registered in one place. This improves block registration performance, as it avoids expensive file lookups, like realpath in register_block_style_handle. The new function register_core_block_style_handles uses glob to get all css files in the blocks directory. This glob is cached in a transient to save lookups on subsequent requests. The function register_block_style_handle now checks to see if the style handle is already registered before trying to register it again.

Props mukesh27, westonruter, flixos90, joemcgill, spacedmonkey.
Fixes #58528.

Note: See TracTickets for help on using tickets.