WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 12 days ago

#16894 assigned defect (bug)

Bulk load Comment Meta upon access

Reported by: dd32 Owned by: bradt
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Comments Keywords: good-first-bug has-patch needs-testing
Focuses: performance Cc:

Description

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.

Attachments (1)

16894.diff (1.6 KB) - added by bradt 3 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 @dd324 years ago

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)
Last edited 4 years ago by dd32 (previous) (diff)

comment:2 @scribu4 years ago

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

comment:3 @dd324 years ago

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

comment:4 @nacin13 months ago

  • Component changed from Performance to Comments
  • Focuses performance added

I would love this.

comment:5 @boonebgorges4 months ago

  • Keywords good-first-bug added; 2nd-opinion removed

See update_postmeta_cache() for a how-to.

Last edited 4 months ago by boonebgorges (previous) (diff)

comment:6 @bradt3 months ago

Taking a crack at this now.

@bradt3 months ago

comment:7 @bradt3 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:8 @DrewAPicture2 months ago

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

comment:9 @bradt12 days ago

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

Note: See TracTickets for help on using tickets.