Consolidate entity_type_values cache tag

Created on 7 February 2025, 2 months ago

Problem/Motivation

In Introduce a list of "common cache tags" to reduce lookup query amount Active , 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

📌 Task
Status

Active

Version

11.0 🔥

Component

entity system

Created by

🇨🇭Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • 🇨🇭Switzerland berdir Switzerland

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

  • 🇬🇧United Kingdom catch

    As far as I can tell, this is only used to avoid clearing the entire cache.entity bin 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.

  • 🇨🇭Switzerland berdir Switzerland

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

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    I 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:

    Eh, double-edged sword really. On some sites it would greatly improve things to flush the entire entity cache in one go, but on other sites I'd be pretty annoyed if my large node cache got flushed because I created a new user.

    But come to think of it, that doesn't happen on entity save 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(). 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.

  • 🇬🇧United Kingdom catch

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

  • Pipeline finished with Failed
    2 months ago
    Total: 108s
    #417805
  • Pipeline finished with Failed
    2 months ago
    Total: 108s
    #417806
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

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

  • Pipeline finished with Failed
    2 months ago
    Total: 94s
    #417807
  • Pipeline finished with Failed
    2 months ago
    Total: 527s
    #417826
  • 🇨🇭Switzerland berdir Switzerland

    would probably wait with this until the preload issue is in this will obviously conflict heavily and we'll want to adjust that anyway.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

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

  • Pipeline finished with Failed
    2 months ago
    Total: 414s
    #417885
  • Pipeline finished with Failed
    2 months ago
    Total: 420s
    #419680
  • 🇨🇭Switzerland berdir Switzerland

    Rebased.

  • Pipeline finished with Success
    2 months ago
    Total: 573s
    #419711
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

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

    • catch committed 53fad5c5 on 11.x
      Issue #3505059 by kristiaanvandeneynde, berdir, catch: Remove...
  • 🇬🇧United Kingdom catch

    Went ahead and did the inline before commit.

    Committed/pushed to 11.x, thanks!

  • 🇨🇭Switzerland berdir Switzerland

    Phew, things are moving fast at the moment, created a change record just in case and also updated https://www.drupal.org/node/3505248 accordingly.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024