Enable static caching for config entities

Tagging

#1596472: Replace hard coded static cache of entities with cache backends would make it easier to swap out different entity cache controllers.

Here and in #1919322: entity_load_unchanged() should be part of the storage controller we have repated code between database and config storage controllers. Would it make sense to have an abstract BaseStorageController as a common ancestry?

Yes please. EntityStorageControllerBase would be a more correct name.

Yes please. EntityStorageControllerBase would be a more correct name.

@sun suggested to address this in #1893772: Move entity-type specific storage logic into entity classes.

Reroll.

Fun fun fun. A lot of that is views, and I have some ideas as to why.

Cross-linking #1596472: Replace hard coded static cache of entities with cache backends.

I have some more Views fixes to post, but I found a big part of the problem with the tests.

During SimpleTest runs (but not page loads), EntityManager is recreated almost every time the plugin.manager.entity service is retrieved.

That means that the property containing all the controllers is reset, and each time a new storage controller will be used.
Which means that the $entityCache for each storage controller is different, and will lead to different results.

Who knows why retrieving a service gives you a new object only during tests?

Because modules are constantly being enabled/disabled, and the container rebuilt?

This is within a single test method.

Looking at:
http://api.drupal.org/api/drupal/core%21modules%21shortcut%21lib%21Drupa...

Between the $set = $this->set; and the #this->drupalPost(), EntityManager is recreated three times.

Unless I'm just losing it, which may be possible.

#1978870: Add an EntityStorageControllerBase

Tagging.

I think we should postpone this on #21 for now. As we can reuse cache related methods from there.

Here's a reroll now we've got that base class. I will also write some tests for this.

We discussed in Portland that static caching on this level is problematic because the config might return different information based on the config context, e.g. language.

So we either can't have a static cache or we need to care about the config context as part of the cache key. We need to check how big the overhead is of creating config entities.

Hmm, good point. Why aren't things ever easy.. :)

So maybe we would need to do something like this? Not sure if it will reap many benefits from a performance point of view tbh.

In most scenarios does the UUID not change I guess (same language, domain, ...), so it should bring something.

Will do some profiling after #1971158: Follow-up: Add loadMultiple() and listAll() caching to (cached) config storage is in.

Yeah, I guess even it is is a bit better, it could be worth it, who knows. Let's wait for that issue. We don't want too many layers of static caching either I guess.

I guess most of those test fails now just need some static cache clears.

Reminder for the next patch: It would be good if getCurrentContextUuid() actually returned something.

Related: #2073297: Remove cache of Field / FieldInstance objects

Updated issue summary.

Ok, so here's another reason why we need this: #2095283: Remove non-storage logic from the storage controllers. That's how often we load roles, up to 4k times in a single test.

Re-roll.

When I was working on this patch before there were issues with forms and clearing of the static cache. Hopefully this has been helped by the work to avoid serialising objects in the form cache etc... That was basically blocking it before iirc.

@damiankloip: There is no issue here :)

Those fails just prove that this works. That's what you get when you combine static caches and change the data in them through form submits. There is only one solution and that's manually clearing them (well, the other alternative would be to clear all entity caches on every drupalPost()). This patch starts doing that, still quite a lot to go, but this should be something that someone with less experience can do, I'll try to get @vladan.me on it, he started to work on a lot of entity issues last week.

Hopefully, this should fix all fails except ViewPageControllerTest, I am not quite sure right now what problem is, test is giving this as description "The test did not complete due to a fatal error."

44: 1885830-44.patch queued for re-testing.
Just to make sure that test results are correct.

Fixing the path and views handler test.

No idea about the other one, that one is interesting...

I couldn't find also cause of last one so I didn't upload those two fixes (had them though). What could cause such error, doesn't seem related at all...

Here's the fix that @dawehner found: http://paste.ubuntu.com/6418465/

Care to roll a patch with that included?

I think we just want the second change and not the first.

Can you be more specific damiankloip? Just to use this?

-      $executable = $view->getExecutable();
+      $executable = static::executableFactory()->get($view);

Yes, exactly that :)

I just tested this out locally, and the test doesn't actually pass with the first change added too. So I guess it was the right hunch.

Uploading new patch, patch is rerolled because of #2134951, also, made change noted in comment above, no interdiff included.

I just tested this out locally, and the test doesn't actually pass with the first change added too. So I guess it was the right hunch.

If we don't clone the object, we end up in manipulating the same view object somehow.

@berdir here comes a question. Let's assume you have the following code in two different blocks:

class BLock1 {
  public function build() {
    $article = $this->entityStorage->load('article');
    $article->label .= ' Elephant';
    return array('#markup' => $article->label());
    // NO SAVE
  }
}

class BLock2 {
  public function build() {
    $article = $this->entityStorage->load('article');
    $article->label .= ' Dolphin';
    return array('#markup' => $article->label());
    // NO SAVE
  }
}

Both of these pieces of code ran, ... should we not try to clone the cached entity, so we don't have conflicting code at all?

55: 1885830-55.patch queued for re-testing.

I don't know, but that's how it's been for nodes/content entities since the static cache for nodes was added in D6...

Re-roll.

Just another reroll.

63: 1885830-63.patch queued for re-testing.

Another reroll.

Let's look at what is still failing.

And with the config context stuff now removed.

Let's see how far this gets us.

We don't need to override those methods anymore at all, we can just use what's provided by EntityStorageControllerBase for that.

And what if code like user_mail() would load config entities and not raw config?

We will need a test for that somewhere...

I haven't looked at the changes at all yet, but according to @alexpott, there's a way to get the cache key from the config factory that could work as a cache separator.

The method for that would be getCacheKey(), but that is actually flawed, as described in #2177739: Fix inefficient config factory caching, because it can not possibly consider module overrides.

And we do have an example for this in core, just not enough test coverage apparently, see Drupal\Core\Datetime\Date::formatDate()

78: 1885830-78.patch queued for re-testing.

Re-roll, bunch of context conflicts.

back to the same 5 failures then...

Fixes the textfieldtest

1. Well, it just makes it as bad as it always was for content entities. Sure, we could consider to loop over all defined entity types and clear their static caches, but what we can not do is just do it for those that were actually used, so we'd have to initially a lot of storage controllers just to empty the obviously empty static cache :) Unless we introduce a new method on the entity manager...

We would also have to only do it for config entities IMHO, because it would be a bit unfortunate for content entities as soon as #597236: Add entity caching to core lands, as that would have to cache tag clears all the time.

See also my latest comment in that issue (https://drupal.org/comment/8541147#comment-85411479), clearing the caches all the time would hide that pretty serious bug, so I'd prefer to live with the worse DX. I think a lot of people that have written tests in 7.x are already pretty used to entity and drupal_static() caches interfering with tests...

2. Hm, that does look strange, but I think we just went through test fails and fixed them? Sounds like that can either be removed or would be a cache clear bug ?

Re-roll.

#91.2 was in fact an actual bug, we didn't clear the cache for renamed entities. This should address that.

Did some micro-benchmarks:

use Drupal\Component\Utility\Timer;

Timer::start('cache');

for ($i = 0; $i < 10000; $i++) {
  entity_load('node_type', 'article');
}

print (Timer::stop('cache')['time']) . "ms\n";

HEAD: ~650ms
Patch: ~180ms

=> ~3.5 times faster.

Question is, how much does it affect and actual page load, that's harder to answer. I did some basic tests, but on a frontpage with two nodes and a custom block, we apparently didn't have a single static cache hit. #2211723: FieldInstance::__construct() loads all field_config entities for example is a bigger problem right now. Not sure if that would change if we'd no longer kept cached entities in FieldInfo.

Based on that, this is probably not worth the effort at the moment, unless someone is able to provide different numbers?

Forgot the interdiff.

Latest profiling by @msonnabaum suggests that this would really help. He noticed us loading up the list of roles many many times for access checks. Anyone up for revisiting this?

Interesting, I did expect that to show up as well, but wasn't able to see that, care to share what/how you profiled?

For the specific case of permissions/roles, @alexpott also opened an issue to cache the list of permissions in User:.hasPermission(), we used to have built-in caches there which were all dropped.

It saw it when profiling a view that listed a large number of nodes/users. I forget where they were coming from, but the calls were to UserSession::hasPermission, which fetches roles each time.

Ah that makes sense, access checks for operations mostly I guess...

One thing that I discussed with @fago in Szeged was that was could provide the functionality but not enable it by default for config entities, we can do that very easily now with @ConfigEntityType.

Then config entity types that are loaded a lot can explicitly enable it and benefit, without having to do so for the dozen or more types that are not used that frequently.

Still for permission checks, even just looping over the roles and their permissions might be more overhead than we want, so we might want to do #2202185: Statically cache Role entities to speed up (User|UserSession)::hasPermission() anyway, and then I'm not sure if we want this to be enabled for roles?

It doesn't hurt for it to be enabled for roles, but I agree it's worth doing 2202185 if for no other reason than it's a code improvement.

I'd much prefer that static caching we're opt-out. Historically, opt-in results in people not knowing about it and then performance issues popping up that are only found when profiling (this happened with ctools plugins in contrib).

The problem with opt-out can be seen in the current patch, if we enable it for all types on core, then we run into all those test issues that we need to fix, and manually disabling it for them will probably result in people copying that without thinking about it :)

Not enabling in this case also only means we just fall through to the next static cache, it's not like we need to execute database queries or something, so it's not *that* bad as long as we don't call it as often as roles can be.

Might also be useful to compare the memory usage when enabling it.

Note that I'm fine with moving forward with this, just want to make sure that we are sure that it's worth it and do it in a way that doesn't result in worse DX than it needs to be (static caches in web tests are the pain, it took us forever to get this green).

Needs a bit of a reroll with the getStorageController > getStorage change.

I think I would definitely like to see this opt-out too. I basically agree totally with #101.

I also agree that this current patch is pretty cumbersome. The amount of ad-hoc cache clears needed is obviously not ideal. If we were to use a real storage backend (memory backend), we could always just use a null backend in tests or something. This is more difficult all the time we have baked in logic like this. However, we discussed this a while ago (in Prague) and using a 'proper' backend/backend chain right would be very difficult to implement, and possibly controversial.

we could just try something like this?

reset the container in post/postForm. Otherwise, we could just reset controllers on the entity manager. That seems a little special snowflake like though.

Note that #2225955: Improve the DX of writing entity storage classes will do #100 because static cache will then be supported for all entity storage implementations that use the default methods.

104: 1885830-104.patch queued for re-testing.

Re-roll.

- Almost no changes are now necessary because most of the static cache handling is now in the base class, also moved the one for deletion.
- Removed the by default disabled static cache flag for config entities but I still think this makes sense to do, just want to see what the testbot has to say. There might be a few that can use it, but most don't because we don't load them that much and then it's just unnecessary memory being used.
- I also still think that we need to correctly check the cache key for a given object or we will have problems with overriden config.

The method for that would be getCacheKey(), but that is actually flawed, as described in #2177739: Fix inefficient config factory caching, because it can not possibly consider module overrides.

Looks like that issue is now fixed, but from what I can tell, subsequent patches here haven't incorporated the call to getCacheKey(). So, I did so in #2303881: Config entity static cache doesn't get reset and isn't override aware.

In #2303881-6: Config entity static cache doesn't get reset and isn't override aware, Berdir asked me:

Would also appreciate your thoughts in #1885830 in regards to enabling it by default vs. only doing it for config entities where we know it's worth it, like roles.

So here's my thoughts:

• Caching is about spending memory to purchase speed.
• Since we already have static caching on the raw config arrays, the speed gains in also static caching the config entities on top of that is minimal: (constructor, postLoad(), hook_ENTITY_TYPE_load(), and hook_entity_load()) so long as there aren't expensive implementation of hook_entity_load().
• However, we don't know what contrib will add. If contrib adds an expensive hook_entity_load() implementation, then static caching the entities is worthwhile.
• We could answer the above by saying that core shouldn't statically cache config entities by default, and let contrib alter that decision when it implements an expensive hook_entity_load(). But, I have some concerns that if we don't cache by default, then when contrib tries to turn caching on, it will find errors that we didn't catch in our integration tests due to not having that be the default.
• So, maybe it would be best to cache by default? But if we're concerned about redundant memory usage, would it make sense to somehow tell ConfigFactory to not statically cache the raw config arrays for the config entities that we cache?
• Or, is the memory usage of config arrays so tiny that it's not worth the bother to optimize away the redundancy?

effulgentsia: The double-caching seems unnecessary to me. If we cache the object but NOT the array, wouldn't that get us the CPU savings we're talking about without a significant increase in memory usage? (I'm assuming that the array takes up the bulk of the memory of the object, which seems a safe assumption.)

Yes, I think for config entities, caching the entities and not the underlying config array makes the most sense. But ConfigFactory will still need to cache the non-entity config arrays, so this would add some complexity in terms of ConfigFactory knowing (or being told) which config arrays are non-entities and which are entities (or alternatively, which individual config IDs or ID patterns to cache/not-cache). I'm sure that's a solveable problem: just in general, while it's ok for the ConfigEntity system to depend on the Config system, we'd rather not have the Config system knowing too much about the ConfigEntity system.

Agreed entirely on the last point. Is there some way for ConfigEntity to tell Config, when loading data, "don't cache this one, I got it"? (In some generic way, of course.)

No, at the moment, there is not, and I also don't think that would be a good idea, config entity queries work on the raw data AFAIK, we want those to rely on the static cache too.

The actual bugs related to config entity caching have been fixed in the related issue, it is being enabled for roles in #2202185: Statically cache Role entities to speed up (User|UserSession)::hasPermission().

In the test fails that we had here, we AFAIK had no real bugs (other than those that were fixed now), only the usual test static cache that had to be cleared after web requests.

The question to ask is IMHO what config entities we expect to load multiple times. And enable static caching for those. Some are only expected to be loaded once or a few times on normal pages, so it does't matter too much if a hook_entity_load() is slow or not.

Just a note that if we start statically caching EntityViewDisplays, then EntityViewBuilder::viewField() will need to be modified to clone the EVD it loads and modifies...

Not sure yet, but profiling showed some some config entity types being loaded a lot, although entity displays actually are the worst and based on what @yched said, we might need to clone them anyway. Or maybe we can improve how they're loaded, seeing a lot of calls originating in views in my case.

profiling showed some some config entity types being loaded a lot, although entity displays actually are the worst

Like, we load the same entity display over and over in the same request ? Weird, I wouldn't have expected that. I wrote #117 "just in case", but I didn't think those would be the primary config entity that needs optim.

Yeah, I think it's if you have views that render fields, then it creates it for every row. Not sure, will need to check again.

@Berdir: oh right. Can't find it atm but we have an issue open with a plan to make "by field" views less horribly inefficient. Been opened since Portland I think, but we didn't get to it yet :-/

Yeah, and that is actually called by EntityViewController, so we do the collecting twice per request for a normal entity view :( And I hope that was breaking most of those tests, was just testing one of them.

Anyway, cloned directly in collect, because we have that alter there, so we need to do it anyway I guess?

Yeah, that $entity->title->view('full') in EnityViewController is weird anyway, we should be passing explicit hardcoded formatter settings there, and not rely on the configuration of the 'full' Display. @jhodgdon opened an issue for that, was stuck on in-place-edit tests breaking.

Anyway, yeah, cloning there makes sense I guess.

EntityStorageBase::delete() assumes that entities are keyed by ID, but we never enforce it. Automatically deleted field storage configs are not, for example.

Adding a note due to discussion on #2406543: Remove ConfigFactory::setOverrideState and ConfigFactory::getOverrideState(). This issue will also need to prevent the config entities from being cached in the config factory. I think the best implementation will be a parameter on the get() and loadMultiple() methods on the ConfigFactory. However this is not simple as @Berdir points out:

But agreed that for those that we do cache statically, we should not cache them twice. Which is more complicated than it sounds I think, maybe the entity storage class could tell the factory to not cache what it is loading, but there is also the entity query implementation that afaik works on raw config values, so that would make entity queries possibly slower.

This is no longer relevant. Config objects are statically cached.

No, they still default to not caching the entity objects statically? It's supported but opt-in. This is about enabling it by default, which I'm not sure we can do, but we could decide if we can explicitly update at least some core entity types?

There's also a core issue to treat entity static cache in tests like other static caches and reset them on POST requests, that removes the need for 95% of the manual cache resets in tests.