Make WordPress Core

Opened 13 years ago

Closed 12 years ago

Last modified 11 years ago

#15447 closed enhancement (fixed)

Cache Post Thumbnails

Reported by: thedeadmedic's profile TheDeadMedic Owned by: nacin's profile nacin
Milestone: 3.2 Priority: normal
Severity: normal Version:
Component: Performance Keywords: has-patch
Focuses: Cc:

Description

Database queries can rapidly increase when using the post thumbnails feature, especially whilst in the loop (on average, 2 additional queries per post).

Suggest caching all thumbnails for the current query - will usually drop to just two additional DB queries in total, regardless of number of posts.

// only run if feature supported by theme

$thumbnail_IDs = array();
foreach ($posts as $post)
	$thumbnail_IDs[] = get_post_thumbnail_id($post->ID);

if ($thumbnail_IDs) {
	$thumbnail_IDs = array_filter( array_unique($thumbnail_IDs) );
	get_posts(array(
		'update_post_term_cache' => false,
		'include' => $thumbnail_IDs,
		'post_type' => 'attachment',
		'post_status' => 'inherit',
		'nopaging' => true
	));
}

Obviously the code above is pure example - when, and where, this is best placed I'm not sure. Perhaps hooked to the 'wp' action, so it only runs for the main query?

Attachments (6)

15447.diff (1.8 KB) - added by garyc40 12 years ago.
cache post thumbnails on first call of get_the_post_thumbnail() in a loop
15447.2.diff (2.0 KB) - added by scribu 12 years ago.
Check $wp_query->current_post before attempting to cache
15447.3.diff (1.7 KB) - added by scribu 12 years ago.
Remove extra caching key
15447.4.diff (1.5 KB) - added by scribu 12 years ago.
Use $wp_query->_thumbnails_cached flag
15447.5.diff (1.7 KB) - added by garyc40 12 years ago.
prevent notice for $wp_query->_thumbnails_cached
15447.6.diff (1.9 KB) - added by greuben 12 years ago.

Download all attachments as: .zip

Change History (41)

#1 @scribu
13 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from feature request to enhancement

#2 @nacin
13 years ago

  • Component changed from General to Post Thumbnails
  • Keywords reporter-feedback added

I don't see how thumbnails results in more queries. We do a full slurp of metadata for posts in the loop, which would include those IDs. Can you post the queries? Is it querying for attachment data? That's not unheard of for, say, galleries.

#3 @scribu
13 years ago

Of course it's querying for attachment data, since it needs the thumbnail url, at least.

#4 @johnbillion
13 years ago

I've written a plugin which does exactly this. On a large site I'm working on with 14 (yeah, fourteen) loops on the home page and thumbnails *everywhere* the number of queries was through the roof.

Here's the relevant code which preloads attachments for all posts in the main loop (my custom loops also call the 'loop_start' action - usually this is not the case).

function cache_thumbnail_posts( $q ) {
	$types = $thumb_ids = array();
	foreach ( $q->posts as $p )
		$types[$p->post_type] = $p->post_type;
	unset( $types['attachment'] );
	if ( empty( $types ) )
		return $q;
	foreach ( $q->posts as $p )
		$thumb_ids[] = get_post_thumbnail_id( $p->ID );
	$thumb_ids = array_filter( $thumb_ids );
	if ( empty( $thumb_ids ) )
		return $q;
	$thumb_ids = implode( ',', $thumb_ids );
	$preload = get_posts( 'post_type=attachment&include=' . $thumb_ids );
	return $q;
}
add_filter( 'loop_start', array( $this, 'cache_thumbnail_posts' ) );

Looks a bit complex but it means it only preloads the attachments when it needs to.

One thing to consider is that this will add unnecessary queries if the current theme isn't displaying the post thumbnails in the loop.

Thoughts?

#5 @scribu
13 years ago

  • Keywords 3.2-early added; reporter-feedback removed

We could add a cache_post_thumbnails() method to WP_Query, which theme authors could call before displaying a Loop with thumbs.

#6 @johnbillion
13 years ago

Or how about add_theme_support('preload-thumbnails')?

#7 @scribu
13 years ago

That's no good, because this has to be set per loop, not per theme.

#8 @nacin
13 years ago

It's one query, so why not add it when the theme supports thumbails? Or cache if in_the_loop on first call to a post thumbnail template tag?

Keep it simple. Theme authors shouldn't be given extra tags just for caching.

#9 follow-up: @scribu
13 years ago

Or cache if in_the_loop on first call to a post thumbnail template tag?

I like that idea.

#10 in reply to: ↑ 9 @johnbillion
13 years ago

Replying to scribu:

Or cache if in_the_loop on first call to a post thumbnail template tag?

I like that idea.

Same here. That's the way to go. An extra bit of logic so it doesn't fire when is_single etc and that'd be spot on.

#11 @nacin
13 years ago

Well, we'd need to run the query anyway, so in that case it'd only be run for one post, versus all of them. We'd just do it for all posts in $wp_query->posts.

#12 follow-up: @TheDeadMedic
13 years ago

Perhaps caching should also depend on the query arg 'cache_results', and possibly a new query arg 'update_post_thumbnail_cache' (to match the pattern of the existing 'update_post_term_cache' and 'update_post_meta_cache' args)?

So by default (if thumbnail feature supported), thumbnails are cached for the main query (i.e. the request), but a theme author has the option to cache/not cache for their own loop(s)?

#13 @garyc40
12 years ago

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

@garyc40
12 years ago

cache post thumbnails on first call of get_the_post_thumbnail() in a loop

#14 in reply to: ↑ 12 ; follow-up: @garyc40
12 years ago

Replying to TheDeadMedic:

Perhaps caching should also depend on the query arg 'cache_results', and possibly a new query arg 'update_post_thumbnail_cache' (to match the pattern of the existing 'update_post_term_cache' and 'update_post_meta_cache' args)?

So by default (if thumbnail feature supported), thumbnails are cached for the main query (i.e. the request), but a theme author has the option to cache/not cache for their own loop(s)?

I don't think that's a good idea. There are loops that don't output thumbnails at all, so caching thumbnails whenever a loop start seems wasteful.

Ignore attachment:15447.diff for the time being, I just realized there's a bug with it (using permanent cache and modifying the thumbnail of a post does not properly invalidate the cache).

Any suggestion on how to tell if get_post_thumbnail() is being called for the first time in a loop?

Last edited 12 years ago by garyc40 (previous) (diff)

#15 in reply to: ↑ 14 @garyc40
12 years ago

  • Keywords has-patch added; needs-patch removed

Ignore attachment:15447.diff for the time being, I just realized there's a bug with it (using permanent cache and modifying the thumbnail of a post does not properly invalidate the cache).

Any suggestion on how to tell if get_post_thumbnail() is being called for the first time in a loop?

Never mind this. attachment:15447.diff works.

Still wondering if there's a better way to tell if get_post_thumbnail() is being called for the first time in a loop.

@scribu
12 years ago

Check $wp_query->current_post before attempting to cache

#16 follow-ups: @scribu
12 years ago

You can check $wp_query->current_post. See 15447.2.diff

With that in place, I doubt the usefulness of permanently caching wether the current loops thumbnails were cached or not.

@scribu
12 years ago

Remove extra caching key

#17 @scribu
12 years ago

15447.3.diff removes the extra caching key.

Also, it removes unnecessary array_filter( array_unique( $thumb_ids ) )

#18 @scribu
12 years ago

All of the update_*_cache() functions would more aptly be named populate_*_cache(), but I guess that's for another ticket.

#19 in reply to: ↑ 16 @garyc40
12 years ago

Replying to scribu:

You can check $wp_query->current_post. See 15447.2.diff

With that in place, I doubt the usefulness of permanently caching wether the current loops thumbnails were cached or not.

This assumes that get_the_post_thumbnail() is always called at the loop start. Unless there are themes that treat the first post differently (not outputting the thumbnail, for example), it's a safe assumption.

#20 in reply to: ↑ 16 @garyc40
12 years ago

Replying to scribu:

With that in place, I doubt the usefulness of permanently caching wether the current loops thumbnails were cached or not.

It might be useful when you're using a permanent cache plugin. The same loop might be requested multiple times on a busy site, without another cache key in place, there will be unnecessary db queries for attachments in update_thumbnail_cache() (the get_posts() call).

#21 @scribu
12 years ago

By that logic, we should also cache WP_Query itself, since it involves even more work, but we don't.

I think reducing N queries to 1 is good enough.

#22 @nacin
12 years ago

15447.3.diff is what I was picturing when I commented a few months ago.

Why can it only fire on the first post? I imagine it should be able to fire at any point, once per loop. Keep in mind subsequent calls to it would simply hit the object cache anyway, but it shouldn't be difficult to ensure it gets fired only once per loop.

@scribu
12 years ago

Use $wp_query->_thumbnails_cached flag

#23 follow-up: @scribu
12 years ago

15447.4.diff is similar to 15447.diff, except it uses a private property on the $wp_query object instead of wp_cache_*().

This ensures a single call per loop, while not requiring get_the_post_thumbnail() to be called for the first post.

@garyc40
12 years ago

prevent notice for $wp_query->_thumbnails_cached

#25 in reply to: ↑ 23 @garyc40
12 years ago

Replying to scribu:

15447.4.diff is similar to 15447.diff, except it uses a private property on the $wp_query object instead of wp_cache_*().

Attached another patch that prevent undefined property notice for 15447.4.diff :)

#26 @scribu
12 years ago

Cool. What would be nice would be to allow update_thumbnail_cache() to be used for custom loops too.

I'm thinking of adding an optional $_wp_query parameter. Then, knowledgeable developers could call it before rolling their custom loop.

#27 @scribu
12 years ago

Similar: #16894

#28 @scribu
12 years ago

  • Component changed from Post Thumbnails to Performance
  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.2

#29 @greuben
12 years ago

  • Keywords needs-refresh added

15447.5.diff is not working( It should be ! empty instead of empty ). However, isn't that variable supposed to be defined in WP_Query class?. If that property is private aren't we supposed to use functions like set and get?

#30 @ryan
12 years ago

  • Milestone changed from 3.2 to Future Release

#31 @nacin
12 years ago

  • Milestone changed from Future Release to 3.2

If someone can refresh this patch in the next day or so, it'll go into 3.2. This is a good enhancement.

#32 @nacin
12 years ago

  • Owner changed from garyc40 to nacin
  • Status changed from assigned to accepted

@greuben
12 years ago

#33 @greuben
12 years ago

  • Keywords needs-refresh removed

Patch refreshed

#34 @nacin
12 years ago

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

In [17883]:

Cache post thumbnails in the loop. props garyc40, scribu, greuben. fixes #15447.

#35 @scribu
11 years ago

Follow-up: #19949

Note: See TracTickets for help on using tickets.