Opened 5 years ago
Last modified 6 weeks ago
#49446 reviewing enhancement
cache the results of get_uploaded_header_images()
Reported by: | pbiron | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Themes | Keywords: | has-patch has-unit-tests changes-requested |
Focuses: | administration, performance | Cc: |
Description
If the current theme supports custom-header
and is_random_header_image()
is true, then _media_states()
calls get_uploaded_header_images()
for every row in the media list table.
If the results of get_uploaded_header_images()
were cached, the media list table would render much quicker, especially if the number of media displayed per page is large.
There is already an inline comment @todo Caching
which has apparently been there since get_uploaded_header_images()
was added in [17757].
Attachments (2)
Change History (32)
This ticket was mentioned in Slack in #core-media by pbiron. View the logs.
5 years ago
#2
in reply to:
↑ description
;
follow-up:
↓ 3
@
5 years ago
#3
in reply to:
↑ 2
@
5 years ago
Replying to SergeyBiryukov:
Even with caching added, saving
$header_images
to a static variable in_media_states()
seems like a quick win to avoid having to go throughwp_list_pluck()
for the result ofget_uploaded_header_images()
for every row.
Agreed...that's kind how I stumbled on this. I've got a custom plugin that does a relatively complex query hooked to display_media_states
and storing the results in a static
sped things up considerably.
I went looking for more improvements and was going to suggest exactly the patch you added...then found that inline comment about caching in get_uploaded_header_images()
:-)
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
4 years ago
#6
@
4 years ago
49446.2.diff adds a missing @staticvar
entry.
#7
follow-up:
↓ 8
@
4 years ago
I wasn't aware of the @staticvar
tag, so I went to PHP Documentation Standards handbook page and don't see it listed as one of the tags (altho a quick search thru the core source, I see ~100 uses).
The handbook page does have an entry for @static
that states:
Note: This tag has been used in the past, but should no longer be used.
Just using the static keyword in your code is enough for PhpDocumentor on PHP5 to recognize static variables and methods, and PhpDocumentor will mark them as static
Should the handbook be updated to mention @staticvar
?
p.s. is this ready to commit now?
#8
in reply to:
↑ 7
;
follow-up:
↓ 10
@
4 years ago
- Keywords has-patch commit added
Replying to pbiron:
I wasn't aware of the
@staticvar
tag, so I went to PHP Documentation Standards handbook page and don't see it listed as one of the tags (altho a quick search thru the core source, I see ~100 uses).
...
Should the handbook be updated to mention@staticvar
?
Yes, it's used in core but for some reason not mentioned in the handbook.
Note that it's different from the @static
tag that was removed in #42803.
p.s. is this ready to commit now?
I think so :)
This ticket was mentioned in Slack in #docs by pbiron. View the logs.
4 years ago
#10
in reply to:
↑ 8
@
4 years ago
Replying to SergeyBiryukov:
Yes, it's used in core but for some reason not mentioned in the handbook.
It turned out the @staticvar
tag should not be used :) Follow-up: #50426.
So 49446.diff would be the preferred patch here.
#13
@
4 years ago
- Keywords commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
The commit message in [48152] is misleading, it provides a performance improvement for _media_states()
, but does not actually add caching to get_uploaded_header_images()
.
I believe there's still benefit in doing the latter, as it's used in some other places.
#16
@
3 years ago
- Owner desrosj deleted
- Status changed from reopened to assigned
Not going to be able to work on this near term. Removing myself as owner so other interested contributors do not hesitate to take this on.
This ticket was mentioned in PR #7154 on WordPress/wordpress-develop by @pbearne.
3 months ago
#17
- Keywords has-unit-tests added
Previously, the get_uploaded_header_images
function did not utilize caching, leading to potential performance issues. This commit introduces transient caching to store the header images, significantly improving efficiency. A corresponding PHPUnit test has also been added to verify the caching functionality.
#18
@
3 months ago
- Milestone changed from Future Release to 6.7
Add caching for this function it looks like it save 4 DB calls
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
3 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
3 months ago
@mukesh27 commented on PR #7154:
3 months ago
#21
@pbearne some unit tests are failing. Could you please take a look?
@mukesh27 commented on PR #7154:
2 months ago
#23
@SergeyBiryukov or @desrosj could one of you please take a look when you have moement. Thanks!
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
2 months ago
#26
@
2 months ago
Just left a review on the PR. It looks good so far in terms of caching logic, but we'll need to also take care of invalidation. This is not something where we should only rely on expiration of the transient, as that would still mean we'd sometimes serve stale values for something that is user-editable.
2 months ago
#27
@pbearne This looks solid in terms of caching logic. However, we also need to make sure that this is invalid when relevant attachments are uploaded/assigned/modified.
Can you implement additional hook callbacks here that delete the transient whenever an attachment is created or assigned to the active theme (via the
_wp_attachment_is_custom_header
meta key/value), or when such a an attachment is edited?
Have add cache invalidation for the meta I belive that this will also cover if it is edited as this meta will be updated as part of the process
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
7 weeks ago
@flixos90 commented on PR #7154:
6 weeks ago
#29
@pbearne Given that 6.7 Beta 1 is tomorrow, we'll have to punt this to 6.8. There's still two outstanding issues here:
- The cache invalidation needs to cater for when the attachment that is currently set as
_wp_attachment_is_custom_header
gets edited. See https://github.com/WordPress/wordpress-develop/pull/7154#pullrequestreview-2323622986. - The tests are failing when using a persistent object cache.
Replying to pbiron:
Even with caching added, saving
$header_images
to a static variable in_media_states()
seems like a quick win to avoid having to go throughwp_list_pluck()
for the result ofget_uploaded_header_images()
for every row.