WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 15 months ago

Last modified 15 months ago

#16894 closed defect (bug) (fixed)

Bulk load Comment Meta upon access

Reported by: dd32 Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.1
Component: Comments Keywords: has-patch 2nd-opinion
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 (5)

16894.diff (1.6 KB) - added by bradt 2 years ago.
16894.2.diff (1.6 KB) - added by wonderboymusic 15 months ago.
16894.3.diff (1.0 KB) - added by wonderboymusic 15 months ago.
16894.4.diff (2.2 KB) - added by boonebgorges 15 months ago.
16894.5.diff (1.7 KB) - added by boonebgorges 15 months ago.

Download all attachments as: .zip

Change History (34)

#1 @dd32
6 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)
Version 0, edited 6 years ago by dd32 (next)

#2 @scribu
6 years ago

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

#3 @dd32
6 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

#4 @nacin
3 years ago

  • Component changed from Performance to Comments
  • Focuses performance added

I would love this.

#5 @boonebgorges
2 years ago

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

See update_postmeta_cache() for a how-to.

Last edited 2 years ago by boonebgorges (previous) (diff)

#6 @bradt
2 years ago

Taking a crack at this now.

@bradt
2 years ago

#7 @bradt
2 years ago

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

#8 @DrewAPicture
2 years ago

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

#9 @bradt
22 months ago

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

#10 @bradt
21 months ago

Still applies cleanly against trunk. Just tried again.

#11 @bradt
20 months ago

Still applies cleanly against trunk. Just tried again.

#12 @bradt
16 months ago

Just re-tested, patch still applies cleanly.

#13 @wonderboymusic
15 months ago

  • Keywords good-first-bug needs-testing removed
  • Milestone changed from Future Release to 4.4

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

#14 @dd32
15 months ago

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

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


15 months ago

#16 @SergeyBiryukov
15 months ago

  • Keywords commit added

#17 @wonderboymusic
15 months ago

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

#18 @wonderboymusic
15 months ago

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

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.

#19 @wonderboymusic
15 months ago

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.

#20 @dd32
15 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#21 @wonderboymusic
15 months ago

  • Keywords commit removed

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.

#22 @wonderboymusic
15 months ago

In 34245:

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

This implementation is losing its shine.

See #16894.

#23 @boonebgorges
15 months ago

  • Keywords 2nd-opinion added

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.

#24 @wonderboymusic
15 months ago

+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

#25 @wonderboymusic
15 months ago

  • Owner changed from bradt to boonebgorges
  • Status changed from reopened to assigned

#26 @boonebgorges
15 months ago

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.

#27 @boonebgorges
15 months ago

[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.

#28 @boonebgorges
15 months ago

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

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.

#29 @DrewAPicture
15 months ago

Very cool. @boonebgorges, nice work!

Note: See TracTickets for help on using tickets.