Performance of wp_maybe_inline_styles

[spacedmonkey]

The function wp_maybe_inline_styles is called twice on templates. Hooked into wp_head and wp_footer. wp_maybe_inline_styles loops through all register styles to inlines them. But a the file operations, like filesize or file_get_contents can be expensive. On the first run, there should be some level or caching or flag set, to ensure that this function can be run multiple times without run the expensive file operations more than once.

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

[spacedmonkey]

Brenchmark 1000 runs on TT3

	Trunk	PR
Response Time (median)	107.41	97.72
wp-load-alloptions-query (median)	0.87	0.86
wp-before-template (median)	51.6	50.04
wp-before-template-db-queries (median)	4.35	4.25
wp-template (median)	49.58	41.54
wp-total (median)	101.94	92.05
wp-template-db-queries (median)	2.51	2.44

[flixos90]

This is an excellent find!

I just conducted a benchmark for both TT3 (block theme) and TT1 (classic theme), and can report that there is definitely a notable impact on performance for both of them, even classic themes:

• For TT3, wp-template is ~21% faster, making overall response time ~7% faster.
• For TT1, wp-template is ~5% faster, making overall response time ~2% faster.

That all makes sense, given that classic themes don't rely on the affected logic as heavily, e.g. don't commonly have stylesheets without a $src enqueued. Still, even there the changes of caching the file size brings a performance benefit that is clear to see in the data.

​Here are the full results.

Given this is a major performance win, I'll milestone this for 6.3.

@joemcgill @felixarntz I am going to add some unit tests to wp_maybe_inline_styles. Add it seems to have none at all :(

@joemcgill @felixarntz Actioned feedback on tests. If you are happy, then I will commit.

This is a nice enhancement. What's unclear to me is why this is being hooked to both the wp_head and wp_footer action. It doesn't seem like this function was designed to be run twice. In fact, wp_enqueue_style doesn't even support an in_footer param like wp_enqueue_script,

I presume this is to print styles that were registered after the header action fires, similar to print_late_styles().

​https://github.com/WordPress/wordpress-develop/blob/c9f3a682812c4df3fde221969619acfdf529ec72/src/wp-includes/script-loader.php#L2228-L2238

[spacedmonkey]

In 55888:

Script Loader: Improve performance of wp_maybe_inline_styles function.

The wp_maybe_inline_styles function is called twice on the average page load. On it's second run however, it did not check to see if the style had already been processed on the first run. This resulted in calling filesize and get_file_contents unnecessarily, which was bad for performance. Now, the loop around the queued styles, checks to see if the source is set to false, meaning it has already been processed. This change also replaces calls to filesize with the core function wp_filesize, which improves extensibility.

Props spacedmonkey, flixos90, peterwilsoncc, joemcgill.
Fixes #58394.

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

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

[dd32]

Just noting that this change has caused PHP Notices on WordPress.org. I'm unsure if this is a WordPress.org-specific oddity, or if it's a sign of something wider.

E_NOTICE: Undefined index: block-name-here in wp-includes/script-loader.php:2875
E_NOTICE: Trying to get property 'src' of non-object in wp-includes/script-loader.php:2875

In short, $wp_styles->registered[ $handle ] is null / unset, but the $handle is present within $wp_styles->queue.

Previously this didn't throw a notice, as wp_styles()->get_data( $handle, 'path' ) returned false.

I believe the trigger is this code:

add_filter( 'wp_enqueue_scripts', function() {
	wp_deregister_style( 'wporg-block-name' );
}, 201 );

It appears that deregistering a style that is enqueued, doesn't actually dequeue the style, just removes the data and relies upon script-loader to skip over it when outputting on the page. It appears that WP_Scripts relies upon WP_Dependencies::do_item() to short-circuit when processing a no-longer-registered item:
​https://github.com/WordPress/wordpress-develop/blob/e8846318cf9a4c8d012ae32b4de9a51655237a24/src/wp-includes/class-wp-dependencies.php#LL159C1-L161C2

re-opening this as a result.

[spacedmonkey]

We need to change the following

foreach ( $wp_styles->queue as $handle ) {
    $src  = $wp_styles->registered[ $handle ]->src;

to

foreach ( $wp_styles->queue as $handle ) {
    if( ! isset( $this->registered[ $handle ] ) ){
       continue;
    }
    $src  = $wp_styles->registered[ $handle ]->src;

There should also be a unit test for this ^.

Thoughts @dd32 @flixos90.

[dd32]

Replying to spacedmonkey:

Thoughts

Works for me.

Draft, trying to write a test.

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

[spacedmonkey]

Replying to dd32:

Replying to spacedmonkey:

Thoughts

Works for me.

I have created a PR / patch for your review at ​#4597. This patch adds a unit tests as well, to ensure this bug will not happen again.

^@kebbet

[spacedmonkey]

In 55909:

Script Loader: Add a check to see in style is registered in wp_maybe_inline_styles.

Add a check in wp_maybe_inline_styles to check that style is registered before processing items in queue. It is possible that developers may have called wp_deregister_style, unregistering style but the style still be in the queue to be processed. Without this check, typing to access the src property would result in a notice error.

Follow on from [55888].

Props spacedmonkey, flixos90, dd32, kebbet.
See #58394.

Committed.