Opened 4 years ago
Closed 3 days ago
#52582 closed defect (bug) (duplicate)
wp_cache_* duplicate/redundant storage and insufficient clearing of cache
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Comments | Keywords: | |
Focuses: | performance | Cc: |
Description
This is something I'm currently encountering, but have not been able to fully investigate myself (yet).
Summarized backstory;
Using Redis, sometimes maxes memory > needs flush. Started investigating whats taking up soo much memory (relatively simple sites).
~42,000/67,000 records stored are for get_comment_child_ids
and get_comments
.
(By far not that many comments on my sites)
Looking for a single specific comment ID it is repeated between 30-50 times, while in theory it should be just once (unless using it in different contexts? (->query_vars))
Possible cause;
For the methods get_comments
and fill_descendants
the wp_cache_get_last_changed( 'comment' );
is used to create a cache key for the comments / childs.
https://core.trac.wordpress.org/browser/tags/5.6.1/src/wp-includes/class-wp-comment-query.php#L432
https://core.trac.wordpress.org/browser/tags/5.6.1/src/wp-includes/class-wp-comment-query.php#L998
When a new comment is inserted for example, there is a attempt to delete the comment cache by calling clean_comment_cache()
, but it seems this is based on the comment ID, which is not the same as the key / doesn't target the childs.
The 'last_changed' is however changed at the same time in that function;
https://core.trac.wordpress.org/browser/tags/5.6.1/src/wp-includes/comment.php#L3195
Which I think makes it impossible for the prior cached data to be found because it uses that in the key.
This causes it to store the same data over and over (redundant), even when it hasn't changed for those comments, with new cache keys without clearing the old ones (duplicate).
Could be I'm completely off, but wanted to get another pair of eyes on it. If whats described is actually happening it looks pretty major for caching efficiency.
Change History (5)
#3
@
4 days ago
Reviewing this after several years, is this something you have ideas or feedback for @tillkruess @spacedmonkey?
#4
@
4 days ago
@flixos90 This would be solved by https://core.trac.wordpress.org/ticket/59592
#5
@
3 days ago
- Keywords 2nd-opinion needs-testing removed
- Milestone Awaiting Review deleted
- Resolution set to duplicate
- Status changed from new to closed
Thanks @tillkruess!
Since that ticket already has a lot more discussion on the approach to address the problem, I'm going to go ahead and mark this ticket as a duplicate.
Thank you again for reporting this @sormano. Please follow along in #59592 for further updates.
Thanks for the report @sormano , and welcome.