WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#36906 closed defect (bug) (fixed)

Comment caches should be persistent

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version: 2.2
Component: Comments Keywords: has-patch
Focuses: performance Cc:
PR Number:

Description

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.

Attachments (1)

36906.patch (2.3 KB) - added by spacedmonkey 3 years ago.

Download all attachments as: .zip

Change History (23)

#1 @lukecavanagh
3 years ago

Good idea.

#2 @spacedmonkey
3 years ago

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.

#3 @spacedmonkey
3 years ago

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?

#4 @boonebgorges
3 years ago

  • Keywords needs-patch needs-unit-tests added

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
3 years ago

#5 @spacedmonkey
3 years ago

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.

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


3 years ago

#7 @boonebgorges
3 years ago

[37608] adds tests for WP_Comment_Query invalidation.

#8 @boonebgorges
3 years ago

In 37609:

Add tests demonstrating individual comment cache invalidation.

See #36906.

This ticket was mentioned in Slack in #core-comments by boone. View the logs.


3 years ago

#10 @boonebgorges
3 years ago

  • Milestone changed from Future Release to 4.6

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

#11 follow-up: @ryan
3 years ago

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 what was missing. Lots of changes (with which I'm out of touch) have happened since, so maybe this is ready to become persistent.

Version 0, edited 3 years ago by ryan (next)

#12 @spacedmonkey
3 years ago

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.

#13 in reply to: ↑ 11 @boonebgorges
3 years ago

  • Keywords needs-patch needs-unit-tests removed
  • Owner set to boonebgorges
  • Status changed from new to assigned

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.

#14 @boonebgorges
3 years ago

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

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.

#15 @spacedmonkey
3 years ago

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

#16 @boonebgorges
3 years ago

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

#17 @boonebgorges
3 years ago

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.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


3 years ago

#19 @ocean90
3 years ago

  • Keywords needs-dev-note added

#20 @ocean90
3 years ago

  • Keywords has-patch added; needs-dev-note removed

#21 @johnbillion
2 years ago

In 41084:

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

See #36906

#22 @boonebgorges
2 years ago

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.

Note: See TracTickets for help on using tickets.