Make WordPress Core

#58394 closed enhancement (fixed)

Performance of wp_maybe_inline_styles

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

Description

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.

Change History (20)

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


11 months ago
#1

  • Keywords has-patch added

#2 @spacedmonkey
11 months ago

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

#3 @flixos90
11 months ago

  • Milestone changed from Future Release to 6.3
  • Owner set to spacedmonkey
  • Status changed from new to assigned
  • Version 5.8 deleted

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.

Last edited 11 months ago by flixos90 (previous) (diff)

@spacedmonkey commented on PR #4500:


11 months ago
#4

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

@spacedmonkey commented on PR #4500:


11 months ago
#5

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

@peterwilsoncc commented on PR #4500:


10 months ago
#6

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

#7 @spacedmonkey
10 months ago

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

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.

#10 @dd32
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Last edited 10 months ago by dd32 (previous) (diff)

#11 follow-up: @spacedmonkey
10 months ago

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.

#12 in reply to: ↑ 11 ; follow-up: @dd32
10 months ago

Replying to spacedmonkey:

Thoughts

Works for me.

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


10 months ago
#13

  • Keywords has-unit-tests added

Draft, trying to write a test.

#15 in reply to: ↑ 12 @spacedmonkey
10 months ago

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.

@spacedmonkey commented on PR #4597:


10 months ago
#16

@kebbet

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


10 months ago

#18 @spacedmonkey
10 months ago

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.

@spacedmonkey commented on PR #4597:


10 months ago
#19

Committed.

#20 @spacedmonkey
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.