Comment caches should be persistent

[boonebgorges]

The 'comment' cache group is non-persistent. We should try to make it persistent.

To make the change, we'll need a full review of how the 'comment' group is used throughout core. It's likely that there are many places where we don't bother invalidating because the cache only lasts for the page load.

It may be possible to phase in the persistent cache by breaking the 'comments' group into two or more pieces. I'm thinking specifically of individually cached comments (eg wp_cache_set( $comment_id, $comment_object, 'comment' )) vs all other uses of the 'comment' group (ID queries, etc). It's likely going to be easier to cover all the invalidation cases for single comments.

[lukecavanagh]

Good idea.

[spacedmonkey]

A quick bit of searching shows that the group isn't being used for anything another than storing commet objects and the query caching. Here are the references I found.

​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/comment.php#L1614
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/comment.php#L2570
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/comment.php#L2582
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/comment.php#L2600
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/comment.php#L2627
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/class-wp-comment.php#L199
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/class-wp-comment.php#L208
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/class-wp-comment-query.php#L384
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/class-wp-comment-query.php#L387
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/class-wp-comment-query.php#L391
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/class-wp-comment-query.php#L394

I might call clean_comment_cache on this line.
​https://github.com/WordPress/WordPress/blob/d11bf53a9998cf8ba267c983074c1eb8f3f91dcb/wp-includes/comment.php#L1614

But that is just me being picky. I personally don't see the harm of turning it on.

[spacedmonkey]

Is there any chance this is this might be in 4.6? Running a large scale sites with comments is results in an extremely high number of sql queries. To patch this I am currently working on caching plugin here

​https://github.com/spacedmonkey/advanced-comment-cache

This prime comment cache for another persistent cache. This is a dirty workaround through. It seems to me like we could have make it a persistent cache group, as the cache invalidation seems to there. It will be about find the edge cases, white testing it help with. Are there any unit test that can help?

[boonebgorges]

Is there any chance this is this might be in 4.6?

It's possible, but someone (maybe you?!) will have to take the initiative to do the research, etc.

I'd suggest starting with a look through the logs to see when the 'comment' was made non-persistent. I assume that this was a change made to fix certain invalidation problems. The conversations around the change probably contain some useful warnings.

Are there any unit test that can help?

We have a lot of comment-related unit tests. $ phpunit --group comment But these test mostly only cover previous bugs, so it's likely that there are gaps. A good strategy is to search the codebase for all the places where a comment can be updated/deleted/created, and then to cross-reference against the existing tests to see whether there are tests covering the necessary invalidation.

[spacedmonkey]

Looking into it, there isn't any documentation for why it was added a non-persistent cache group add here #6740 .

Looking into the delete / update functions already has clean_comment_cache function calls.
​https://github.com/WordPress/WordPress/blob/da78aeffe9fb2350e09352202c2a33ddfce0b329/wp-includes/comment.php#L1193
​https://github.com/WordPress/WordPress/blob/da78aeffe9fb2350e09352202c2a33ddfce0b329/wp-includes/comment.php#L2024

I ran all of the tests for the comment group and nothing fails. I have added a patch, that is pretty simple. I will work on some tests later.

[boonebgorges]

[37608] adds tests for WP_Comment_Query invalidation.

[boonebgorges]

In 37609:

Add tests demonstrating individual comment cache invalidation.

See #36906.

[boonebgorges]

Before moving forward, we're hoping to get some feedback from @ryan as to why the 'comment' group was originally made non-persistent.

[ryan]

It's likely that there are many places where we don't bother invalidating because the cache only lasts for the page load.

Exactly. Back when, the comment (and counts) groups didn't have the invalidation needed to allow them to persist beyond a page load. We made do with using non-persistent cache to reduce duplicate queries. I don't recall exactly what invalidation was missing. Lots of changes (with which I'm out of touch) have happened since, so maybe this is ready to become persistent.

[spacedmonkey]

I think this was long before the clean_comment_cache function was implemented. In my patch, I implement clean_comment_cache in wp_insert_comment. The last changed value is changed, but the create function should also call the cache clear. All other insert functions do this (such as clean_post_cache) and it just makes sense. When looking into the code, I was surprised to see that everything I thought that was needed for the persistent was in place. I think it's time.

[boonebgorges]

Replying to ryan:

It's likely that there are many places where we don't bother invalidating because the cache only lasts for the page load.

Exactly. Back when, the comment (and counts) groups didn't have the invalidation needed to allow them to persist beyond a page load. We made do with using non-persistent cache to reduce duplicate queries. I don't recall exactly what invalidation was missing. Lots of changes (with which I'm out of touch) have happened since, so maybe this is ready to become persistent.

Thanks! This context is very helpful.

In my patch, I implement clean_comment_cache in wp_insert_comment. The last changed value is changed, but the create function should also call the cache clear.

A new comment hasn't yet had a chance to be cached, so there's nothing to be cleared. If I'm wrong, and calling clean_comment_cache() fixes a bug here, please provide instructions on how to reproduce the bug. (Probably a good topic for a separate ticket.) Otherwise, let's not add the overhead, small as it may be.

[boonebgorges]

In 37613:

Make the 'comment' cache group persistent.

'comment' was made non-persistent in [7986], to address the difficulty of
reliable cache invalidation. Since then, the comment system has improved such
that we can be more confident that caches are being busted as needed.

Props spacedmonkey.
Fixes #36906.

[spacedmonkey]

@boonebgorges it's doesn't fix a bug, but it adds consistency to the insert functions. Both posts and terms call the function for different reasons.

​https://github.com/WordPress/WordPress/blob/477f6346eb3935f8d2672b6e4ec2359e7d23ef6b/wp-includes/taxonomy.php#L2410
​https://github.com/WordPress/WordPress/blob/9193013158d02ecd51ab8aa52d594aa789ac9836/wp-includes/post.php#L3430

If you are building a caching plugin or the like for comments, it is a expected that when you create function would also all the cache clean function. I had to add a work around to get my comment caching plugin working.

Example here
​https://github.com/spacedmonkey/advanced-comment-cache/blob/master/advanced-comment-cache.php#L104-L107

For developing plugins, it is important that the 'clean_comment_cache' action runs on insert of comments as well. I know that it will delete nothing likely, as the no object will not exist, but it will not hurt.

[boonebgorges]

@spacedmonkey We don't generally make internal changes for mere consistency reasons, which can be largely aesthetic. But

it is important that the 'clean_comment_cache' action runs on insert of comments as well

this sounds like a reasonable expectation.

[boonebgorges]

In 37614:

Use clean_comment_cache() in wp_insert_comment().

Previously, only the 'last_changed' incrementor was manually invalidated, since
the newly created comment did not yet exist in the cache. However, this created
an inconsistency with the other comment CRUD functions, which result in the
'clean_comment_cache' action firing.

Props spacedmonkey.
See #36906.

[johnbillion]

In 41084:

Build: Switch to a Travis build matrix for the 3.9 branch.

See #36906

[boonebgorges]

In 41188:

Ensure that an assertion takes place in Tests_Comment_Query::test_comment_query_should_be_cached().

Missed during [37608]. See #36906.

Fixes #41357.