Replace hard coded static cache of entities with cache backends

Sounds like a good improvement, the ArrayCache implementation is a good idea!

class DatabaseBackend implements CacheBackendInterface { Wrong name, right? Should be ArrayBackend ?

'cache controller' => 'Drupal\Core\Cache\MemoryBackend', Sounds wrong too, is it ArrayBackend or MemoryBackend? I'm fine with both, but you have to agree with yourself about this one!

If you don't react before I'm getting back home, I'll try to fix your patch.

I think this needs more architectural planning.

The 'static cache' flag in the entity (crud) controller is vastly more intelligent than a completely separate memory cache backend.

It is true that we could attempt to split and separate that backend, but we don't want to lose the static caching within the crud controller entirely.

This patch adds "cache controller", but actually means "static cache controller".

Let's also bear in mind that there's a cost of instantiating one more object (including config + DI) for every single entity type.

We can chain backends if we want both static and a real cache, sounds like an easy win: we could get rid of this awful loading logic that do both, and rely on a unique object with a clean interface that would do both at the same time. It's a pattern we could also re-use at various places in Drupal too.

Instanciating a single object, you won't notice it on your runtime, it's not significant, even with 10 or 20 different entity controllers, this won't hurt a bit your execution time, it's really nothing compared to what does a single Views, yet most people continue to put up to 10 Views on the same page.

In Drupal 7 the static cache is quite dumb, all you have is a cacheGet() and cacheSet() internal methods, which acts like a basic key/value store (if we omit the deprecated conditions array). If it is smarter than this, please enlighten me because I probably missed it.

Some other ideas arround the idea:
- We could, in the future, share cache chains to entity controllers that needs the same chain (we should then prepare some kind of mass deregistration mecanism in the array cache for static entries to act upon controller destructor)
- This pattern, with a clean cache chain definition mecanism, could be used in a lot of other places where the code actually emulate this behavior, for example a lot of info hooks

Using it massively would remove a lot of duplicated code everywhere, it might even be useful in conjunction with the actual DrupalCacheArray implementation that could evolve to use this base.

Using the chain of command (for saving), chain of responsability (for loading) and proxy patterns (basically using a chain towards the objects behind) for this seems really clean in term of architecture because the final object only rely on the cache interface, without worrying about what's behind, it's actually maintainer's even somehow.

We can chain backends if we want both static and a real cache, sounds like an easy win: we could get rid of this awful loading logic that do both, and rely on a unique object with a clean interface that would do both at the same time. It's a pattern we could also re-use at various places in Drupal too.

I like the idea of having a dedicated cache system, which has a single consistent interface to developers. Having a NullCacheBackend + a static cache backend that proxies another permanent backend makes much sense too me too.

I don't like having the EntityStorageController directly talking to the cache system though. Imho, we loose some flexibility here.

I'd prefer the approach taken by #1302378: Use multiple specialized entity controllers: Have a dedicated entity cache controller class, which is invoked by entity_load() and handles everything related to caches - see #1302378-4: Use multiple specialized entity controllers (comment #4).

Reasons:

• I like being able to talk directly to the storage controller, bypassing any caches without resetting them. E.g. this is what entity_load_unchanged() does now....
• Then, the entity cache controller could be used by the entity providing module to easily implement custom caching extensions. E.g. in profile2 I'm statically caching profile ids per user. But by having this cache outside of the controller you'll have to re-implement all the cache reseting logic. Compared to that, you'd get the reset-logic basically for free when the custom cache is implemented in the cache-controller, as the cache-controller gets already reset when necessary.
• We'll probably want some entity-type specific cache controller configuration, e.g. the cache bin to use. The entity cache controller would be the obvious place to take care of that.
• Then, I'm not so sure about loosing the possibility to fetch from cache when loading by $conditions. Yep, $condition is deprecated, but we have not yet built the easy to use replacement aka entity_load_by_properties() or so. I could see having the helper using the same pattern as entity_load(), what would make it possible to specifically add at least static caching for frequently used queries.

Then, I'm not so sure about loosing the possibility to fetch from cache when loading by $conditions. Yep, $condition is deprecated, but we have not yet built the easy to use replacement aka entity_load_by_properties() or so. I could see having the helper using the same pattern as entity_load(), what would make it possible to specifically add at least static caching for frequently used queries.

I think that the conditions (or properties later) loading wouldn't be affected that much by replacing the internal cacheGet() with a backend directly: you still control what happens in your load() function and you are not forced to cache entities by id, you can do something else instead. You can still add a layer for frequently used queries in specific controllers.
But, to be honest, I think that trying to implement such layer would be more than difficult, the load() method may still be optimized later for very specific cases if needed. I even have a hunch that if the controller has direct access to its own cache backend, it would be easier to implement such frequently used queries caching: the controller knows its own schema and fields.

I'd prefer the approach taken by #1302378: Use multiple specialized entity controllers: Have a dedicated entity cache controller class, which is invoked by entity_load() and handles everything related to caches - see #1302378-4: Use multiple specialized entity controllers (comment #4).

The approach in #1302378: Use multiple specialized entity controllers suppose that there is an upper layer that needs to combine itself the cache and "main" controller, which means that the entry point of the API is in another piece of code (in the exemple in procedural code), I think this would be prone to errors: some developers will continue to hit the controllers directly (which makes a lot of sense) instead of calling entity_load(). I don't think this a good idea until there is a front controller for all entity operations.
I think that in the end, both provide the same feature and generalize caching method, but I prefer catch's method: it allows the entity "main" controller to implement its own load/save etc.. methods and have a finer control over its own cache if needed, while most of the others would probably continue to use the generic one.

We'll probably want some entity-type specific cache controller configuration, e.g. the cache bin to use. The entity cache controller would be the obvious place to take care of that.

Regarding specific entity type cache configuration, weither it's a cache controller or a chain set inside the main controller, I think both solutions are equivalent in term of configuration, especially if we can create the cache chain outside and inject it when building the controller instance: instead of creating a cache controller from configuration, we create a cache chain from configuration, doesn't seem too different in the end, it just moves some responsabilities.

And, latest note, I think that even if the cache controller is the way to go, this isntance can still benefit from using the cache chain catch is proposing, it's just a matter of moving it into another objet.

I think that in the end, both provide the same feature and generalize caching method, but I prefer catch's method: it allows the entity "main" controller to implement its own load/save etc.. methods and have a finer control over its own cache if needed, while most of the others would probably continue to use the generic one.

That gets the point. I've aimed for moving that cache-control into a separate class, such that it stays separated from the storage class.
However, I just realized this thinking is deprecated, as we won't have "pluggable" storage controller anyways (sry, I'm still used to that thinking). As discussed in the entity-interface introduction issue, they already implement the per entity-type storage logic, so any simple storage-pluggability (=without repeating entity-type specific customizations) would have to happen on a lower level. In that light, having the cache-control in the storage-class makes sense to me too. :)

Apart from the conditions stuff but I'd prefer to press on with the issue that removes it. Almost did that before starting this one but wanted to get this out for discussion.

Great, I'm about to look at that as well :)

@patch: Having a permanent cache renders hook_entity_load() useless. If possible I'd prefer to drop it in the long road anyways, an entity really shouldn't be altered by page request. If it needs to be changed, re-save it with new property values.

Having a permanent cache renders hook_entity_load() useless. If possible I'd prefer to drop it in the long road anyways, an entity really shouldn't be altered by page request. If it needs to be changed, re-save it with new property values.

There's a point I agree with is that we shouldn't alter this kind of stuff on every load, it can get quite CPU intensive if you have a lot of modules playing with this. But, in the other end, having the hook_entity_load() can be useful for some modules to set business data which are not properties, but that needs to be on the entity and cached along with it. Concrete use case: I could load data from business tables and set them on a node, but I don't want to store this data in actual node properties because I don't want to duplicate it and handle the full synchronization between my source business data and node properties. If I can still attach arbitrary data on the object I can then just handle nice synchronisation by clearing a few cache on my business table updates. Right now, I actually achieve this using the virtual_field module, that provides a null field storage backend, but defining some virtual fields allows me to actually store precomputed data in field cache along with my commerce products, line items and stuff, without worrying about a complete field storage layer.

EDIT: Sorry, a bit off topic, and that's an akward and quite ugly way of doing this, but in my case, it's mainly for denormalizing some data for performances. I don't know where the property system is going to right and if it's going to help me with those kind of use cases.

What matters in here, is that by caching entities, we cache the hook_entity_load() result with it, and that's why I really like the idea, because if we do this I can then remove my need for hacks such as virtual field etc.

What matters in here, is that by caching entities, we cache the hook_entity_load() result with it, and that's why I really like the idea, because if we do this I can then remove my need for hacks such as virtual field etc.

Yeah, if there remains a hook_entity_load() it has to be cacheable. Altering stuff or changing stuff on a per request base there is a bad idea, and we should stop supporting that.

The discussion of removing it probably belongs more into #1497374: Switch from Field-based storage to Entity-based storage though, as it depends on whether we want to support a "modular entity storage". However, even with a centralized entity storage I agree that it might be worthwhile to keep it for easily adding non-queryable stuff as you've depict in your use-case.

Yes, I honestly think those kind of non queryable stuff are really ugly, but sometime you just don't really have other straightforward choices. I don't know what will be final API for properties, but properties could solve that somehow.

The ArrayCache implementation is a great idea indeed. However, I want to propose some changes and ideas that came up today while we were discussing the Entity Property API and waiting for fago to write down the initial interface definitions :).

The CacheChainInterface

Since we have multiple caching layers we might end up with various combinations of them (static cache only, static + persistent cache, persistent cache only, etc.). We could prevent that from happening if we had a way to chain cache backends in some way. This could be implemented in a generic fashion as a CacheChainInterface that would extend the CacheBackendInterface. This interface would extend the constructor definition for the CacheBackendInterface with an additional argument: An array of cache backends that the should be queried sequentially in order to retrieve an object from the cache.

The CacheChain class would iterate over the CacheBackend implementations that it has attached to it and forward the get / getMultiple queries to them. Once we have a full result for a query we exit the loop and, if there are any cache backends that could not return a result for the query before, we use the set() method on them to fill them with everything that they don't have cached yet. The next time the same query hits the CacheChain the first backend in the chain will now have element(s) that are subject to the query and return them right away.

This way we would be able to easily stack caching layers on top of each other and also replace single layers independently.

Oh great. I will take a look at your sandbox then. I think this can work really well with the PropertyContainerInterface that is part of the 3rd iteration of the Entity Property API for defining the tag-properties of the cache items (Note: the PropertyContainerInterface is generic and Entity agnostic so it would be completely generic).

Anyways... I agree this should be in a separate issue. I will create one later today and maybe already provide some code.

Sorry for off-topic :)

Not that off-topic, the cache chain interface is the idea behing this issue, and IMHO it should be done before we can actually continue this issue. It is a good reusable pattern that could be used at many places.

Opened and implemented and unit tested #1651206: Added a cache chain implementation.

A small changes here could have significant ramifications (for better or worse), so I am adding a tag as a reminder to check performance.

I nearly committed a very basic EntityCache controller for the Countries module before doing some simple tests to discover that this caused a 20 fold decrease in speed. On the plus side of this testing, reading in an array of Country entities from the database was 3 times faster than including iso.inc and doing a single drupal_alter() on the countries list. So performance gains could be significant here :)

Awesome. Now we can get this rolling again. I worked on this a bit during DCon Munich already but was not really satisfied with the outcome because it was still hard-wired with the storage controller. My wish would be to make the caching layer independent of the storage system. I am not entirely sure how we would approach that yet, though. I do have a crazy suggestion though which I have been playing around with in my head lately but it would be a bit drastic to say the least. Will post more about that soon.

Anyone up for reviving this? Next steps?

#3 woops

root@pubuntu:/var/www/d8/drupal# git apply --check entitycache_0.patch
error: patch failed: core/modules/comment/comment.module:116
error: core/modules/comment/comment.module: patch does not apply
error: patch failed: core/modules/entity/entity.module:69
error: core/modules/entity/entity.module: patch does not apply
error: core/modules/entity/lib/Drupal/entity/EntityController.php: No such file or directory
error: patch failed: core/modules/system/system.module:273
error: core/modules/system/system.module: patch does not apply

a fast look to all the changes already in repository says this implementation very different against repo one so not possible for a fast reroll

Related #1497374: Switch from Field-based storage to Entity-based storage.

Tagging.

Weird, I could swear the the tags list was empty when I added those tags...

So exactly how does this related to #597236: Add entity caching to core ?

Given #1497374: Switch from Field-based storage to Entity-based storage and #1969728: Implement Field API "field types" as TypedData Plugins we really need to replace field cache with entity cache.

Both of those are major, so raising this one to major as well.

How about adding the caching layer as a decorator around the storage controller? We would then basically have a CacheController (which wraps the storage controller) and implements the StorageControllerInterface. Said CacheController would get a cache backend injected and try to retrieve entries from it and upon cache miss forward to the decorated storage controller.

Edit: This would help us to decouple storage from caching and and thereby remove the need for every storage controller to implement caching separately.

Edit: This would help us to decouple storage from caching and and thereby remove the need for every storage controller to implement caching separately.

Indeed, this sounds clean and is straight-forward!

Going to work on this.

Actually, not right now!

Tagging.

The issue summary needs updating if there is remaining work here.

Just like I said years ago, I don't think it is realistic to replace the special "static" cache, it's more complicated than that. We do have persistent entity caching now, so I don't think this is still major.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -135,8 +153,11 @@ public function resetCache(array $ids = NULL) {
+    if ($this->entityType->isStaticallyCacheable()) {
+      foreach ($ids as $id) {
+        if ($cached = $this->cacheBackend->get($this->getCid($id)))
+          $entities[$id] = $cached->data;
+        }
     }

missing closing } ?

\Drupal\Core\Entity\EntityTypeManager::clearCachedDefinitions():

$this->handlers = [];

And some more cases as well. That invalidates the static cache (and injected definition) of those handlers. That doesn't work anymore now.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -73,18 +75,28 @@
    */
-  public function __construct(EntityTypeInterface $entity_type) {
+  public function __construct(EntityTypeInterface $entity_type, CacheBackendInterface $cache_backend = NULL) {
     $this->entityTypeId = $entity_type->id();
     $this->entityType = $entity_type;
     $this->idKey = $this->entityType->getKey('id');
     $this->uuidKey = $this->entityType->getKey('uuid');
     $this->langcodeKey = $this->entityType->getKey('langcode');
     $this->entityClass = $this->entityType->getClass();
+    if (!isset($cache_backend)) {
...
+    }
   }

I think this should read:

$this->staticCacheBackend = $cache_backend ?: \Drupal::service('cache.static')

(right now you never assign if the backend is actually injected)

Also, I was just thinking..

a) $cache_backend should probably be $static_cache_backend?
b) I know that default_backend is overridden by setting a global default cache backend. Means you have to explicitly specify fast chained backends when using e.g. redis as default backend. Also means that redis will be used for cache.static in that case, oops... I think default_backend should override the default, need to open an issue about that.

fail: [Other] Line 178 of core/tests/Drupal/KernelTests/Core/Command/DbDumpTest.php:
Drupal\KernelTests\Core\Command\DbDumpTest::testScriptLoad
Drupal\Core\Database\SchemaObjectExistsException: Table cache_static already exists.

This is a fun test fail. static isn't supposed to create a table. are kernel tests overriding something somewhere? looking into it.

Ok, that is easy, we just need to override that services as that test defaults to database backend.

Here's why the views test is failing and why this change is a performance problem and behavior change:

MemoryBackend::set():

  /**
   * {@inheritdoc}
   */
  public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array()) {
    assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($tags)', 'Cache Tags must be strings.');
    $tags = array_unique($tags);
    // Sort the cache tags so that they are stored consistently in the database.
    sort($tags);
    $this->cache[$cid] = (object) array(
      'cid' => $cid,
      'data' => serialize($data),
      'created' => $this->getRequestTime(),
      'expire' => $expire,
      'tags' => $tags,
    );
  }

The memory backend on purpose serializes the data to behave the same as non-memory backends and break references. That means this is an overhead and it also breaks the current behavior that two load calls for the same entity get the same object. You can argue whether that is good or bad, but we can't just change it. That's going to cause nasty bugs and side effects in code that currently works.

Specifically, what happens here is that we load the same node multiple times, then switch to the requested language. And then we save all of them. But that means that saving the second language will not have the changes of the first x, so saving that overrides the changes that the first language saved.

This issue looks a lot like #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset()

@Berdir pointed out in irc that $this->container is risky in tests since it can get out of sync. Switching to \Drupal fixes it.

Where are they out of sync? Is it in a test base class or is it in the entity system?

Could it be the same problem as this? #2641518-38: Replace deprecated usage of entity_create('config_test') with a direct call to ConfigTest::create()

Your patch modifies EntityReferenceIntegrationTest which uses config_test, and #2641518: Replace deprecated usage of entity_create('config_test') with a direct call to ConfigTest::create() addresses config_test, so maybe config_test needs some love.

ConfigTest::create() ends up being \Drupal::entityManager()->getStorage()->getEntityTypeFromClass(ConfigTest::class). So one of the proscribed ways of instantiating an entity relies on \Drupal and the injected container being in sync, but that behavior doesn't work under some circumstances, which we can't seem to figure out.

So I'll wager that the thing that's out of sync is within the entity API.

Here's a patch that fixes EntityReferenceIntegrationTest by having getTestEntities() use $this->container to build the fixture entities.

Follow-up: Why does this patch fail if you switch to using the Entity::create() pattern in getTestEntities(), or use \Drupal in the test?

Ugh. #96 passed locally.

1. +++ b/core/core.services.yml
   @@ -1639,3 +1639,5 @@ services:
   +  static_cache:
   +    class: Drupal\Core\Cache\StaticCache\StaticCache
   

   should we make this entity specific, so that you can have a LRU implementation for entities but maybe not for other things?

2. +++ b/core/lib/Drupal/Core/Cache/StaticCache/StaticCache.php
   @@ -0,0 +1,65 @@
   +    // Sort the cache tags so that they are stored consistently in the database.
   +    sort($tags);
   

   what database? :)

   I know, copied, but does make even less sense here than in the parent, should we probably drop teh array nique and sort calls?

3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -115,11 +133,12 @@ public function loadUnchanged($id) {
   -      $this->entities = array();
   +      // Call the backend method directly.
   +      $this->staticCache->invalidateTags(['entity_static_cache']);
   

   and this tag should then be entity type specific?, maybe a property that is defined in __construct(), so you could override it?

4. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
   @@ -50,6 +51,13 @@ class KeyValueEntityStorage extends EntityStorageBase {
      /**
   +   * The static cache.
   +   *
   +   * @var \Drupal\Core\StaticCache\StaticCacheInterface
   +   */
   +  protected $staticCache;
   

   duplicated?

5. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
   @@ -60,9 +68,12 @@ class KeyValueEntityStorage extends EntityStorageBase {
   +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('static_cache');
   +    parent::__construct($entity_type, $static_cache);
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -154,9 +155,12 @@ public function getFieldStorageDefinitions() {
   +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('static_cache');
   +    parent::__construct($entity_type, $entity_manager, $cache, $static_cache);
        $this->database = $database;
   

   we don't need to duplicate the fallback logic here?

Did some cleanup, added some @todos.

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceIntegrationTest.php
@@ -82,9 +82,7 @@ public function testSupportedEntityTypesAndWidgets() {
-      /** @var \Drupal\Core\Entity\EntityStorageInterface $storage */
-      $storage = $this->container->get('entity_type.manager')->getStorage($this->entityType);
-      $entity = current($storage->loadByProperties(['name' => $entity_name]));
+      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

@@ -110,7 +108,7 @@ public function testSupportedEntityTypesAndWidgets() {
-      $entity = current($storage->loadByProperties(['name' => $entity_name]));
+      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

@@ -121,7 +119,7 @@ public function testSupportedEntityTypesAndWidgets() {
-      $entity = current($storage->loadByProperties(['name' => $entity_name]));
+      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

@@ -175,8 +173,7 @@ public function testSupportedEntityTypesAndWidgets() {
-    $entity = current($this->container->get('entity_type.manager')->getStorage(
-    $this->entityType)->loadByProperties(['name' => $entity_name]));
+    $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));

However, I'm not too happy with this change, especially, with the fact that $this->container gets out-of-sync. I suspect that the problem might be in this patch. Probably entity cache gets out-of-sync?

Ouch!

bah wrong issue... sorry ignore my post.

Reroll, quite a few conflicts but mostly just on array syntax.

Meanwhile, the oups was funny :)

How does this allow me to clear a node from the static cache without also clearing it from the persistent cache?
Or better yet, load nodes without them being cached anywhere at all?

Use case: iterating over many nodes without running out of memory.

1. So like \Drupal::service('entity.static_cache')->reset(); or ->deleteAll(); or ->removeBin();?
It would be nice to have a more granular approach available.
Maybe change \Drupal\Core\Entity\ContentEntityStorageBase::buildCacheId() from protected to public? Then we'd be able to reset specific $cids.

2. Neat, hadn't seen the LRU ticket yet.

Few minor issues.

1. +++ b/core/core.services.yml
   @@ -539,6 +539,8 @@ services:
   +  entity.static_cache:
   +    class: Drupal\Core\Cache\StaticCache\StaticCache
   

   About the terminology. We used to use the term "static cache" because this cache is stored in static variables. But I don't think that the "static" word tells too much about the storage/backend. And this is a cache backend implementation. IMO, we need something that tells about the backend where the cache is stored and that would be "memory", so yes, "memory_cache" (and MemoryCache for class) would be more relevant.

2. +++ b/core/lib/Drupal/Core/Cache/StaticCache/StaticCache.php
   @@ -0,0 +1,61 @@
   +      'tags' => $tags,
   

   The parent class ensures that $tags contain unique values. Shouldn't we do the same here?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -104,9 +105,13 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
   +    // @todo Remove this line in Drupal 9.0.x and make $static_cache required.
   +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('entity.static_cache');
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -44,9 +45,13 @@
   +    // @todo Remove this line in Drupal 9.0.x and make $static_cache required.
   +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('entity.static_cache');
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -160,9 +161,13 @@ public function getFieldStorageDefinitions() {
   +    // @todo Remove this line in Drupal 9.0.x and make $static_cache required.
   +    $static_cache = isset($static_cache) ? $static_cache : \Drupal::service('entity.static_cache');
   

   This line is redundant. If the caller doesn't send the service instance, NULL is passed to the parent constructor. And the parent constructor knows how to deal with NULL.

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -104,9 +105,13 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
   +   * @param \Drupal\Core\Cache\StaticCache\StaticCacheInterface $static_cache
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -44,9 +45,13 @@
   +   * @param \Drupal\Core\Cache\StaticCache\StaticCacheInterface $static_cache
   
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -72,19 +73,38 @@
   +   * @param \Drupal\Core\Cache\StaticCache\StaticCacheInterface
   
   +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
   @@ -60,9 +61,11 @@ class KeyValueEntityStorage extends EntityStorageBase {
   +   * @param \Drupal\Core\Cache\StaticCache\StaticCacheInterface $static_cache
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -160,9 +161,13 @@ public function getFieldStorageDefinitions() {
   +   * @param \Drupal\Core\Cache\CacheBackendInterface $static_cache
   

   |null

5. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -115,11 +142,12 @@ public function loadUnchanged($id) {
   -        unset($this->entities[$id]);
   

   The protected property $entities should be also removed. And I think we can get rid also of the __sleep() implementation.

6. +++ b/core/modules/field/tests/src/Functional/EntityReference/EntityReferenceIntegrationTest.php
   @@ -83,9 +83,7 @@ public function testSupportedEntityTypesAndWidgets() {
   -      /** @var \Drupal\Core\Entity\EntityStorageInterface $storage */
   -      $storage = $this->container->get('entity_type.manager')->getStorage($this->entityType);
   -      $entity = current($storage->loadByProperties(['name' => $entity_name]));
   +      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));
   
   @@ -111,7 +109,7 @@ public function testSupportedEntityTypesAndWidgets() {
   -      $entity = current($storage->loadByProperties(['name' => $entity_name]));
   +      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));
   
   @@ -122,7 +120,7 @@ public function testSupportedEntityTypesAndWidgets() {
   -      $entity = current($storage->loadByProperties(['name' => $entity_name]));
   +      $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));
   
   @@ -176,8 +174,7 @@ public function testSupportedEntityTypesAndWidgets() {
   -    $entity = current($this->container->get('entity_type.manager')->getStorage(
   -    $this->entityType)->loadByProperties(['name' => $entity_name]));
   +    $entity = current(\Drupal::entityTypeManager()->getStorage($this->entityType)->loadByProperties(['name' => $entity_name]));
   

   Unrelated changes.

+++ b/core/lib/Drupal/Core/MemoryCache/MemoryCache.php
@@ -0,0 +1,63 @@
+namespace Drupal\Core\Cache\MemoryCache;

This won't work :)

I've found only nitpicks...

1. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/MemoryCache.php
   @@ -0,0 +1,63 @@
   +   $tags = array_unique($tags);
   

   Incorrect indentation.

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -101,6 +114,13 @@ public function getEntityType() {
   +   * Build a cache ID for an entity.
   

   Build or Builds?

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -72,19 +66,38 @@
   +    // @todo Remove this line in Drupal 9.0.x and make $memory_cache required.
   +    $this->memoryCache = isset($memory_cache) ? $memory_cache : \Drupal::service('entity.memory_cache');
   

   We can throw a silenced deprecation if $memory_cache is NULL so everyone knows to start passing in entity.memory_cache in D8.

2. We still have the needs profiling tag on the issue

3. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/MemoryCache.php
   @@ -0,0 +1,63 @@
   +    assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($tags)', 'Cache Tags must be strings.');
   

   Incorrect indentation.

4. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/MemoryCacheInterface.php
   @@ -0,0 +1,18 @@
   + * This has additional requirements over CacheBackendInterface and
   + * CacheTagsInvalidatorInterface. Objects stored must be the same instance when
   + * retrieved from cache, so that this can be used as a replacement for protected
   + * properties and similar.
   
   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -104,9 +105,11 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
   +  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager, MemoryCacheInterface $memory_cache = NULL) {
   +    parent::__construct($entity_type, $memory_cache);
   

   If we say that objects must be the same instance what does that mean for swapping this out with something like Redis? Ie. https://www.drupal.org/project/entitycache in D8. Does that mean once you've created the object all further retrievals for the same object must return the same instance?

Re #137.4:

If we say that objects must be the same instance what does that mean for swapping this out with something like Redis? Ie. https://www.drupal.org/project/entitycache in D8. Does that mean once you've created the object all further retrievals for the same object must return the same instance?

@alexpott, this is how the current implementation of the static cache behaves and I think there is code relying on this. I don't think that we could use Redis here, because the static entity cache is meant to be per request and not shared between requests. Additionally by using Redis we'll be losing the references, as for the Redis cache we have to serialize the data. @see \Drupal\redis\Cache\CacheBase::createEntryHash().

P.S. Redis can be used for the persistent entity cache.

#6. I wish these were unrelated changes, but there is a bug in the testing system where \Drupal::container() and $this->container are not always identical instances, which was causing test failures in this patch. Uploading two versions of the patch in case this has changed though.

It's not a bug in the testing system. It's an anti-pattern in how we do dependency injection, which is revealed by tests: #2066993-46: Use magic methods to sync container property to \Drupal::getContainer in functional tests

Way out of scope here, though.

Didn't know about weakref. Neat.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -95,7 +95,12 @@ public function __construct(EntityTypeInterface $entity_type, MemoryCacheInterfa
+      @trigger_error('The $memory_cache parameter was added in Drupal 8.6.x and will be required in 9.0.0.', E_USER_DEPRECATED);

Needs a link to the change record as per policy. I guess this issue needs one anyways.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -324,43 +328,10 @@ public function hasData() {
      /**
   -   * Gets entities from the static cache.
   -   *
   -   * @param array $ids
   -   *   If not empty, return entities that match these IDs.
   -   *
   -   * @return \Drupal\Core\Entity\EntityInterface[]
   -   *   Array of entities from the entity cache.
   -   */
   -  protected function getFromStaticCache(array $ids) {
   -    $entities = [];
   -    // Load any available entities from the internal cache.
   -    if ($this->entityType->isStaticallyCacheable() && !empty($this->entities)) {
   -      $config_overrides_key = $this->overrideFree ? '' : implode(':', $this->configFactory->getCacheKeys());
   -      foreach ($ids as $id) {
   -        if (!empty($this->entities[$id])) {
   -          if (isset($this->entities[$id][$config_overrides_key])) {
   -            $entities[$id] = $this->entities[$id][$config_overrides_key];
   -          }
   -        }
   -      }
   -    }
   -    return $entities;
   -  }
   -
   -  /**
   -   * Stores entities in the static entity cache.
   -   *
   -   * @param \Drupal\Core\Entity\EntityInterface[] $entities
   -   *   Entities to store in the cache.
   -   */
   -  protected function setStaticCache(array $entities) {
   -    if ($this->entityType->isStaticallyCacheable()) {
   -      $config_overrides_key = $this->overrideFree ? '' : implode(':', $this->configFactory->getCacheKeys());
   -      foreach ($entities as $id => $entity) {
   -        $this->entities[$id][$config_overrides_key] = $entity;
   -      }
   -    }
   

   Really nice to see this disappear. I'm a bit torn about adding entity.memory_cache to the constructor since Config doesn't support it atm but I think keeping Config and Content entity storages aligned seems like a good idea. The other option would be to introduce a setter on EntityStorageBase but that feels more risky. Maybe someone somewhere has enabled static caching of config entities.

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -101,6 +119,13 @@ public function getEntityType() {
   +  /**
   +   * Builds a cache ID for an entity.
   +   */
   +  protected function getCacheId($id) {
   +    return 'entity_storage_cache:' . $this->entityTypeId . ':' . $id;
   +  }
   

   This versus \Drupal\Core\Entity\ContentEntityStorageBase::buildCacheId() is confusing. Shall we pull buildCacheId() up to StorageBase() so it's consistent?

3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -132,11 +158,15 @@ public function resetCache(array $ids = NULL) {
   -  protected function getFromStaticCache(array $ids) {
   +  protected function getFromMemoryCache(array $ids) {
   
   @@ -147,9 +177,11 @@ protected function getFromStaticCache(array $ids) {
   -  protected function setStaticCache(array $entities) {
   +  protected function setMemoryCache(array $entities) {
   

   Are we sure about changing these names - might some custom / contrib entity storages rely on these?

4. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -541,16 +573,4 @@ public function getAggregateQuery($conjunction = 'AND') {
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function __sleep() {
   -    // In case the storage is being serialized then we prevent from serializing
   -    // the static cache of entities together with it, as this could lead to a
   -    // memory leak.
   -    $vars = parent::__sleep();
   -    unset($vars['entities']);
   -    return $vars;
   -  }
   

   Really nice to see this go. I think it shows this change is in the right direction.

1. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/MemoryCache.php
   @@ -0,0 +1,63 @@
   +   * @param bool $allow_invalid
   +   *   (optional) If TRUE, cache items may be returned even if they have expired
   +   *   or been invalidated.
   ...
   +  protected function prepareItem($cache, $allow_invalid) {
   

   This parameter doesn't seem to be optional.

2. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/MemoryCache.php
   @@ -0,0 +1,63 @@
   +    $cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= $this->getRequestTime();
   

   We could use static::CACHE_PERMANENT here instead. And then the use statement at the top can be removed as well.

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -104,9 +105,11 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
   +   * @param \Drupal\Core\Cache\MemoryCache\MemoryCacheInterface|null $memory_cache
   +   *   The static cache backend.
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -44,9 +45,11 @@
   +   * @param \Drupal\Core\Cache\MemoryCache\MemoryCacheInterface|null $memory_cache
   +   *   The static cache backend.
   
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -72,19 +66,43 @@
   +   * The static cache.
   +   *
   +   * @var \Drupal\Core\Cache\MemoryCache\MemoryCacheInterface
   +   */
   +  protected $memoryCache;
   ...
   +   * @param \Drupal\Core\Cache\MemoryCache\MemoryCacheInterface|null
   +   *   The static cache.
   

   "The memory cache..."

4. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -72,19 +66,43 @@
   +    $this->memoryCache = $memory_cache;
   +    $this->memoryCache = isset($memory_cache) ? $memory_cache : \Drupal::service('entity.memory_cache');
   

   The second line needs to go :)

5. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueEntityStorage.php
   @@ -3,6 +3,7 @@
   +use Drupal\Core\Cache\StaticCache\StaticCacheInterface;
   
   @@ -60,9 +61,11 @@ class KeyValueEntityStorage extends EntityStorageBase {
   +   * @param \Drupal\Core\Cache\StaticCache\StaticCacheInterface $static_cache
   +   *   The static cache.
   ...
   +  public function __construct(EntityTypeInterface $entity_type, KeyValueStoreInterface $key_value_store, UuidInterface $uuid_service, LanguageManagerInterface $language_manager, StaticCacheInterface $static_cache = NULL) {
   +    parent::__construct($entity_type, $static_cache);
   
   +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
   @@ -5,6 +5,7 @@
   +use Drupal\Core\Cache\StaticCache\StaticCache;
   
   @@ -133,7 +134,7 @@ protected function setUp() {
   +    $this->entityStorage = new ConfigEntityStorage($entity_type, $this->configFactory->reveal(), $this->uuidService->reveal(), $this->languageManager->reveal(), new StaticCache());
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Cache\StaticCache\StaticCache;
   
   @@ -378,7 +379,7 @@ public function testOnEntityTypeCreate() {
   +      ->setConstructorArgs(array($this->entityType, $this->connection, $this->entityManager, $this->cache, $this->languageManager, new StaticCache()))
   
   @@ -1123,7 +1124,7 @@ protected function setUpEntityStorage() {
   +    $this->entityStorage = new SqlContentEntityStorage($this->entityType, $this->connection, $this->entityManager, $this->cache, $this->languageManager, new StaticCache());
   
   @@ -1198,7 +1199,7 @@ public function testLoadMultipleNoPersistentCache() {
   +      ->setConstructorArgs([$this->entityType, $this->connection, $this->entityManager, $this->cache, $this->languageManager, new StaticCache()])
   
   @@ -1250,7 +1251,7 @@ public function testLoadMultiplePersistentCacheMiss() {
   +      ->setConstructorArgs([$this->entityType, $this->connection, $this->entityManager, $this->cache, $this->languageManager, new StaticCache()])
   
   @@ -1305,7 +1306,7 @@ public function testHasData() {
   +    $this->entityStorage = new SqlContentEntityStorage($this->entityType, $database, $this->entityManager, $this->cache, $this->languageManager, new StaticCache());
   
   +++ b/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php
   @@ -2,6 +2,7 @@
   +use Drupal\Core\Cache\StaticCache\StaticCache;
   
   @@ -94,6 +95,7 @@ protected function setUp() {
   +      ->setConstructorArgs(['role', new StaticCache()])
   

   There are quite a few places that need to be updated to use MemoryCache instead.

1. +++ b/core/lib/Drupal/Core/Cache/MemoryCache/MemoryCache.php
   @@ -0,0 +1,62 @@
   +   * @param bool $allow_invalid
   +   *   (optional) If TRUE, cache items may be returned even if they have expired
   +   *   or been invalidated.
   ...
   +  protected function prepareItem($cache, $allow_invalid = NULL) {
   

   I think the intended default value for this param is FALSE? Should also be mentioned at the end of the docblock: "Defaults to FALSE."

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -104,9 +105,11 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
   +   *   The memory cache backend.
   

   Here and a few other places: if we call it "the memory cache *backend*", it can be easily confused with \Drupal\Core\Cache\MemoryBackend.

   But that's already the case, so we should have a followup to clean up this naming problem.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -978,7 +982,7 @@ public function loadUnchanged($id) {
   -        $this->setStaticCache($entities);
   +        $this->setMemoryCache($entities);
   

   This change needs to be reverted as well.

4. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -72,19 +66,42 @@
   +   * The memory cache cache tag.
   +   *
   +   * @var string
   +   */
   +  protected $cacheTag;
   

   Maybe this should be renamed to memoryCacheTag?

5. +++ b/core/modules/comment/src/CommentStorage.php
   @@ -59,7 +59,8 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   +      $container->get('entity.memory_cache'),
   

   Removing this comma should show us some real results from the testbot :)

Cleaned up the rest of the places that were triggering the deprecation warnings, as well a couple of coding standard issues.

Getting there :)

Third time is the lucky one?

I would RTBC but still needs profiling.

I spent some time profiling this patch. Here is what I came up with using the following conditions:

• Content was created with devel_generate with the article content type in the standard profile.
• Both tests were a cold cache.
• Tests was hitting the script in profiling-script.patch, for 500 nodes load each one 1000 times.

The results were:

• #1596472: HEAD
• #1596472: Patch #157 applied

I don't have much experience analysing these results, but the initial outlook shows the patch being quicker and using less memory.

I thought loading the node a bunch of times would help highlight any performance regressions, but I suppose the script may have inadvertently timed out or the reporting isn't perfect for a really large number of calls. I might retest this and make sure the calls to ::load at least equal out across tests.

New profiling results:

• #1596472-163: HEAD 1000 loads
• #1596472-163: Patch #157 applied 1000 loads

Loading a single node twice truncated the function calls to actually load the node off the report, so I bumped it up to make sure they appeared. I can confirm we're calling load 1000 times in both sets of results at least.

I agree with @catch on #161. Also the results, how significant they would be, are not showing to much difference. I see all other concerns were addressed.

The patch looks great. Just wondering whether or not...

+++ b/core/lib/Drupal/Core/Field/BaseFieldOverrideStorage.php
@@ -26,9 +27,11 @@ class BaseFieldOverrideStorage extends FieldConfigStorageBase {
+  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager, FieldTypePluginManagerInterface $field_type_manager, MemoryCacheInterface $memory_cache) {

+++ b/core/modules/comment/src/CommentStorage.php
@@ -43,9 +44,11 @@ class CommentStorage extends SqlContentEntityStorage implements CommentStorageIn
+  public function __construct(EntityTypeInterface $entity_info, Connection $database, EntityManagerInterface $entity_manager, AccountInterface $current_user, CacheBackendInterface $cache, LanguageManagerInterface $language_manager, MemoryCacheInterface $memory_cache) {

+++ b/core/modules/field/src/FieldConfigStorage.php
@@ -56,9 +57,11 @@ class FieldConfigStorage extends FieldConfigStorageBase {
+  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager, EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_manager, DeletedFieldsRepositoryInterface $deleted_fields_repository, MemoryCacheInterface $memory_cache) {

+++ b/core/modules/field/src/FieldStorageConfigStorage.php
@@ -66,9 +67,11 @@ class FieldStorageConfigStorage extends ConfigEntityStorage {
+  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, FieldTypePluginManagerInterface $field_type_manager, DeletedFieldsRepositoryInterface $deleted_fields_repository, MemoryCacheInterface $memory_cache) {

+++ b/core/modules/shortcut/src/ShortcutSetStorage.php
@@ -36,9 +37,11 @@ class ShortcutSetStorage extends ConfigEntityStorage implements ShortcutSetStora
+  public function __construct(EntityTypeInterface $entity_info, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager, MemoryCacheInterface $memory_cache) {

we should do MemoryCacheInterface $memory_cache = NULL on all of these just in case contrib / custom has extended one of these. They will get the deprecation messages (once make it easier for people to test this outside of core) but at least nothing with break for 8.6.x.

Discussed #165 with @catch yesterday. @catch pointed out that as storages are constructed by their own createInstance method if someone has overridden the constructor they are going to need to change it anyway. So we can ignore #165.

My only other question is why...

+++ b/core/modules/user/tests/src/Functional/Update/UserUpdateOrderPermissionsTest.php
@@ -31,6 +31,7 @@ public function testPermissionsOrder() {
     $this->runUpdates();
+    \Drupal::service('entity.memory_cache')->reset();

... do we need to do this?

@alexpott, that's because UserUpdateOrderPermissionsTest::testPermissionsOrder() is using the entity API to load a user role before running the updates, so the role entity objects ends up in the static cache.

However, we don't support using any API functions in update tests before running the update, so here's an alternative fix for that problem.

I managed to screw up the patch somehow... This one should be better, it's #157 with the interdiff from #167.

@amateescu: +1 on that fix. That said, the role was in the static cache before as well, so something isn't quite transparent compared to before. The difference is that runUpdates() calls $needs_updates = \Drupal::entityDefinitionUpdateManager()->needsUpdates();, which itself calls useCaches(FALSE)/useCaches(TRUE), which has the side effect of dropping any entity handlers including their static caches.

Can we keep that behavior for the new approach as well? I've actually used those methods in projects before exactly for that purpose (clearing the entity static cache), so if that doesn't work anymore then those projects would get into troubles until the the memory cache includes an LRU or some other fixed-size/purging mechanism.

@amateescu my question is really how come HEAD passes - roles are being statically cached there too.

@Berdir ah yeah... that's what I was asking - thanks!

@Berdir, very good point! Let's not break BC unnecessarily.

Updating review credits. Crediting @pounard, @fago, @alexpott, @fubhy, and @hchonov for comments that affected the patch or made contributions to the discussion.

Committed d8029d7 and pushed to 8.6.x. Thanks!

Yay! #2924218: Clearing the persistent entity cache every time we switch between workspaces is super wasteful is now blocked only on #2928888: Add a hook_entity_preload() for modules that need to load a different revision than the default one :)

+ assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($tags)', 'Cache Tags must be strings.');

Makes it fail on 7.2.x with assert(): Calling assert() with a string argument is deprecated, see:
https://www.drupal.org/pift-ci-job/978615
https://www.drupal.org/pift-ci-job/978614

Committed 1e37fc8 and pushed to 8.6.x. Thanks!

I just pushed a follow-up quick fix by copying \Drupal\Core\Cache\ApcuBackend::set()

Yikes, haven't attached the patch to my comment on purpose because it felt wrong piggybacking like that on a 6 year old awesome issue I had no part in ;).

@tacituseu it felt more important to just fix it. And since you reported I credited you on the commit but not this issue.

Since storage handlers are basically free to choose their own memory cache backend, shouldn't EntityStorageInterface be complemented with a ::getStaticCache() method or at least have a ::resetStaticCache() method? Same requirement applies for the persistent cache too.

Attaching a patch which can be applied to 8.5, as this might be urgent for production.

As a workaround on 8.5.x, you can use $entity_type_manager->useCaches(FALSE) and then again useCaches(TRUE), that resets the entity handlers, you can get a new one and it will have a reset static cache (that assumes that it won't be injected somewhere, but it will at most have the double static cache if you're doing this in a long loop.

Not opposed to having a method on the storage for resetting a single entity type's static cache, I'd suggest you open an issue.

#186 Thanks a lot, you saved my day.