Add an in-memory LRU cache

Updated the patch from #105 in a MR

- Move allowedSlots property declaration to constructor; replaces mismatched default value on property (#106)
- Added missing early return in set() (#113)
- Override setMultiple() to only count() extra items once before removal (#108)
- Code style fixes

Just cross-referencing with another old issue: https://www.drupal.org/project/drupal/issues/1961934#comment-7274730 →

Back when I raised that I was seeing a pattern of entity cache problems, and observed that a LRU approach was similarly problematic in the circumstances in question ("With LRU and limited memory, you can easily throw out everything you want to keep"), so I proposed a way of providing hints to the cache to enable it to be smarter.

I haven't tried to catch up with what's currently happening in this issue, so apologies in advance if it's not relevant any more; but seeing as how there's current activity on this LRU caching issue, I thought I'd add a link in case it was still a going concern.

@jweowu we're talking about static memory caches here so what other users are doing is not really relevant. A static memory cache is empty at the start of every request.

Very true, "other users" are only relevant to the persistent caches (I was considering both in the other issue).

I don't believe that renders the notion irrelevant, though. After all, no cache-expiry scheme has any advantage over any other if the cache doesn't fill up. We're only concerned about what happens when, for whatever reason, the cache fills up, and my suggestion is that giving the programmer some control over what should be flushed and what should be kept in those situations would be an improvement, and that a simple LRU approach isn't always going to be the best approach.

Would also add the original issue described for Drupal 7 was fixed for Drupal 8+ in this issue #1596472: Replace hard coded static cache of entities with cache backends → - which is what allows this one to happen cleanly e.g. the entity static cache can already be reset independently of the persistent cache.

Adding a reference to 📌 Add an entity iterator to load entities in chunks Needs work too which would help when iterating over a very large number of entities.

Pushing on this one again. The entity static cache filling up on long running jobs is an issue I run into quite often work around and then when I hit it again I get sad that we're get to fix this.

I opened this issue, but the last time I worked on the actual code here was 7 years ago, so I think I can RTBC it.

I'll have a quick look myself to see if I can +1 this.

Okay so the code will work, I really like the thorough test coverage and the fact that we uncovered MemoryCache itself didn't have a test extending GenericCacheBackendUnitTestBase yet. Only remark I have in general is that some methods are dedicated to numeric keys while other methods cover both string and numeric keys in one go. But don't let that detract from the overall excellent coverage.

I found a tiny nit regarding the word pidgin and think the test could benefit from renaming the assertion and an extra comment or two. This is why I'm setting back to NW.

The comment about moving the shared logic I'll leave up to you to decide, as MemoryBackend also seems to copy-paste things across methods occasionally. So I wouldn't be too bothered if we copy-paste a few lines of code here too. So feel free to RTBC without addressing this.

Now that this introduces another memory cache, would it be prudent to add a line to MemoryCache and LruMemoryCache's docblocks to tell people they should declare these bins using the cache.bin.memory tag?

I fixed 3/4 of the comments on the MR. There was one I didn;'t think needed doing.

Re the cache.bin.memory tag - interestingly the entity.memory_cache does not use this tag and it's not really managed like other cache bins or memory cache bins. I think we should consider adding documentation about somewhere but I don't think it should be in this issue. I also think we could consider defaulting to a LRU cache for all memory caches.

One final nit, but can be accepted upon commit.

I agree that we should create a follow-up for checking if we have sufficient documentation re cache.bin.memory and why entity storages don't use it like that.

I tested this code on a long running batch process from the Entity Usage module - to build the entity_usage table from a large site 30,000+ nodes and 200000+ paragraphs using a module I made with this code - see https://www.drupal.org/project/entity_lru_cache → - and it makes a massive difference. Memory no longer spirals to 90% of VM I'm running the job in and everything is a bit quicker too.

Oh wow, that's great. We should consider making it the default then, no?

@kristiaanvandeneynde see 📌 Use LRU Cache for static entity cache Postponed

I'm running a migration that usually takes 40 hours ish.
As a control I first am running it with attemptMemoryReclaim commented out on Drupal 11.1.0

I will update periodically.
It's several migrations chained
11280/81350 8min 43s/1hr 2min 24MiB

Discussed sizing a bit @catch and we agreed to remove the default as any code that uses a LRU cache should consider the number of slots for the use-case.

Since my last comment I ran the migration on production with and without the LRU cache. Both took 39 hours.

@alexpott mentioned this is a positive result since it's the same, this update will either improve or do nothing, my migration helps prove if it's not needed it does not make the situation worse.

Wonder if we could go with LFU cache instead. Will the implementation be more complex?

LFU would be much more complex yes, you have to keep a count of how often an item has been accessed and then evict the least frequently accessed item. And the current implementation is being careful to add as little CPU overhead as possible.

In general on a normal web request, we've set the limits high enough that items should never be evicted, so it will only kick in for migrations and similar tasks, or pages that load an extreme number of entities for some reason.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed the PHPStan issue.