I was testing a big upgraded with lots of fields (290 fields and 500 instances or so, 1600 yml files in total with entity displays, image styles and other things like that).

One major performance issue that I noticed (apart from 300+ loaded config files, duplicated to 600 by locale.module) was that glob() in buildQuery() gets really slow with that amount of configuration.

I displayed a simple view on the frontpage and glob() took up almost 10% of the whole request time, with 100 calls to buildQuery().

CommentFileSizeAuthor
#99 1971158-deleteTags-99.patch2.04 KBAnonymous (not verified)
#92 1971158-deleteTags-92.patch1.67 KBalexpott
#90 1971158-deleteTags.patch1.54 KBAnonymous (not verified)
#84 81-84-interdiff.txt761 bytesalexpott
#84 config-load-multiple-1971158-84.patch23.8 KBalexpott
#81 79-81-interdiff.txt4.3 KBalexpott
#81 config-load-multiple-1971158-81.patch23.79 KBalexpott
#79 config-load-multiple-1971158-79.patch23.39 KBBerdir
#79 config-load-multiple-1971158-79-interdiff.txt638 bytesBerdir
#77 config-load-multiple-1971158-77.patch22.77 KBBerdir
#77 config-load-multiple-1971158-77-interdiff.txt732 bytesBerdir
#75 config-load-multiple-1971158-75.patch22.77 KBBerdir
#73 config-load-multiple-1971158-73.patch22.6 KBBerdir
#73 config-load-multiple-1971158-73-interdiff.txt3.5 KBBerdir
#65 config-load-multiple-1971158-65.patch19.84 KBBerdir
#65 config-load-multiple-1971158-65-interdiff.txt732 bytesBerdir
#63 config-load-multiple-1971158-63.patch19.84 KBBerdir
#63 config-load-multiple-1971158-63-interdiff.txt3.29 KBBerdir
#58 57-58-interdiff.txt1.61 KBalexpott
#58 config-load-multiple-1971158-58.patch19.8 KBalexpott
#57 config-load-multiple-1971158-57.patch19.62 KBalexpott
#57 55-57-interdiff.txt749 bytesalexpott
#55 53-55-interdiff.txt2.12 KBalexpott
#55 config-load-multiple-1971158-55.patch19.29 KBalexpott
#53 config-load-multiple-1971158-53.patch16.85 KBBerdir
#53 config-load-multiple-1971158-53-interdiff.txt2.43 KBBerdir
#48 config-load-multiple-1971158-48.patch17.08 KBBerdir
#48 config-load-multiple-1971158-48-interdiff.txt3.09 KBBerdir
#46 config-load-multiple-1971158-46.patch16.69 KBAnonymous (not verified)
#45 config-load-multiple-1971158-45.patch16.69 KBAnonymous (not verified)
#41 config-load-multiple-1971158-41.patch16.57 KBAnonymous (not verified)
#38 config-load-multiple-1971158-38.patch15.4 KBAnonymous (not verified)
#33 config-load-multiple-1971158-33.patch16.27 KBAnonymous (not verified)
#30 config-load-multiple-1971158-30.patch12.79 KBBerdir
#30 config-load-multiple-1971158-30-interdiff.txt1.68 KBBerdir
#27 config-load-multiple-1971158-27.patch11.9 KBBerdir
#27 config-load-multiple-1971158-27-interdiff.txt1022 bytesBerdir
#25 config-load-multiple-1971158-25.patch11.37 KBBerdir
#25 config-load-multiple-1971158-25-interdiff.txt471 bytesBerdir
#20 config-load-multiple-1971158-20.patch10.91 KBBerdir
#20 config-load-multiple-1971158-20-interdiff.txt765 bytesBerdir
#18 1971158-replace_slow_glob.patch1.79 KBamateescu
#15 config-load-multiple-1971158-15.patch10.76 KBBerdir
#15 config-load-multiple-1971158-15-interdiff.txt2.63 KBBerdir
#13 config-load-multiple-1971158-13.patch10.73 KBBerdir
#13 config-load-multiple-1971158-13-interdiff.txt2.61 KBBerdir
#11 config-load-multiple-1971158-11.patch9.55 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Can you identify why do we run a query on configuration entities here to begin with? This sounds like something you should only need to do rarely, and only in an admin context.

Berdir’s picture

I think buildQuery() is just called like that because that's also how the method is called in the database storage controller, it's called from load().

Wondering about that, why do we need this in the first place? Maybe it was necessary when we had to support $conditions but now, we can simply loop over the id's and do a config($prefix . $id) on them, no?. @yched suggested in #1880766: Preload configuration objects that we'd add support for loading multiple config files at once, mostly because we could then also do a getMultiple() against the config storage cache.

Berdir’s picture

Oh, I misread it, that listAll()/glob() business only happens when you load pass NULL and load all entities of a certain type... Wondering who's doing that...

Berdir’s picture

Added debug($this->entityType); in that if and got dozens of 'editor''s and a bunch of 'field_config''s.

Looks like we need support for a fullyLoaded flag so that we don't run through this multiple times. Also, we could combine that with a id map and load all at once from the cache or something like that.

@timplunkett also found #1971174: BreakpointGroup::loadAllBreakpoints() is expensive

Damien Tournoud’s picture

The culprit in the Editor module is editor_load(), which look like a premature optimization anyway.

For the field module, those calls should be cached already (in the field_info:instances cache), not sure why it's not working for you.

The main problem is that the storage structure is really stupid. Browsing files like that from the filesystem cannot possibly scale. We need to either start hashing this directory or move the store to a proper database (like SQLite or a db* file).

Berdir’s picture

Yeah, the problem with it is that it assumes that the entity_load_multiple('editor') call is somehow cached and initially faster, which isn't the case.. The optimization makes sense I think because it seems to be called for all formats of a textfield, so if it's called, then it really is called for all of them. If we can improve the listAll() and do a config getMultiple() then this would be true.

The problem with fields seems to be that it assumes that there is a call to field_info_instances($entity, $bundle), where it would pre-fill the internal field by id static cache. That call never seems to happen, possibly due to the EntityNG conversion of nodes so each field is loaded separately.

Also related #1880766: Preload configuration objects. Moving the config entity caching logic to config storage controller might allow #1880766: Preload configuration objects to focus on only the non-entity config files, of which there should only be a limited amount.

quicksketch’s picture

The culprit in the Editor module is editor_load(), which look like a premature optimization anyway.

I'm 100% okay with removing that optimization, though it sounds like there's a deeper bug that is causing something that should be faster to be slower instead.

Wim Leers’s picture

quicksketch and I worked on editor.module, he wrote editor_load() specifically; I talked to him and he's fine with changing that to not doing the multiple loading, especially now that anonymous and authenticated users now have one format anyway.

I don't mind writing that patch, but berdir is saying we can (should?) fix listAll() and introduce $configStorage->getMultiple() to make it faster.

So… which should we do? Fix editor+field or improve "all entities" loading?

Berdir’s picture

I think those are three different issues. This issue should attempt to make the load all use case as fast as possible, which is currently used quite often for config entities (that's how EFQ works, for example).

Once this is fixed then the editor_load() issue depends on what we expect in terms of actually using/seeing all editors for most users or not and how much memory overhead this would mean. If we try to be intelligent, we could also load them based on the editors a user would be allowed to see?

And the field case sounds like yet another separate bug. There is specific code to avoid those fields being loaded separately and that doesn't work right now.

yched’s picture

Regarding fields & FieldInfo cache :
My hope/target is to be able to ditch FieldInfo's persistent cache of Field and FieldInstance config entities, and rely solely on the config() cache on raw config data. Caching ConfigEntities on top of it sounds like a recipe for hair loss.
The "static" cache in FieldInfo would stay of course, this would be where Field and Fieldinstance objects are persisted across the request once they've been loaded (and we usually need them at several places in the callstack within a request).

The flow would be:
- FieldInfo uses config_get_storage_names_with_prefix() (which currently does listAll() / glob()) to build a "field map" (which fields / instances are present on which entity types / bundles), and persistently caches it.
- FieldInfo::getBundleInstances() figures out a list of config names to load based on the "field map", multiple-loads the config items (hitting config persistent cache), and statically persists them.

This relies on being able to multiple-load config items, and on the assumption that instantiating a ConfigEntity from raw config data is not too expensive and does not need to be persistently cached...

Berdir’s picture

Status: Active » Needs review
FileSize
9.55 KB

Here's a proof of concept that introduces ConfigFactory::loadMultiple() and a listAll() cache. Seeing some nice performance improvements (700 instead of 800 queries) but I doubt it will get through the tests just yet ;)

Notes:
- loadMultiple() is a bit tricky because ConfigFactory/Config are specifically built to support lazy loading but here we actually don't want that and want to load the upfront. Note that lazyloading is pointless for config entities anyway as we do a get() right after loading them anyway, so this is no no way a regression.
- Unlike get(), loadMultiple() only returns existing, successfully loaded configuration objects
- Note sure about listAll() being a separate or a single cache entry. A single one might make sense as we can do more intelligent cache clears and only have to do a single cache get for them.

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.61 KB
10.73 KB

Stupid mistake :(

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-13.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
10.76 KB

This should fix the installer. looks like there are multiple cached storage objects floating around, so changed to use a static property.

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-15.patch, failed testing.

Berdir’s picture

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

While this issue has derailed a bit from its title, I think we can still have a really quick win for cold caches by replacing glob() with GlobIterator, at least until someone takes on the last paragraph from #5 :/

I got a nice ~5ms improvement on a test site with 600 config files, and ~10ms with 1200 files, basically for free.

Status: Needs review » Needs work

The last submitted patch, 1971158-replace_slow_glob.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
765 bytes
10.91 KB

Ok, figured out what's causing those test fails.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -226,24 +226,20 @@ protected function buildQuery($ids, $revision_id = FALSE) {
-        $config = config($prefix . $id);
-        if (!$config->isNew()) {
-          $result[$id] = new $config_class($config->get(), $this->entityType);

The reason we're doing a isNew() check here isn't just because this could theoretically create a new config, it's also because deleted configuration is kept in CacheFactory::$cache as a new config object, as that's what Config::delete() is doing.

I just moved that check one level down as it's a problem within the config system and not config entities. Seems like deleted configuration should get removed from there.

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-20.patch, failed testing.

yched’s picture

Anxiously waiting for this to get green so that I can fix FieldInfo caching ;-)

Berdir’s picture

Ok, I figured out the problem but not yet a solution.

During installation, we're re-creating the container many times. As we're using the memory backend cache, each time we get a new CachedStorage(), we also get a new MemoryBackend, with a separate storage. Which means that in same cases, we call deleteTags() on one memory backend and load from another that still has the cache, which leads to inconsistent results.

This is also something that's happening in #1885830: Enable static caching for config entities.

Tried to make the cache use static but that broke things as well, maybe drupal_static() would work but that within a class is ugly as well.

Any ideas?

Berdir’s picture

Issue tags: +D8 cacheability
Berdir’s picture

Status: Needs work » Needs review
FileSize
471 bytes
11.37 KB

Could it be that easy?

The config factory has the persisted tag while the cached storage doesn't. So direct access to the storage will result in a different instance being used. So it seems only obvious to make this persisted as well.

The comment preview tests seem to pass, let's see...

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1022 bytes
11.9 KB

Ok, let's see how this works. Added a method that allows to clear the static list cache from within CacheFactory::reset() which is called in resetVariables().

Berdir’s picture

Title: Slow glob() calls in ConfigStorageController::buildQuery() with many yml files » Add loadMultiple() and listAll() caching to (cached) config storage
Component: configuration entity system » configuration system
Category: bug » task
Anonymous’s picture

this looks great!

+    // Clear the static list cache if supported by the storage.
+    if (method_exists($this->storage, 'resetListCache')) {
+      $this->storage->resetListCache();
+    }

can we make this something like:

    if ($this->storage instanceof StorageListCacheInterface) {
      $this->storage->resetListCache();
    }

i'm ok with that being a follow up, and i don't really care about the interface name.

Berdir’s picture

Something like this?

Named it StorageCacheInterface, in case we want to add more methods that aren't listAll() related

Completely untested.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

yay, i think this is ready to go.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We need to test this stuff

Anonymous’s picture

Status: Needs work » Needs review
FileSize
16.27 KB

added some tests for CachedStorage, and the new behaviour we've added for loading multiple.

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-33.patch, failed testing.

subson’s picture

Assigned: Unassigned » subson

In progress... Drupalcon sprint, May 24, 2013

Anonymous’s picture

Status: Needs work » Needs review
Issue tags: -Performance, -Needs tests, -D8 cacheability

Status: Needs review » Needs work
Issue tags: +Performance, +Needs tests, +D8 cacheability

The last submitted patch, config-load-multiple-1971158-33.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
15.4 KB

rerolled, let's see if the bot will run it this time.

Status: Needs review » Needs work
Issue tags: -Performance, -Needs tests, -D8 cacheability

The last submitted patch, config-load-multiple-1971158-38.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Performance, +Needs tests, +D8 cacheability
Anonymous’s picture

added some tests for the listAll static caching.

Berdir’s picture

+++ b/core/tests/Drupal/Tests/Core/Config/CachedStorageTest.phpundefined
@@ -0,0 +1,117 @@
+  /**
+   * Test CachedStorage::listAll() persistent cache.
+   */
+  public function testListAllPrimedPersistentCache() {
+    $prefix = __FUNCTION__;
+    $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
+    $storage->expects($this->once())
+      ->method('listAll')
+      ->with($prefix)
+      ->will($this->returnValue(array("$prefix.bar", "$prefix.baz")));
+
+    $cache = new MemoryBackend(__FUNCTION__);
+    $cache->set('list:' . $prefix, array("$prefix.bar", "$prefix.baz"));
+    $cachedStorage = new CachedStorage($storage, $cache);
+    $cachedStorage->listAll($prefix);

Not sure I understand this test. It talks about primed cache but then expects a call to the storage?

If we set a cache entry, then that shouldn't happen?

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-41.patch, failed testing.

Anonymous’s picture

derp derp derp durpal.

sorry, i'll reroll and fix that. i was playing with something else in that test and forgot to restore it.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
16.69 KB

new patch, minus the derp derp to address #42.

Anonymous’s picture

derp derp derp.

Berdir’s picture

+++ b/core/tests/Drupal/Tests/Core/Config/CachedStorageTest.phpundefined
@@ -0,0 +1,114 @@
+      ->will($this->returnValue(array('dididi.idiback' => array('yo wtf'))));

We should probably use a bit more serious naes :)

We have randomName() as well, so you could use that.

I think it would also not hurt to have some basic assertions on the returned values, so that this part is also checked.

Then we just need to find someone to RTBC the implementation and tests :)

Berdir’s picture

msonnabaum’s picture

Tests look great in #48.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

yay, this looks RTBC to me. berdir++

Berdir’s picture

Assigned: subson » Unassigned
Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Noticed some things that need to be cleaned up after @alexpott pointed me to one of them.

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.phpundefined
@@ -86,6 +86,29 @@ public function read($name) {
+   * Implements Drupal\Core\Config\StorageInterface::read().
+   *
+   *
+   * @param array $names

Needs tidying up, should also use {@inheritdoc}.

+++ b/core/lib/Drupal/Core/Config/FileStorage.phpundefined
@@ -90,6 +90,21 @@ public function read($name) {
+   * Implements Drupal\Core\Config\StorageInterface::readMultiple().
+   *
+   * @throws Symfony\Component\Yaml\Exception\ParseException

We still don't have a solution for inheritdoc + additional stuff, that said, this actually seems wrong (I copied it). Seems to me that a class that throws exceptions that are not defined in the interface violates that interface but that's not related to this issue.

+++ b/core/lib/Drupal/Core/Config/StorageInterface.phpundefined
@@ -39,6 +39,18 @@ public function exists($name);
+   * @param ayrray $name

That's the pirate version of an array ;)

+++ b/core/lib/Drupal/Core/Config/StorageInterface.phpundefined
@@ -39,6 +39,18 @@ public function exists($name);
+   * @return array|bool

This shouldn't specify bool I think.

Might have time tonight, but if someone else wants to do this, go for it...

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
16.85 KB

Here's the promised cleanup.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

looks good!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
19.29 KB
2.12 KB

After installing this patch and query cache_config table I noticed we have some duplicate keys due to a inconsistency with how we pass in the prefix for config entities eg. list:views.view. and list:views.view

Patch attached cleans this up... more cache hits ftw

Doing a standard profile install and viewing the frontpage as user 1

Before

select cid from cache_config where cid like 'list:%';
+---------------------------+
| cid                       |
+---------------------------+
| list:block.block.         |
| list:contact.category.    |
| list:custom_block.type.   |
| list:entity.view_mode.    |
| list:field.field.         |
| list:field.instance       |
| list:field.instance.      |
| list:menu.menu.           |
| list:taxonomy.vocabulary. |
| list:tour.tour.           |
| list:views.view           |
| list:views.view.          |
+---------------------------+
12 rows in set (0.00 sec)

After

mysql> select cid from cache_config where cid like 'list:%';
+---------------------------+
| cid                       |
+---------------------------+
| list:block.block.         |
| list:contact.category.    |
| list:custom_block.type.   |
| list:entity.view_mode.    |
| list:field.field.         |
| list:field.instance.      |
| list:menu.menu.           |
| list:taxonomy.vocabulary. |
| list:tour.tour.           |
| list:views.view.          |
+---------------------------+
10 rows in set (0.00 sec)
Berdir’s picture

Hm, that's quite a list. Wondering if it would be safe to cache this in a single cache key or if the data could grow too big.

alexpott’s picture

Okay so lets not cache when a prefix is not provided. This is the first step in refactoring the storageinterface to have separate findAll() and findByPrefix() instead of a do-it-all listAll() method. But rather than do that here lets keep the scope creep down and do this is a followup.

alexpott’s picture

Actually doing it this way round is cleaner... implementing the findByPrefix() method rather than the findAll()...

alexpott’s picture

Issue tags: +Performance

Okay we have tests but the Performance tag should still be there...

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

this looks good to go. again ;-)

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -33,6 +33,13 @@ class CachedStorage implements StorageInterface {
   /**
+   * List of listAll() prefixes with their results.
+   *
+   * @var array
+   */
+  protected static $listAllCache = array();

Why does this have to be a static as opposed to a class variable? I'd expect this to only get instantiated once per request, or at least in such a way that the cache shouldn't be shared.

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -134,12 +173,45 @@ public function decode($raw) {
+  protected function findByPrefix($prefix) {
+    if (!isset(static::$listAllCache[$prefix])) {
+      // The : character is not allowed in config file names, so this can not
+      // conflict.
+      if ($cache = $this->cache->get('list:' . $prefix)) {
+        static::$listAllCache[$prefix] = $cache->data;
+      }
+      else {
+        static::$listAllCache[$prefix] = $this->storage->listAll($prefix);
+        $this->cache->set('list:' . $prefix, static::$listAllCache[$prefix], CacheBackendInterface::CACHE_PERMANENT, array('listAll' => TRUE));
+      }
+    }

Shouldn't the variable be renamed now the cache belongs to this method?

Berdir’s picture

I made it static because there were many different config storage objects floating around during installation and module enabling. Maybe that's no longer the case thanks to the persist tag, I need to check that.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.29 KB
19.84 KB

Let's see if the testbot likes this.

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-63.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
732 bytes
19.84 KB

Looks like he does, that's nice!

This should fix the php unit test.

Also had a quick look at doing a single cache, but it looks like there aren't that many different listAll() calls on the same page, so let's keep it separate for now.

What I did see was an insane amount of entity constructing for field config entities (again) as in 12k config entities insane ;). We'll need to look into that after this is in, I thought I fixed it with the field definition cache, but something changed it again it seems...

yched’s picture

@Berdir: not sure what you mean exactly in your last paragraph, can you expand a bit ? (although maybe this doesn't belong in this issue ?)

Status: Needs review » Needs work
Issue tags: -Performance, -D8 cacheability

The last submitted patch, config-load-multiple-1971158-65.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-65.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance, +D8 cacheability

The last submitted patch, config-load-multiple-1971158-65.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.5 KB
22.6 KB

This uses the injected config factory and refactors that phpunit test to mock load() and not call down into mocked config objects. This doesn't doesn't need to bother.

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-73.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
22.77 KB

Re-rolled, something conflicted with that role test.

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-75.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
732 bytes
22.77 KB

Would work better if would use the factory ;)

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-77.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
638 bytes
23.39 KB

Ok, this turned out to be a tag checksum static cache problem. When we delete a tag multiple times on page requests, then the parent (test) process is not being updated and still uses the deletions count from the static cache. Very surprised that this does not cause failures anywhere else.

catch’s picture

Status: Needs review » Needs work

Overall this looks great. I found a couple of nitpicks.

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -80,6 +87,32 @@ public function read($name) {
+    $remaining_names = $names;
+    $list = array();

$names isn't used anywhere else in the method, so no need for the temporary variable. Also they aren't 'remaining' until after the getMultiple() call.

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -80,6 +87,32 @@ public function read($name) {
+    // The cache backend removed names that were successfully loaded from the
+    // cache.

I had to read this three times, because I initially read it as "when items are successfully loaded, the cache backend removes them from the cache". Could we just say "Items loaded from cache are removed from the list of $names to fetch" or similar?

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -87,6 +120,8 @@ public function write($name, array $data) {
       $this->cache->set($name, $data, CacheBackendInterface::CACHE_PERMANENT);
+      $this->cache->deleteTags(array('findByPrefix' => TRUE));
+      $this->findByPrefixCache = array();

Should the cache tag be configFindByPrefix? or 'configPrefixes'? Not sure it should but the tag name jumped out a little given it's a verb - almost looks like it's an options array to deleteTags() to find the tags by prefix (which would be horrifying).

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -178,14 +178,14 @@ public function getFieldMap() {
     // Get active fields.
-    foreach (config_get_storage_names_with_prefix('field.field') as $config_id) {
+    foreach (config_get_storage_names_with_prefix('field.field.') as $config_id) {

This seems easy to get wrong in contrib modules too, not sure there's anything we can do about it except fix the cases in core though.

alexpott’s picture

1. Removed and added a comment

2. This comment is removed

3. So I've created a class constant for the cache tag name... this means one we can change it after API freeze and two that we can clear the cache items using this tag reliably from other code.

4. I don't think there is anything we can do about @catch last comment in #80 at this stage.

alexpott’s picture

Status: Needs work » Needs review

Forget to change status... Shows how often I roll patches these days...

Status: Needs review » Needs work

The last submitted patch, config-load-multiple-1971158-81.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
23.8 KB
761 bytes

So #2024833: Adopt load() and loadMultiple() on entity storage controllers changed a bunch of things but missed out changing the mock in \Drupal\user\Tests\Views\Argument\RolesRidTest this currently works in head because the call to loadMultiple \Drupal\user\Plugin\views\argument\RolesRid::title_query() just depends on the filesystem and entity loading ... this patch introduces a dependency on the cache system.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

i think this is ready to go.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Looks great except:

+      $this->cache->deleteTags(array('listAll' => TRUE));

Needs to be Cache::deleteTags() - these aren't the same thing.

#918538: Decouple cache tags from cache bins is related.

Anonymous’s picture

so i guess this means we need a test that shows that $this->cache->deleteTags(array('listAll' => TRUE)); breaks stuff?

@catch: are you ok with that being a follow up?

alexpott’s picture

Status: Needs work » Fixed

@catch actually committed this... see 8ceb6e1.

So I'm marking this a fixed and yep #86/#87 ... this needs to be a followup.

catch’s picture

Title: Add loadMultiple() and listAll() caching to (cached) config storage » Follow-up: Add loadMultiple() and listAll() caching to (cached) config storage
Status: Fixed » Active
$this->cache->deleteTags(array('listAll' => TRUE));

That won't actually break anything at the moment, because the tag is only used for one bin, but it's wrong API usage (which shows we should do the split). I think we could commit the one-liner here so re-opening.

Anonymous’s picture

Status: Active » Needs review
FileSize
1.54 KB

here ya go.

Status: Needs review » Needs work

The last submitted patch, 1971158-deleteTags.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

Missing a use statement...

Status: Needs review » Needs work
Issue tags: -Performance, -D8 cacheability

The last submitted patch, 1971158-deleteTags-92.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#92: 1971158-deleteTags-92.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1971158-deleteTags-92.patch, failed testing.

catch’s picture

Status: Needs work » Needs review

#92: 1971158-deleteTags-92.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +D8 cacheability

The last submitted patch, 1971158-deleteTags-92.patch, failed testing.

Anonymous’s picture

wow. so that change really breaks things. shows you how much we collectively actually get how this cache tags stuff works.

digging around now.

Anonymous’s picture

FileSize
2.04 KB

berdir pointed out in IRC that we should persist the 'cache.config' service, attached patch does that, and more tests pass.

i have NFI why that works, but that is above my D8 pay grade.

Anonymous’s picture

Status: Needs work » Needs review

derp derp durpal.

yched’s picture

#99: 1971158-deleteTags-99.patch queued for re-testing.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

jibran’s picture

#99: 1971158-deleteTags-99.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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