Split up `wp_lazyload_term_meta()`; check `$check` before lazyloading

[dlh]

The attached patch proposes a few adjustments to wp_lazyload_term_meta():

1. Move most of the logic in wp_lazyload_term_meta() into a function that can be called with any WP_Query. The new function could be used by queries that aren't going to become the global $wp_query. It would also be available to developers even if they wanted to unhook wp_lazyload_term_meta() itself.

2. Don't do anything in wp_lazyload_term_meta() if $check is non-null. This change would come out of the argument that a developer who is short-circuiting get_metadata() wouldn't want to update_termmeta_cache() either.

3. With (2), decrease the wp_lazyload_term_meta() hook priority to allow more filters to affect $check first.

4. Try to fix a few docs typos.

[boonebgorges]

Yes, this is a good idea.

My first instinct when writing this lazyloading was to make it a method of WP_Query. Then it's automatically portable to other WP_Query instances. The only part of it that would be funky is that you'd have to have an add_action() inside of WP_Query, after the main query had been run. Anyway, this makes the lazyloading automatic for all query objects, and removes a bunch of the checks done in the original implementation ($wp_query instanceof WP_Query), etc. See 34047.diff​.

Does this make sense to you?

If so, let's do the same thing for commentmeta lazy loading.

[dlh]

Thanks @boonebgorges. I had some questions and comments about this approach, if I'm following it correctly.

• Every WP_Query instance that hooks into get_term_metadata() would now keep calling lazyload_term_meta() during the action, as opposed to just the one instance currently. Would that be a performance risk or load unrelated meta?

• You would need the object to remove the action. If I were using get_posts() or filtering the arguments for a new WP_Query, then I couldn't affect lazyloading unless I disabled update_post_term_cache. But maybe the performance gain is worth it?

• You couldn't call lazyload_term_meta() without mimicking the get_term_metadata() filter arguments. Depending on the construction, it also might not be true there are // No extra queries. Term cache should already be primed by 'update_post_term_cache'.

• Would you want to return $check, not return, to avoid potentially altering $check?

[boonebgorges]

dlh - Thanks for the good questions. A slightly modified approach is in 34047.2.diff​.

Every WP_Query instance that hooks into get_term_metadata() would now keep calling lazyload_term_meta() during the action, as opposed to just the one instance currently. Would that be a performance risk or load unrelated meta?

You would need the object to remove the action

The new technique has the method unhook itself after it's been run once. No need to do it more than once for a given set of posts. (In fact, if you do it more than once, it won't hurt anything - update_meta_cache() does nothing if it finds that the caches are already primed - but removing the filter does reduce CPU cycles a bit, and seems cleaner.)

If I were using get_posts() or filtering the arguments for a new WP_Query, then I couldn't affect lazyloading unless I disabled update_post_term_cache. But maybe the performance gain is worth it?

It's hard to imagine a situation where you'd want to disable lazyloading. I guess if you were querying for hundreds of posts, but knew that you were only going to need the term meta for a small percentage of them, you might prefer to prime the termmeta cache only for those terms. As you note, it'd still be possible to do this by (a) disabling 'update_post_term_cache' (and presumably doing your own version of it) and then (b) running your own termmeta cache priming.

As a rule, I think that unhookability is a pretty powerful argument against using object methods as add_filter() callbacks. But this case seems like an exception to me: we're only using the filter API because of a technicality regarding the way that get_metadata() works, and IMO there are exceedingly few realistic cases where one would want to change the behavior. So I don't mind the hooking method being used, despite my general distaste for the technique. But maybe I'm missing something.

You couldn't call lazyload_term_meta() without mimicking the get_term_metadata() filter arguments.

Because lazyload_term_meta() is called by default, you never need to call it directly.

Depending on the construction, it also might not be true there are No extra queries. Term cache should already be primed by 'update_post_term_cache'.

It'll never be the case that lazyload_term_meta() causes extra queries, but you're right that the term cache wouldn't be primed if you called the method in a different context. I added a note to the documentation to clarify this a bit.

If you (or anyone else) think that the technique is too cute or too problematic, I'm not married to it. I just don't really care for passing WP_Query objects to functions.

[dlh]

It's hard to imagine a situation where you'd want to disable lazyloading.

The only other situation I had in mind is where I knew I didn't need the meta at all for my particular query -- rare as that might be, true enough.

In 34047.2, it still happens that one get_term_meta() call is enough to fire get_term_metadata and prime every existing query's cache that needs it. With that in mind, what about stepping back a bit for something like:

• Add update_term_meta_cache to the $args array in WP_Query::parse_query(). If true, does the same priming as the other patches here.

• If update_term_meta_cache is true, require update_post_term_cache to also be true.

I'm not sure whether the argument would default to true. If it did, then of course term meta would always be loaded, earlier than they would be on the get_term_metadata hook.

But assuming it wouldn't take very long for get_term_meta() calls to start appearing in the wild, am I following correctly to say the difference would be roughly a wash? Is that assumption even a safe one?

In any case, I think my main motivation for opening this ticket is being addressed regardless, so thanks again. I'll be interested to hear from others as well and will help how I can from here.

[boonebgorges]

Thanks, dlh! This discussion is helping me to clarify what I think is the best way forward.

The only other situation I had in mind is where I knew I didn't need the meta at all for my particular query -- rare as that might be, true enough.

The "lazy" part of the lazyload means that nothing is queried until it's first used. That is: if you never call get_term_meta() within the loop, then no queries against wp_termmeta will ever take place. So no overhead is added at all.

In 34047.2, it still happens that one get_term_meta() call is enough to fire get_term_metadata and prime every existing query's cache that needs it.

Yeah, I see what you mean. Say you have code like the following:

wp_set_object_terms( 1, 'Foo', 'post_tag' );
wp_set_object_terms( 2, 'Bar', 'post_tag' );
wp_set_object_terms( 3, 'Baz', 'post_tag' );
wp_set_object_terms( 4, 'Quz', 'post_tag' );
$q1 = new WP_Query( array( 'include' => array( 1, 2 ) ) );
$q2 = new WP_Query( array( 'include' => array( 3, 4 ) ) );

At this point, the first time you call get_term_meta(), termmeta for both $q1 and $q2 is going to be loaded - that is, all the metadata belonging to 'Foo', 'Bar', 'Baz', and 'Quz'.

I suppose one way to do somewhat more targeted lazyloading is to bail if the $term_id passed to get_term_meta() doesn't belong to any of the posts in the loop. This is not foolproof: if Quz belongs to post 1 as well as post 3, then get_term_meta( $quz_id, 'foo' ) would prime the caches for *both* loops. But maybe it's better than nothing.

Add update_term_meta_cache to the $args array in WP_Query::parse_query(). If true, does the same priming as the other patches here.

I'm leaning against doing this. 'update_post_term_cache' means: after querying posts, prime the term cache for each post immediately. That action invokes a not-insignificant amount of overhead. What we're talking about here is more subtle: after querying posts, we do *not* prime termmeta cache; we set up lazyloading, so that the data is loaded only when we need it. This introduces basically zero overhead, and I'm not sure it's something that you need to be able to turn off (and, as noted, you can do it if you really need to).

As a side note, I think that the lazyloading technique we're talking about here is, in most ways, preferable to the 'update_post_meta_cache' and 'update_post_term_cache' techniques that WP_Query currently uses. If it works out well for termmeta and commentmeta, we should consider migrating other cache priming to the lazyload model in the future.

[boonebgorges]

In 34704:

Improve lazyloading of term metadata in WP_Query loops.

[34529] introduced lazyloading for the metadata belonging to terms matching
posts in the main WP_Query. The current changeset improves this technique
in the following ways:

• Term meta lazyloading is now performed on the results of all WP_Query queries, not just the main query.
• Fewer global variable touches and greater encapsulation.
• The logic for looping through posts to identify terms is now only performed once per WP_Query.

Props dlh, boonebgorges.
See #34047.

[boonebgorges]

In 34711:

Improve lazyloading of comment meta in WP_Query loops.

Lazy-loading logic is moved to a method on WP_Query. This makes it possible
for comment feeds to take advantage of metadata lazyloading, in addition to
comments loaded via comments_template().

This new technique parallels the termmeta lazyloading technique introduced in
[34704].

Fixes #34047.

[dlh]

34047.3.diff​ has a few copy-edits to [34704] and [34711]:

• add_filter() instead of _action
• get_(term|comment)_metadata, not pre_
• Use mixed instead of null in both the @param and @return docs for $check

Apologies if I've misunderstood anything in the original commits.

[boonebgorges]

Looks good - thanks, dlh. (We're pretty much using the filters like actions, which I think is why I made the error!)

[boonebgorges]

In 34731:

s/add_action()/add_filter() in WP_Query metadata lazyloading.

Props dlh.
See #34047.

[boonebgorges]

In 34732:

Correct some WP_Query metadata lazyloading docs.

Props dlh.
Fixes #34047.