Performance issue in register_block_style_handle

[spacedmonkey]

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

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

[spacedmonkey]

@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

[spacedmonkey]

Related: #58525

[joemcgill]

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.

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

[spacedmonkey]

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

CC @aristath

[spacedmonkey]

Here is blackfire compare, of trunk version this PR.

​https://blackfire.io/profiles/compare/7f7c72de-9e5c-4424-8ff6-1727776612f4/graph

[flixos90]

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.

[spacedmonkey]

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.

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

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

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

@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 FYI I just committed https://core.trac.wordpress.org/changeset/56005, which you have already accounted for here in ​https://github.com/WordPress/wordpress-develop/pull/4621/files#diff-c62beff71446448ff952e38805c8ae7652f4e2717d3ca41b1d9cd9963bd82464R38-R41 👍

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

[spacedmonkey]

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.

Committed ​https://github.com/WordPress/wordpress-develop/commit/d8409a205a40a9c5a69d668a9aa2aa3123f4c3e5