Add "after_get_posts" action to `WP_Query::get_posts()`

[stevegrunwell]

When troubleshooting a long-running WP-CLI script on ​ElasticPress, @lpawlik discovered an issue within WP_Query::get_posts() that can have major implications on memory consumption.

In a nutshell, when update_post_meta_cache and/or update_post_term_cache aren't explicitly set to false (for instance, if the query is called by way of get_posts()), then we end up hooking callbacks like this:

if ( $q['update_post_term_cache'] ) {
  add_filter( 'get_term_metadata', array( $this, 'lazyload_term_meta' ), 10, 2 );
}

On a normal web request, this isn't a huge deal, but this can become catastrophic memory-wise for long running scripts: the fact that the lazyload_term_meta() method is being hooked for each WP_Query instance means that PHP's garbage collector can't clean up old query objects (since they're still technically in-use).

Again, this often isn't an issue for any but the most intensive web requests, as we're usually not iterating through tens of thousands of posts and, when we do, the garbage collector automatically runs when the script terminates. However, for teams doing large-scale migrations and heavier data processing, it would be great to have a way to explicitly unset callbacks unintentionally set on hooks like "get_term_metadata".

I propose adding an "after_get_posts" action, called right at the end of WP_Query::get_posts() that acts as the sunset to pre_get_posts sunrise. The WP_Query instance would be passed by reference, and teams that do need to worry about garbage collection could use something like this:

/**
 * Remove WP_Query hooks on get_term_metadata, as they prevent PHP's garbage collection from
 * cleaning up old instances.
 *
 * @param WP_Query $query The WP_Query instance (passed by reference).
 */
function my_cli_script_unset_get_term_metadata( $query ) {
	remove_filter( 'get_term_metadata', array( $query, 'lazyload_term_meta' ) );
}
add_action( 'after_get_posts', 'my_cli_script_unset_get_term_metadata' );

[stevegrunwell]

Patch adding after_get_posts action, written in collaboration with @lpawlik

[boonebgorges]

@stevegrunwell Thanks very much for the detailed request.

Related: #35454

A new action here shouldn't be a problem.

That said, it would be nice to avoid the "catastrophic" consequences to begin with. You mention that the overhead is being caused by PHP keeping the WP_Query objects in memory. Is this conjecture, or is your profiling telling you this? I ask because there are a number of potential resource-hogs here: call_user_func() in apply_filters(); excessive object cache access due to race conditions (see https://core.trac.wordpress.org/ticket/35454#comment:3); etc.

The ideal solution is to remove_filter( 'get_term_metadata', array( $this, 'lazyload_term_meta' ), 10, 2 ); after the callback takes place the first time. As you probably saw in the source code, I noted when the lazyload functionality was introduced that it was impossible to remove_filter() because _wp_filter_build_unique_id() was not reliably returning unique IDs for similar objects; as a result, calling remove_filter() in a nested WP_Query instance could result in lazyloading being disabled for *all* current instances. However, I've been doing some more testing, and while I still think it's possible for _wp_filter_build_unique_id() to behave like this (something I'll open a separate ticket about), I think it will very rarely have an effect in this case, and it only did during 4.4 development because of the very specific kinds of tests I'd written.

So here's what I'm going to do: I'm going to swap out the updated_term_meta_cache flags, and replace with remove_filter(). This ought to fix your immediate issue, since the object will be removed from the $wp_filter global, and PHP should be able to reclaim the memory space. If you don't mind, I'll ask you to run this against your import scripts, to verify that it's resolving the issue for you. Then we can talk about whether the new action you've suggested still has a use case.

[boonebgorges]

In 36524:

Improve WP_Query lazyloading logic, for better performance.

Lazyloading for comment meta and term meta, introduced into WP_Query in
4.4, used flags - updated_term_meta_cache and updated_comment_meta_cache -
in an attempt to prevent cache priming from happening more than once per query
object. This technique was mostly effective, but not entirely efficient, since
the flag didn't prevent the lazyload_*_meta callbacks from running. The
obvious solution - removing the filter callback after it'd be run once - was
dismissed for 4.4 because of concerns that remove_filter() could disable
lazyloading too generally in the context of nested queries, due to the way
_wp_filter_build_unique_id() doesn't always build sufficiently unique IDs for
similar objects. However, further testing shows that this concern is only valid
in a very small subset of cases, while the cost of keeping the query objects in
memory, via the $wp_filter global, is quite significant. As such, this
changeset removes the flags in favor of the remove_filter() technique.

See #35454, #35816.

[boonebgorges]

#35454 was marked as a duplicate.

[lpawlik]

Replying to boonebgorges:

@stevegrunwell Thanks very much for the detailed request.

So here's what I'm going to do: I'm going to swap out the updated_term_meta_cache flags, and replace with remove_filter(). This ought to fix your immediate issue, since the object will be removed from the $wp_filter global, and PHP should be able to reclaim the memory space. If you don't mind, I'll ask you to run this against your import scripts, to verify that it's resolving the issue for you. Then we can talk about whether the new action you've suggested still has a use case.

@boonebgorges thank you for your response. The problem with your solution is that if callback is not called then hook still resides in $wp_filter and it still consumes memory. I just sent to @stevegrunwell more complex patch which seems to be solving the core of the issue. I am attaching it here but please keep in mind that it's still a beta version :)

[lpawlik]

POC - More complex solution

[boonebgorges]

@lpawlik This is a promising technique. But it doesn't distinguish between multiple instances of WP_Query existing at the same time. Say you have two posts in two categories, p1/c1 and p2/c2. Now do this:

$q = new WP_Query( array( 'p' => $p1 ) );
while ( $q->have_posts() ) {
    $q->the_post();

    $nested_q = new WP_Query( array( 'p' => $p2 ) );
    while ( $nested_q->have_posts() ) {
        $nested_q->have_posts();

        get_term_meta( $c2, 'foo' );
    }
}

That call to get_term_meta() should only prime the cache for the terms belonging to $nested_q - ie, $c2 - and not those belonging to the parent query - ie, $c1. (If you're not convinced by the nested queries, imagine a dozen queries in serial.)

Your approach could be adapted, by indexing post IDs in the WP_Lazy_Loader object by an identifier that is unique to the query instance. Something like:

    ...
    if ( $q['update_post_term_cache'] ) {
        handle_lazy_loads( $this );
    }
    ...

class WP_Lazy Loader {
    ...
    function add_posts( WP_Query $query ) {
        $key = _wp_filter_build_unique_id( array( $query, '' ) );
        $this->queries[ $key ] = $posts;
    }
    ....

    function lazyload_term_meta( $check, $term_id ) {
        ...
        foreach ( $this->queries as $qposts ) {
            // If term belongs to one of the posts, then prime cache for all post terms in the query
        }

        ...
    }

Does this seem right to you?

[lpawlik]

@boonebgorges Yes, you're right that my patch doesn't distinguish between multiple instances of WP_Query existing at the same time. I'd like to ask you a question as I may not fully understand whole flow here. What kind of benefits we have by distinguishing WP_Query instances? My point is that if we do as you suggest we will still have the same issue as now. Let me show you our use scenario:

• we were going to index all posts in Elasticsearch so we executed WP_Query in a loop.

$q = new WP_Query( ...args to get group of posts we need... );
$posts = $q->get_posts();
foreach( $posts as $post ) {
    $index_data = array(
        'key1' => $post->dummy_value_1,
        'post_content' => apply_filters( 'the_content', $post->post_content ),
        ....
        'keyX' => $post->dummy_value_x
    );
    index_post( $index_data );
}

Let's assume that we have 45000 posts to index so above loop will be executed 45k times. Each post content contains gallery shortcode which executes its own WP_Query (not directly but via get_posts() function). We will have 45k keys in $this->queries inside WP_Lazy_Loader class and each key will contain collection of WP_Post items. I am quite sure this is more or less the same as we have now but the only difference is that $wp_filter doesn't contain all data in favour of huge WP_Lazy_Loader instance. What are possible issues with not distinguishing between multiple instances of WP_Query? I think we may add a little overhead with my approach but on the other hand we may reduce memory consumption significantly. As I mentioned above I may not fully understand all possible issues this patch can introduce.

One extra note: we could keep only post ID and post_type inside WP_Query_Lazy_Load instead of all WP_Post data which would reduce memory requirements.

[boonebgorges]

@lpawlik Thanks for sticking with me here :)

What kind of benefits we have by distinguishing WP_Query instances?

Well, two kinds of benefits, I guess. First, a general separation of concerns. It feels aesthetically weird that doing something inside of one post loop would reach out and touch other query loops. Second, a concern about performance. If you have two loops, one with 5 posts and one with 100, it feels odd that calling get_term_meta() in the first one would cause termmeta to be loaded for terms belonging to all 105 posts.

But I've taken some time this morning to get some actual numbers behind these hunches. I set up an installation with 10,000 terms, all with some term meta. Then I assigned 10 of these terms to 1,000 different posts. I then ran some profiling where I fired up 100 different WP_Query objects (basically, paginating through all posts). I was able to reproduce your report of memory overhead from $wp_filter. I then started working with your patch and I found that very little overhead was associated with the call to update_termmeta_cache(), which means that my concern about keeping WP_Query instances separate probably is not warranted. It certainly simplifies the lazyloading logic if we can simply dump all of the posts in a single pot, and it doesn't seem to have serious performance implications in anything but the most extreme cases.

35816.2.diff​ builds upon your POC. It shows how it might integrate into WP with file structure, naming, etc. It abstracts the $meta_type so that the same logic can be used for commentmeta too (though I haven't implemented that yet). And it eliminates the check that causes metadata to be loaded only for those terms belonging to posts attached to the $term_id (gosh, that's confusing). Instead: the first time that get_term_meta() is called, termmeta cache is primed for all pending posts.

One extra note: we could keep only post ID and post_type inside WP_Query_Lazy_Load instead of all WP_Post data which would reduce memory requirements.

I implemented this too.

What do you think of this direction?

[lpawlik]

@boonebgorges thank you for taking care of my patch. I like how you implemented initial idea - this looks great and from my internal tests it fully solves our issues however I think that maybe we could add also some action hook when posts are added into lazy loader class. I've modified your patch a bit. I believe that even 100k of posts (actually only ID + post_type) inside WP_Metadata_Lazyloader won't occupy too much memory however it would be excellent if developers could control it somehow (reset() method added into WP_Metadata_Lazyloader)

<?php
add_action( 'metadata_lazyloader_posts_added', function( WP_Metadata_Lazyloader $metadata_loader ) {
    $metadata_loader->reset();
});

I think that your patch fully solves those issues (I saw that comments should also be fixed as they behave the same as posts) and it would be awesome if it could be merged into core.

[boonebgorges]

35816.3.diff​ is a full patch ready for review and feedback.

It looks a little bit bonkers, but I think it's a good long-term solution. Let me summarize what's going on for people who are just joining us :)

• In 4.4, we introduced the concept of "lazyloading" commentmeta and termmeta. This means that, in loops where we have a full set of object IDs (post IDs, comment IDs), we don't query the database for any term meta or comment meta until the first time get_term_meta() or get_comment_meta() is called in the loop. When that happens, metadata for all relevant objects is loaded, with a single query, into the cache.
• The idea behind this technique is good, but the 4.4 implementation involved a callback method on WP_Query. This means that WP_Query objects were getting stored in the $wp_filter global. And WP_Query objects can get very, very large, as when you have a huge number of posts or the post_content is very big.
• 35816.3.diff​ refines the concept by introducing a central queue for objects waiting to have their metadata lazyloaded. So, for example, during a comments loop like wp_list_comments(), comment IDs are queued into the WP_Metadata_Lazyloader singleton. It's this (much smaller) singleton that contains the callback that gets hooked to 'get_comment_metadata'. Likewise for termmeta for terms belonging to posts in a WP_Query loop.

The metadata loader is meant to be fairly abstract, so that we can easily implement the same technique for other kinds of metadata and loops - like users, or even postmeta - at some point in the future.

(@lpawlik it also contains your reset() idea. I accidentally deleted your most recent patch, though - sorry!)

[lpawlik]

Replying to boonebgorges:

35816.3.diff​ is a full patch ready for review and feedback.

@boonebgorges awesome work on refactoring and polishing initial idea!

Thank you for your work on this issue. We really appreciate it.

[stevegrunwell]

I step away from the ticket for a long weekend and @boonebgorges + @lpawlik completely innovate the way we load large datasets in WordPress :)

The latest patch looks great, and I think this opens up a lot of possibilities when it comes to large data sets. The original patch was a "well, a simple action should be easier to get into core so we can work around this issue" move, but seeing such rapid movement on attacking the root cause is awesome!

[boonebgorges]

In 36566:

More performance improvements to metadata lazyloading.

Comment and term meta lazyloading for WP_Query loops, introduced in 4.4,
depended on filter callback methods belonging to WP_Query objects. This meant
storing WP_Query objects in the $wp_filter global (via add_filter()),
requiring that PHP retain the objects in memory, even when the local variables
would typically be expunged during normal garbage collection. In cases where a
large number of WP_Query objects were instantiated on a single pageload,
and/or where the contents of the WP_Query objects were quite large, serious
performance issues could result.

We skirt this problem by moving metadata lazyloading out of WP_Query. The
new WP_Metadata_Lazyloader class acts as a lazyload queue. Query instances
register items whose metadata should be lazyloaded - such as post terms, or
comments - and a WP_Metadata_Lazyloader method will intercept comment and
term meta requests to perform the cache priming. Since WP_Metadata_Lazyloader
instances are far smaller than WP_Query (containing only object IDs), and
clean up after themselves far better than the previous WP_Query methods (bp
only running their callbacks a single time for a given set of queued objects),
the resource use is decreased dramatically.

See [36525] for an earlier step in this direction.

Props lpawlik, stevegrunwell, boonebgorges.
Fixes #35816.

[DrewAPicture]

In 36897:

Docs: Add a missing file header to wp-includes/class-wp-metadata-lazyloader.php, introduced in [36566].

See #35816. See #35986.

[DrewAPicture]

In 36898:

Docs: Improve inline documentation syntax throughout WP_Metadata_Lazyloader, introduced in [36566].

See #35816. See #35986.

[DrewAPicture]

In 36919:

Docs: Improve the DocBlock summary for wp_queue_comments_for_comment_meta_lazyload(), introduced in [36566].

See #35816. See #35986.

[DrewAPicture]

In 36941:

Docs: Improve the DocBlock summary for wp_metadata_lazyloader(), introduced in [36566].

See #35816. See #35986.

[DrewAPicture]

In 36945:

Docs: Improve the DocBlock summary for wp_queue_posts_for_term_meta_lazyload(), introduced in [36566].

See #35816. See #35986.