Make WordPress Core

Opened 12 months ago

Closed 9 months ago

Last modified 9 months ago

#58235 closed enhancement (fixed)

Enhance hero image loading performance with Fetchpriority

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 6.3 Priority: high
Severity: normal Version:
Component: Media Keywords: has-patch commit has-dev-note add-to-field-guide
Focuses: performance Cc:

Description

The fetchpriority attribute is a standard HTML attribute that allows annotating resources (e.g. images, scripts, stylesheets, ...) on a web page to give them a certain priority.

While the attribute can be used on different resources, this ticket is scoped to images only. Using the fetchpriority attribute is recommended with a value of “high” on the most important image on a page, which is an important performance best practice that improves LCP performance.

Please see the proposal post for additional context.

As mentioned in the post, support for fetchpriority on images should be implemented relying on similar logic as lazy-loading images uses. Yet, the two attributes need to function on their own, e.g. even sites that disable lazy-loading should be able to use Fetchpriority. Therefore the engineering effort to support the fetchpriority attribute will involve decoupling that logic from the loading attribute.

Last but not least, it should also be highlighted that, since fetchpriority can be used on other elements on images, we should consider that for the implementation. Certain parts of the implementation may be specific to images while other parts may apply to fetchpriority on any resource.

Change History (42)

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


12 months ago

#2 @flixos90
11 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Owner set to flixos90
  • Priority changed from normal to high
  • Status changed from new to assigned

Milestoning this for 6.3 and as a high priority for the performance team, given that this is part of our team roadmap.

This ticket was mentioned in PR #4495 on WordPress/wordpress-develop by kt-12.


11 months ago
#3

  • Keywords has-patch added

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


11 months ago

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


11 months ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


10 months ago

@flixos90 commented on PR #4495:


10 months ago
#7

@kt-12 In https://github.com/WordPress/wordpress-develop/pull/4495/commits/2405a5a44740e42cdc82c2952e67265e71ead0a1, I pushed a few changes to ensure the existing iframe logic doesn't call the now deprecated wp_get_loading_attr_default() function:

  • wp_iframe_tag_add_loading_attr() now uses the new function.
  • The new function now works for iframe tags too.
  • The private functions were enhanced with a $tag_name parameter, per my last code review feedback.
  • For iframe tags, the fetchpriority private function returns early because we don't want to handle that yet.
  • A TODO comment was added in wp_iframe_tag_add_loading_attr() to eventually call the function in a cleaner way, however the dimension do not matter at this point, so parsing out the actual image dimensions would be wasteful for now. It's a bit weird, but as long as there's a comment explaining it, it's okay to have in there. Once we add fetchpriority support for iframes in the future, that'll be removed.

@spacedmonkey commented on PR #4495:


10 months ago
#8

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/ed855cde-8ec3-48d8-865c-1dc45672dd45

If you have the same image twice in content, both of the images get fetchpriority as high. Is this to be expected?

@spacedmonkey commented on PR #4495:


10 months ago
#9

Doing benchmarks, I can also see performance regeneration.

Benchmark, TT3, 500 runs. Tested with post block gallery ( from

Trunk

Response Time (median),97.08
wp-load-alloptions-query (median),0.6
wp-before-template (median),47.64
wp-before-template-db-queries (median),3.75
wp-template (median),42.53
wp-total (median),90.17
wp-template-db-queries (median),4.23

PR
Response Time (median),99.34
wp-load-alloptions-query (median),0.6
wp-before-template (median),47.21
wp-before-template-db-queries (median),3.68
wp-template (median),44.72
wp-total (median),92.39
wp-template-db-queries (median),4.22

@spacedmonkey commented on PR #4495:


10 months ago
#10

It seems to be slow to use WP_HTML_Tag_Processor. We may need to consider using a regex for now, for performance reason. Maybe in a later release where we can use WP_HTML_Tag_Processor to handle get / updating all html attributes.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/2a7f18a7-4e52-4635-bda4-c8345d6a3fe3

### Trunk
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/8d18d078-71c0-4f25-b518-a2dadd1cb88a

### PR
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/54d64828-83e9-4e19-acdf-1a66c4548bbe

@flixos90 commented on PR #4495:


10 months ago
#11

@spacedmonkey Good point. While eventually it would be great to refactor all of wp_filter_content_tags() to use WP_HTML_Tag_Processor, right now we're only doing it in a single place, partially, so maybe in that case that leads to a performance regression.

We should probably try to replace that code with regular expressions to get the attribute values and see if that gets rid of the regression. That's not as nice-looking code for sure, but nice-looking code is not as important as performance.

It also doesn't mean that WP_HTML_Tag_Processor in general is slow, but probably the way we use it here, mixed with regular expressions, is probably not efficient.

joemcgill commented on PR #4495:


10 months ago
#12

I think it was a good idea to give the WP_HTML_Tag_Processor approach a try here, and really instructive to see that it seems to be slower than manipulating the HTML string manually. I agree that we should go back to the regex approach used in other places and do another comparison.

@spacedmonkey commented on PR #4495:


10 months ago
#13

@kt-12 If you can revert using WP_HTML_Tag_Processor and replace with regex<, I will return profiles again. We can then compare the three profiles and see what the overhead of using WP_HTML_Tag_Processor is.

kt-12 commented on PR #4495:


10 months ago
#14

https://i0.wp.com/user-images.githubusercontent.com/237508/246151529-ed855cde-8ec3-48d8-865c-1dc45672dd45.png
If you have the same image twice in content, both of the images get fetchpriority as high. Is this to be expected? I am also not seeing lazy loading at all anymore.

This should not be the case ideally. I'll have a look at this.

kt-12 commented on PR #4495:


10 months ago
#15

@kt-12 If you can revert using WP_HTML_Tag_Processor and replace with regex<, I will return profiles again. We can then compare the three profiles and see what the overhead of using WP_HTML_Tag_Processor is.

@spacedmonkey I have replaced it with regex (commit), you can try profiling now. cc: @felixarntz

@flixos90 commented on PR #4495:


10 months ago
#16

@kt-12 As discussed earlier, I pushed a few commits, some with miscellaneous test fixes following review, but most importantly:

The main piece of work left to do now is to replicate the existing test coverage for the deprecated wp_get_loading_attr_default() function for the new wp_get_loading_optimization_attributes() function. For the most part, you can probably copy the existing tests and update them to rely on the new function.

@spacedmonkey commented on PR #4495:


10 months ago
#17

@kt-12 This PR needs a rebase.

#18 @spacedmonkey
10 months ago

I have done profiling and benchmarking on this PR.

Benchmark - TT theme - 500 runs - Theme test data - gallery post.

Trunk - 55962 PR - a2c977f
Response Time (median) 56.29 55.96
wp-load-alloptions-query (median) 0.62 0.62
wp-before-template (median) 18.56 18.43
wp-before-template-db-queries (median) 1.35 1.35
wp-template (median) 30.46 30.31
wp-total (median) 49.17 48.93
wp-template-db-queries (median) 5.58 5.53

Here are comparison using blackfire.

This PR is showing a net benefit to php performance. Which is a nice side effect.

@thekt12 @flixos90

#19 @mukesh27
10 months ago

  • Keywords needs-dev-note added

This ticket was discussed in the recent performance bug scrub.

needs-dev-note added for new feature.

Props to @joemcgill @spacedmonkey

@spacedmonkey commented on PR #4495:


10 months ago
#20

@kt-12 Can you rebase this PR. Once done, i will re-review and re-profile.

#21 @flixos90
10 months ago

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

In 56037:

Media: Automatically add fetchpriority="high" to hero image to improve load time performance.

This changeset adds support for the fetchpriority attribute, which is typically added to a single image in each HTML response with a value of "high". This enhances load time performance (also Largest Contentful Paint, or LCP) by telling the browser to prioritize this image for downloading even before the layout of the page has been computed. In lab tests, this has shown to improve LCP performance by ~10% on average.

Specifically, fetchpriority="high" is added to the first image that satisfies all of the following conditions:

  • The image is not lazy-loaded, i.e. does not have loading="lazy".
  • The image does not already have a (conflicting) fetchpriority attribute.
  • The size of of the image (i.e. width * height) is greater than 50,000 squarepixels.

While these heuristics are based on several field analyses, there will always be room for optimization. Sites can customize the squarepixel threshold using a new filter wp_min_priority_img_pixels which should return an integer for the value.

Since the logic for adding fetchpriority="high" is heavily intertwined with the logic for adding loading="lazy", yet the features should work decoupled from each other, the majority of code changes in this changeset is refactoring of the existing lazy-loading logic to be reusable. For this purpose, a new function wp_get_loading_optimization_attributes() has been introduced which returns an associative array of performance-relevant attributes for a given HTML element. This function replaces wp_get_loading_attr_default(), which has been deprecated. As another result of that change, a new function wp_img_tag_add_loading_optimization_attrs() replaces the more specific wp_img_tag_add_loading_attr(), which has been deprecated as well.

See https://make.wordpress.org/core/2023/05/02/proposal-for-enhancing-lcp-image-performance-with-fetchpriority/ for the original proposal and additional context.

Props thekt12, joemcgill, spacedmonkey, mukesh27, costdev, 10upsimon.
Fixes #58235.

#23 @flixos90
10 months ago

In 56110:

General: Add missing parentheses to functions referenced in _deprecated_function() calls added in 6.3.

See #58235, #58301, #58555.

@flixos90 commented on PR #4756:


10 months ago
#25

cc @kt-12

#26 @flixos90
10 months ago

In 56112:

Media: Fix inconsistent docs for existing wp_img_tag_add_loading_attr filter and remove duplicate.

Follow-up to [56037].

Props thekt12.
See #58235.

#28 @spacedmonkey
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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


10 months ago
#29

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

Follow on from https://core.trac.wordpress.org/changeset/56037.

Move the check to see if properties exist before doing anymore processing.

#30 @spacedmonkey
10 months ago

Based on the feedback, Felix is correct. I am closing again.

#31 @spacedmonkey
10 months ago

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

@spacedmonkey commented on PR #4766:


10 months ago
#32

@felixarntz Good catch. Sorry I missed that comment how. Closing this PR.

#33 @flixos90
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening due to a small bug that [56037] reintroduced after [55825] / #58089 had fixed it: Programmatically injected images within post content (the_content filter) should be skipped except when the the_content context is used so that they are only considered once, as part of the whole post content blob.

[55825] fixed that, but then the refactoring as part of this ticket accidentally added the postprocessing logic call to that clause so that now such images may incorrectly receive fetchpriority="high" when they should not.

See https://core.trac.wordpress.org/ticket/58681#comment:13, which originally discovered this regression.

@flixos90 commented on PR #4800:


10 months ago
#35

@kt-12 @spacedmonkey Could you please take a look?

#36 @flixos90
9 months ago

In 56154:

Media: Ensure that the image widget supports loading optimization attributes.

This changeset adds support for loading optimization attributes such as loading="lazy" and fetchpriority="high" to the image widget. A new context widget_media_image is introduced for that purpose.

Props spacedmonkey, thekt12, mukesh27, westonruter.
Fixes #58704.
See #58235.

#37 @mukesh27
9 months ago

  • Keywords commit added

Thanks @flixos90, The PR got enough approval for merge.

commit. keyword added for consideration.

#38 @flixos90
9 months ago

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

In 56164:

Media: Avoid programmatically created images within post content from incorrectly receiving fetchpriority="high".

Follow-up to [56037], as that changeset accidentally did not consider the changes made in [55825]: Images that are programmatically injected into post content (e.g. through a block, or shortcode, or any hook calling a function like wp_get_attachment_image()) must be treated as part of the whole post content blob since otherwise the heuristics for applying fetchpriority="high" and loading="lazy" are skewed by parsing certain images before others rather than sequentially parsing the entire post content. [55825] addressed that for lazy-loading, but when [56037] introduced fetchpriority support, the related refactor missed making the same consideration for that attribute.

Props flixos90, spacedmonkey, thekt12, mukesh27.
Fixes #58235.
See #58089.

#41 @flixos90
9 months ago

  • Keywords needs-dev-note removed

#42 @stevenlinx
9 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.