Bulk load Comment Meta upon access

[dd32]

When we query posts, we bulk load all of the posts meta and taxonomies at the time of request. Since adding comment meta, we load the comment meta on-the-fly since nothing really currently uses it in core.

I've added a filter to get_comment_text which modifies the comment content based on the value of a meta key.

The problem arises when multiple comments are displayed on a page, The metadata for every comment is loaded individually:

SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (4)
SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (5)
SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (6)
SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (7)
SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (34)
SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (35)
SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (37)

in all cases, the function stack is:

comments_template, wp_list_comments, Walker->paged_walk, Walker_Comment->display_element, Walker->display_element, call_user_func_array, Walker_Comment->start_el, call_user_func, twentyten_comment, comment_text, get_comment_text, apply_filters, call_user_func_array, MY_FUNCTION_HOOKED_TO_GET_COMMENT_TEXT, get_comment_meta, get_metadata, update_meta_cache #12 (0.7ms)

It isn't ideal that every comment causes an extra query, instead, we should bulk load comment meta for the comments being displayed.

However, Since comment meta is not always used, in order to keep the load down, I'd suggest that we only bulk load the comment meta on the first request for comment meta, this should allow current users to have no detrimental affect on query count/performance, whilst allowing those who use comment meta to manage each comment better performance.

[dd32]

Adding this to the code in question bulk loads the comment meta for all requests for comment meta (which is suitable for the site in question I'm working on):

	add_filter('the_comments', 'cache_all_comment_meta', 100); // using get_comments()
	add_filter('comments_array', 'cache_all_comment_meta', 100); // using Direct SQL in comments_template() 
	function cache_all_comment_meta($comments) {
		$comment_ids = wp_list_pluck($comments, 'comment_ID');
		update_meta_cache('comment', $comment_ids);
		return $comments;
	}
comments_template, apply_filters, call_user_func_array, cache_all_comment_meta, update_meta_cache #12 (0.7ms)
SELECT comment_id, meta_key, meta_value FROM wp_commentmeta WHERE comment_id IN (4,5,6,7,34,35,36,37)

[scribu]

A similar approach is being considered for post thumbnails: #15447

[dd32]

An implementation of lazy loading of the comment meta, which requires a bit of juggling due to the fact that this is being implemented as an external dropin.

class Lazy_Load_Comment_Meta {
	var $last_comment_query_ids = array();
	function __construct() {
		add_filter('get_comment_metadata', array(&$this, 'lazy_load_comment_metadata'), 100, 2);
		add_filter('the_comments', array(&$this, 'lazy_load_comment_metadata_fillids'), 100);
		add_filter('comments_array', array(&$this, 'lazy_load_comment_metadata_fillids'), 100);
	}

	function lazy_load_comment_metadata($data, $object_id) {
		if ( !wp_cache_get($object_id, 'comment_meta') ) {
			if ( !empty($this->last_comment_query_ids) )
				update_meta_cache('comment', $this->last_comment_query_ids );
		}
		return null;
	}
	function lazy_load_comment_metadata_fillids($comments) {
		foreach ( $comments as $c )
			$this->last_comment_query_ids[] = $c->comment_ID;
		return $comments;
	}
}

Stack:

Front: 
comments_template, wp_list_comments, Walker->paged_walk, Walker_Comment->display_element, Walker->display_element, call_user_func_array, Walker_Comment->start_el, call_user_func, twentyten_comment, comment_text, get_comment_text, apply_filters, call_user_func_array, secret_hooked_function, get_comment_meta, get_metadata, apply_filters, call_user_func_array, Lazy_Load_Comment_Meta->lazy_load_comment_metadata, update_meta_cache

Back:
WP_Comments_List_Table->display, WP_List_Table->display_rows_or_placeholder, WP_List_Table->display_rows, WP_Comments_List_Table->single_row, WP_List_Table->single_row_columns, call_user_func, WP_Comments_List_Table->column_comment, comment_text, get_comment_text, apply_filters, call_user_func_array, secret_hooked_function, get_comment_meta, get_metadata, apply_filters, call_user_func_array, Lazy_Load_Comment_Meta->lazy_load_comment_metadata, update_meta_cache

[nacin]

I would love this.

[boonebgorges]

See update_postmeta_cache() for a how-to.

[bradt]

Taking a crack at this now.

[bradt]

Just tried applying this patch against latest trunk and it still applies cleanly.

[bradt]

Still applies cleanly against trunk. Just tried again.

[bradt]

Still applies cleanly against trunk. Just tried again.

[bradt]

Just re-tested, patch still applies cleanly.

[wonderboymusic]

@dd32 - do you want to weigh in on 16894.2.diff​ (which refreshes the previous) ?

[dd32]

@wonderboymusic I'm not wild about a flag on $wp_query, but it does seem to be best option all things considered.

[wonderboymusic]

The metadata funcs shouldn't be altered, and the use of globals is bad. 16894.3.diff​ simplifies all of this.

[wonderboymusic]

In 33925:

In wp_list_comments(), update the comment meta cache when the comments derive from WP_Query and the new ->comment_meta_cached prop is false.

There are no uses of wp_list_comments() in Core where $comments are passed as the 2nd argument.

Adds unit tests.

Props wonderboymusic, bradt.
Fixes #16894.

[wonderboymusic]

In 33926:

After [33925], rename the unit test class to Tests_Comment_Meta_Cache to avoid collisions on the PHP 5.2 superhighway.

See #16894.

[dd32]

Since the committed direction is different to the proposed solutions..

• r33925 updates metadata caches upon comment listing, not upon comment meta access. If you're using anything other than wp_list_comments() you need to call update_meta_cache() manually.

$comment_meta_cached no longer really seems useful with this approach, a lot has changed with comment caches since this ticket was created.

• WP_Comment_Query calls update_comment_cache() which only updates the comment cache, no meta caches
• WP_Comment_Query doesn't trigger a metadata cache update
• wp_list_comments() triggers a comment metadata update for all comments on all sites even if the site has no custom comment metadata usage

So, I'd propose that maybe update_comment_cache() should be used here, which should also handle comment meta, No flags, and review any performance impacts always loading the cache will have.

[wonderboymusic]

Yeah, there's a lot going on. The WP_Query flag is only useful in the loop. The patch on the ticket essentially made this so by checking the global, which is only set in wp_list_comments(), which only works if comments are passed to it (they never are in core) or is $wp_query has the ->comments prop populated, which only happens when comments_template() is called. WP_Query doesn't eager-load comments, it is decorated by comments_template(). That's why I quarantined the code to wp_list_comments().

I'm not sure any of this needed without a better overall strategy.

[wonderboymusic]

In 34245:

Revert [33925], by-reference array manipulation is breaking comments in some themes.

This implementation is losing its shine.

See #16894.

[boonebgorges]

16894.4.diff​ is a naive implementation of dd32's suggestion. No lazy-loading - all comment meta is primed during WP_Comment_Query::query(), configurable with the 'update_comment_meta_cache' flag.

Lazy-loading is obviously going to be more complicated, and we don't have good systems in place for it anywhere else in core. My inclination would be to go with the naive implementation in 4.diff for general use of WP_Comment_Query. Then we could introduce lazy-loading as a progressive enhancement in places where we already have a container for keeping track of global state, like in WP_Query (which doesn't use WP_Comment_Query anyway). See 16894.5.diff​ for an implementation that I think is in the spirit of what dd32 originally suggested.

I'm not sure I understand the "by-reference array manipulation" issue wonderboymusic refers to in [34245]. Is it just because of how wp_list_pluck() works? I skipped using it in 16894.5.diff​.

Either way, let's clean this up, because builds are failing due to the update_comment_cache unit test.

[wonderboymusic]

+1

wp_list_pluck() alters the array you pass to it, doesn’t modify a copy - it borked for me, because I was passing $wp_query->comments. Bugs like these are a jazz to discover/realize

[boonebgorges]

In 34268:

Prime comment meta caches in WP_Comment_Query.

The new 'update_comment_meta_cache' parameter, which defaults to true, can
be used to disable this behavior.

update_comment_cache() has been updated to support an $update_meta_cache
parameter, which also updates to true; this matches the pattern we use for
priming post caches.

See #16894.

[boonebgorges]

[34268] turns on commentmeta priming by default. For the purposes of viewing comments on a single page, via comments_template() - which is the most common use of comments, and is the most likely to cause performance concerns - I'm going to introduce lazy-loading.

A note that I'm not going to set a $wp_query flag. update_meta_cache() should be idempotent for the same set of comment IDs; the function bails early if all objects have already had their metadata loaded into cache. If we find that calling update_meta_cache() more times than strictly necessary is causing real-life performance issues, let's introduce a comment_meta_updated flag or something like that.

[boonebgorges]

In 34270:

Lazy-load comment meta on single post pages.

[34268] introduced cache priming for commentmeta, enabled by default. To
ensure performance on single post pages - where commentmeta is most likely
to cause performance issues - we disable up-front cache-priming. Instead, we
prime commentmeta caches for all comments in the loop the first time
get_comment_meta() is called on the page.

Props bradt, dd32, wonderboymusic, boonebgorges.
Fixes #16894.

[DrewAPicture]

Very cool. @boonebgorges, nice work!