Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#34430 closed defect (bug) (fixed)

Improve the performance of wp_make_content_images_responsive()

Reported by: joemcgill's profile joemcgill Owned by: joemcgill's profile joemcgill
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch needs-testing needs-docs
Focuses: Cc:

Description

In an effort to make sure the content filter introduced in 34855 runs as efficiently as possible, we should pass around data like attachment metadata when possible, to avoid redundant calls to the same functions and make sure we use cached data when possible (see #34359 for example).

Attachments (13)

34430.diff (8.7 KB) - added by joemcgill 9 years ago.
34430.1.diff (29.8 KB) - added by azaozz 9 years ago.
34430.2.diff (30.0 KB) - added by azaozz 9 years ago.
34430.3.diff (30.1 KB) - added by azaozz 9 years ago.
34430.4.diff (30.8 KB) - added by joemcgill 9 years ago.
34430.5.diff (34.8 KB) - added by joemcgill 9 years ago.
34430.docs.diff (1.9 KB) - added by swissspidy 9 years ago.
34430.6.diff (15.6 KB) - added by jaspermdegroot 9 years ago.
34430.7.diff (17.4 KB) - added by jaspermdegroot 9 years ago.
Replaces 34430.6.diff
34430.8.diff (2.8 KB) - added by jaspermdegroot 9 years ago.
34430.9.diff (9.4 KB) - added by azaozz 9 years ago.
34430.10.diff (3.0 KB) - added by jaspermdegroot 9 years ago.
34430.11.diff (7.6 KB) - added by azaozz 9 years ago.

Download all attachments as: .zip

Change History (50)

@joemcgill
9 years ago

#1 follow-up: @joemcgill
9 years ago

My first approach passes both the metadata and the original image source to the other internal functions, which avoids the need to call internal media functions at all, but makes the function signatures a bit weird. In my tests, the performance of the content filter with 50 images in a post were almost twice as fast with this patch. Would love some feedback on the approach.

#2 @joemcgill
9 years ago

  • Owner set to joemcgill
  • Status changed from new to assigned

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

@azaozz
9 years ago

#4 in reply to: ↑ 1 @azaozz
9 years ago

Replying to joemcgill:

Yep, it's moving in the right direction. Got some ideas from the patch and went ahead with more changes/fixes.

In 34430.1.diff:

  • Change wp_get_attachment_image_srcset_array() and wp_get_attachment_image_sizes() to accept the image size (from the image tag), image meta and attachment ID as arguments. These are the actual bits that are needed to generate srcset and sizes.
  • Change so the image size argument is always an array of width and height. This makes it more precise and avoids any edge cases when the default image sizes have been changed by the user. There is an optional function to "translate" named sizes into arrays, not used for now and not sure if it's needed.
  • Changed so no (other) image API functions are used while generating srcset and sizes.
  • Several logic fixes and improvements.
  • Some names changed to be (hopefully) more descriptive.
  • Fixed/updated the tests.

Needs more testing that it didn't break anything.

@azaozz
9 years ago

#5 @azaozz
9 years ago

In 34430.2.diff: go back to priming the cache. Prime only the post_meta cache with update_meta_cache(). As far as I see it makes sense to do that when two or more local images are found in a post.

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


9 years ago

@azaozz
9 years ago

#7 @azaozz
9 years ago

  • Keywords needs-testing added; 2nd-opinion removed

In 34430.3.diff: refresh after r35402, r35404 and r35405.

#8 @azaozz
9 years ago

#34460 was marked as a duplicate.

This ticket was mentioned in Slack in #feature-respimg by jaspermdegroot. View the logs.


9 years ago

This ticket was mentioned in Slack in #feature-respimg by azaozz. View the logs.


9 years ago

@joemcgill
9 years ago

#11 @joemcgill
9 years ago

@azaozz 34430.4.diff is a riff off of your last patch with the following changes:

  1. Generally, I'd prefer leaving the signature for our srcset and sizes functions as is and add the image meta as a third, optional parameter. My thinking here is that we should optimize the parameters for developers who would use functions like wp_get_attachment_image_srcset() in a theme or plugin, in which case, passing $attachment_id and $size would be consistent with the other wp_get_attachment_image_ functions. This change would have no affect on the performance of our display filter.
  1. In wp_get_attachment_image_srcset_array() calling path_join() can be a bit of a drag on performance, so we should only call it when absolutely necessary.
  1. In wp_make_content_images_responsive() we can't assume that all images with the same ID are using the same markup (e.g., when the same image is embedded at two sizes on the same page, the same image using different alignments, etc.). Doing so will result in some images not getting filtered. This was causing Tests_Media:test_wp_make_content_images_responsive() to fail on my end.

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

@joemcgill
9 years ago

#13 @joemcgill
9 years ago

34430.5.diff is another iteration which introduces a few notable changes.

First, wp_get_attachment_image_srcset_array() has been removed in favor of the lower level function, _wp_calculate_image_srcset() which accepts a URL, image meta, and a size array comprised of a width and height value.

The generation of these parameters are now handled in wp_get_attachment_image_srcset() which is meant to be used in templates but is no longer used directly in core when adding a srcset attribute to images in post content or to image markup generated by wp_get_attachment_image() on the front end. Instead, wp_get_attachment_image() and wp_image_add_srcset_and_sizes() both rely on _wp_calculate_image_srcset(). All former filter hooks remain in place.

The other notable change is that the order of parameters for wp_get_attachment_image_sizes() has changed so that $attachment_id can be an optional parameter. The order of the parameters in the wp_get_attachment_image_sizes filter have also been updated to reflect this change.

All tests have been updated accordingly.

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

#15 @azaozz
9 years ago

In 35412:

Responsive images:

  • Introduce wp_calculate_image_srcset() that replaces wp_get_attachment_image_srcset_array() and is used as lower level function for retrieving the srcset data as array.
  • Use the new function when generating srcset and sizes on the front-end. This is faster as no (other) image API functions are used.
  • Change the wp_get_attachment_image_srcset(). Now it is meant for use in templates and is no longer used in core.
  • A few logic fixes and improvements.
  • Some names changed to be (hopefully) more descriptive.
  • Fixed/updated tests.

Props joemcgill, jaspermdegroot, azaozz.
See #34430.

#16 follow-up: @DrewAPicture
9 years ago

  • Keywords needs-docs added

Following [35412], we need better descriptions than "Private, do not use." for _wp_upload_dir_baseurl() and _wp_get_image_size_from_meta(). We use these docs internally too, remember :-)

If we don't want it to be used outside core, e.g. "publicly", we should use @ignore in the DocBlocks to skip parsing, but that shouldn't preclude adequately documenting the functions including parameters and returns.

#17 in reply to: ↑ 16 ; follow-up: @swissspidy
9 years ago

Following [35412], we need better descriptions than "Private, do not use." for _wp_upload_dir_baseurl() and _wp_get_image_size_from_meta(). We use these docs internally too, remember :-)

That's what I thought too this morning, so 34430.docs.diff is a first pass at adding proper docs.

I wonder if we should make _wp_upload_dir_baseurl() public, as something similar was requested in #33963.

#18 @jaspermdegroot
9 years ago

Following [35412], in 34430.6.diff:

  • Various inline docs corrections and improvements
  • Correction of two filter names
  • Consistent type casting in the sizes function
  • Consistency in max_srcset_image_width filter and variable name
Version 0, edited 9 years ago by jaspermdegroot (next)

@jaspermdegroot
9 years ago

Replaces 34430.6.diff

#19 @jaspermdegroot
9 years ago

34430.7.diff contains all changes from 34430.6.diff plus:

  • A few more corrections in inline docs.
  • Check if wp_get_attachment_metadata() has returned an array: Users that allow SVGs in their WordPress installation reported errors in the RICG responsive images plugin repo (see https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/issues/208 for example) which can be prevented by checking if wp_get_attachment_metadata has returned an array.
  • Only add the srcset attribute if there will be a sizes attribute as well: We already did check if we can set both attributes in wp_image_add_srcset_and_sizes() but not in wp_get_attachment_image().

#20 @jaspermdegroot
9 years ago

@swissspidy - In your patch 34430.docs.diff on line 897 @return array|false should be @return array|bool

#21 in reply to: ↑ 17 @joemcgill
9 years ago

Replying to swissspidy:

I wonder if we should make _wp_upload_dir_baseurl() public, as something similar was requested in #33963.

I'm wondering the same thing. This function addresses the main issue brought up in #34359 so we also need to make sure the concerns mentioned in that ticket have been addressed. The approach @azaozz took for using a static variable inside _wp_upload_dir_baseurl() should allow us to avoid any collisions with persistent cache. I'm not sure about situations where a site is conditionally filtering the uploads directory or if that is even a concert in our context here.

#22 @azaozz
9 years ago

In 35419:

Responsive images:

  • Check if wp_get_attachment_metadata() has returned an array to prevent errors when using SVGs.
  • Only add the srcset attribute if there will be a sizes attribute.
  • Better filter names.
  • Some more inline docs fixes.

Props jaspermdegroot.
See #34430.

#23 @azaozz
9 years ago

In 35426:

Responsive images: add inline docs for private functions.

Props swissspidy.
See #34430.

#24 @jaspermdegroot
9 years ago

Following [35426], in 34430.8.diff:

  • A few more corrections in inline docs
  • Removed unwanted blank lines
  • Renamed a confusing variable name in wp_get_attachment_image_sizes

#25 @SergeyBiryukov
9 years ago

#34464 was marked as a duplicate.

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

@azaozz
9 years ago

#27 @azaozz
9 years ago

After today's chat, in 34430.9.diff:

  • Merge wp_image_srcset_attr() into wp_calculate_image_srcset().
  • Remove the wp_image_srcset filter. Keep the wp_calculate_image_srcset filter for the array containing the data for creating the srcset value.
  • Fix the tests to reflect these changes.

#28 @azaozz
9 years ago

In 35464:

Responsive images:

  • Merge wp_image_srcset_attr() into wp_calculate_image_srcset().
  • Remove the wp_image_srcset filter.
  • Fix the tests for the above changes.

See #34430.

#29 @azaozz
9 years ago

In 35465:

Responsive images: few more inline docs fixes.

Props jaspermdegroot.
See #34430.

#30 follow-up: @jaspermdegroot
9 years ago

Following [35465], in 34430.10.diff:

  • Correction in inline doc: The $attachment_id param of wp_get_attachment_image_srcset() is not optional.
  • Added a more comprehensive description of the $sources param of the wp_calculate_image_srcset filter.
  • In wp_get_attachment_image_srcset(): Check if the meta data array contains sizes instead of just checking if it's an array. Consistent with what we do in wp_image_add_srcset_and_sizes().
  • In wp_get_attachment_image_srcset(): Check if wp_get_attachment_metadata() has returned an array to prevent errors when using SVGs.
  • In wp_get_attachment_image_sizes(): call get_post_meta() directly like we do in wp_get_attachment_image() and wp_make_content_images_responsive().
  • In wp_get_attachment_image_sizes(): Removed the check if $size is a number since it only accepts an array.

#31 in reply to: ↑ 30 @azaozz
9 years ago

Replying to jaspermdegroot:

Think there is an inconsistency in wp_get_attachment_image_srcset() in 34430.10.diff. First we want to check if the $image_meta was passed (as it's optional), and get the meta if not. Then we want to see if $image_meta['sizes'] is not empty. Will fix that and commit.

#32 @azaozz
9 years ago

Actually not sure we need to check if $image_meta['sizes'] is not empty in both wp_get_attachment_image_srcset() and wp_calculate_image_srcset(). It doesn't make any difference in the former and is the first conditional in the latter.

Last edited 9 years ago by azaozz (previous) (diff)

#33 @azaozz
9 years ago

In 35491:

Responsive images:

  • More fixes to inline docs.
  • Replace the last wp_get_attachment_metadata() with get_post_meta().
  • For consistency only accept array or named size in wp_get_attachment_image_sizes().

Props jaspermdegroot.
See #34430.

@azaozz
9 years ago

#34 @azaozz
9 years ago

In 34430.11.diff:

  • Fix _wp_upload_dir_baseurl() to cache by blog_id.
  • Replace path_join() with trailingslashit(). We are joining URLs, there's no need for the extra checks in path_join() (and it's much slower).
  • Rename $image_url to $image_src for consistency (used at about 50 other places).
  • Couple of tests fixes.
Last edited 9 years ago by azaozz (previous) (diff)

#35 @azaozz
9 years ago

In 35498:

Responsive images:

  • Fix _wp_upload_dir_baseurl() to cache by blog_id.
  • Replace path_join() with trailingslashit(), it's much faster.
  • Rename $image_url to $image_src for consistency (used at about 50 other places).
  • Couple of tests fixes.

See #34430.

#36 @joemcgill
9 years ago

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

#37 @joemcgill
9 years ago

Once #34359 is resolved, we can open a new ticket for removing _wp_upload_dir_baseurl().

Note: See TracTickets for help on using tickets.