Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
entity system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
7 Feb 2025 at 08:05 UTC
Updated:
24 Feb 2025 at 21:09 UTC
Jump to comment: Most recent
Problem/Motivation
In #3436146: Introduce a list of "frequent cache tags" to reduce lookup query amount, one set of cache tags that's awkward to deal with are the entity storage {$entity_type}_values cache tags. Unlike others such as the _view cache tags, they are often used standalone and often pretty early (for their respective page before dynamic page cache).
So they would be a good target to preload, the challenge is knowing which ones to preload without overhead of dynamically collecting them and avoiding to add too many.
Quoting me from that issue:
that would be an option yes, not sure how many of those we want to have. it's one more event subscriber that's called on every page, but that should be much faster than a query. node seems like something that's a fairly safe but what about others?
in our project, I added paragraph_values, crop_values, menu_link_content_values, media_values, file_values, crop_values, block_content_values as those that I quickly identified as being used frequently on their own. But many other entity types aren't, as mentioned.
One thing is that they are very rarely used. there are very few reasons to call $storage->resetCache(). I see almost no calls outside of tests, we don't even do it when making field changes. I checked two projects that have been running for a while, not a single invalidation on any *_values cache tag. I was wondering if we should maybe optimize this away internally by just having a single cache tag for all entity types or something like that. would mean it would invalidate all entity types caches.
The cache tag is used for persistent and memory caches. Not sure if memory caches are more likely to just invalidate one tag, but I usually don't bother with that, I just empty the memory cache.
The downside is that calling $storage->resetCache() on any entity type would in practice invalidate all of them. But basically the only reasons for doing that are:
a) Tests
b) Bypassing the entity storage API, e.g. commerce seems to have a drush command that empties certain tables and then it clears the cache.
Steps to reproduce
Proposed resolution
Replace the tags with a single entity_values, add that to the preload list.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3505059
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3505059-consolidate-entitytypevalues-cache
changes, plain diff MR !11145
Comments
Comment #2
berdirThis would break manual invalidations of that tag. We could could consider a BC layer that translates these cache tags to entity_values somewhere.
I found 0 matches of 'node_values' in code search and exactly one for user_values, which is use case b) above (manually inserting stuff into a user field table).
But such a system could also be useful for the block cache tag issue.
Comment #3
catchAs far as I can tell, this is only used to avoid clearing the entire
cache.entitybin when ::resetCache() is called on content entities without IDs.If we consolidated to a single cache tag, then it would be equivalent to clearing the entire bin anyway , so can we just remove the cache tag entirely and clear the whole bin?
I personally don't think we need to worry about bc for the cache tags as such, just that ::resetCache() should still work.
Comment #4
berdirI was wondering about that, I guess that's an option too.
I assumed people might use that bin for some other stuff, but finding almost zero direct usages to it: http://codcontrib.hank.vps-private.net/search?text=%5CDrupal%3A%3Acache%..., http://codcontrib.hank.vps-private.net/search?text=%40cache.entity&filen....
Worst case is we invalidate a tiny bit more, but again, it's an extremely rare operation to do that.
Comment #5
kristiaanvandeneyndeI basically said as much as #3 in your MR here: https://git.drupalcode.org/project/drupal/-/merge_requests/10962#note_45...
One thing I'd like to quote from my comment on the other MR:
But come to think of it, that only happens on a manual ::resetCache() call, so the tag is really just adding resource consumption for almost no gain outside of some tests and even there we could argue there's no point to be this granular. It will happen, though, on sites that manually called ::resetCache() for whatever reason. Those people might be a bit annoyed if their resetting of one entity type's cache now clears the entire entity bin.
For BC reasons theoretically we should keep the tag for one more major,, but that would still incur the cache tag lookup for an entire major and, as pointed out by @berdir, there is very little code that calls it.
+1 for removing the tag altogether, but I do think we need a CR for it, even if only as a warning that calling ::resetCache() now clears the entire bin.
Comment #6
catchCache tags themselves aren't really an API we need to provide bc for. Obviously we wouldn't just rename the entity list cache tags for fun but otherwise you're not really supposed to deal with individual cache tags directly that you don't control, or at least be prepared to update. Agreed with a change record though.
Comment #8
kristiaanvandeneyndeMy Selenium is acting up locally so created a MR to see what testbot thinks. Will see if I can fix my local test setup. I chose to keep the persistent cache check because it seems counterintuitive to flush the persistent cache when we call ::resetCache() on an entity type that isn't even in said cache.
Comment #9
berdirwould probably wait with this until the preload issue is in this will obviously conflict heavily and we'll want to adjust that anyway.
Comment #10
kristiaanvandeneyndeYeah sure, just wanted to get the ball rolling already and see what tests have to say. Made it go green locally so should go green here too.
Comment #11
berdirRebased.
Comment #12
kristiaanvandeneyndeLooks good to me. Left a note that I think a committer might raise, but good enough to RTBC for all I care as it can be changed upon commit if they so prefer.
Comment #14
catchWent ahead and did the inline before commit.
Committed/pushed to 11.x, thanks!
Comment #16
berdirPhew, things are moving fast at the moment, created a change record just in case and also updated https://www.drupal.org/node/3505248 accordingly.