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().

Files: 
CommentFileSizeAuthor
#99 1971158-deleteTags-99.patch2.04 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 58,917 pass(es).
[ View ]
#92 1971158-deleteTags-92.patch1.67 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 56,576 pass(es), 185 fail(s), and 293 exception(s).
[ View ]
#90 1971158-deleteTags.patch1.54 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#84 81-84-interdiff.txt761 bytesalexpott
#84 config-load-multiple-1971158-84.patch23.8 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,691 pass(es).
[ View ]
#81 79-81-interdiff.txt4.3 KBalexpott
#81 config-load-multiple-1971158-81.patch23.79 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 56,871 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#79 config-load-multiple-1971158-79.patch23.39 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,395 pass(es).
[ View ]
#79 config-load-multiple-1971158-79-interdiff.txt638 bytesBerdir
#77 config-load-multiple-1971158-77.patch22.77 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,505 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#77 config-load-multiple-1971158-77-interdiff.txt732 bytesBerdir
#75 config-load-multiple-1971158-75.patch22.77 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#73 config-load-multiple-1971158-73.patch22.6 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-load-multiple-1971158-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#73 config-load-multiple-1971158-73-interdiff.txt3.5 KBBerdir
#65 config-load-multiple-1971158-65.patch19.84 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,076 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#65 config-load-multiple-1971158-65-interdiff.txt732 bytesBerdir
#63 config-load-multiple-1971158-63.patch19.84 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,128 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 55,056 pass(es).
[ View ]
#57 config-load-multiple-1971158-57.patch19.62 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,417 pass(es).
[ View ]
#57 55-57-interdiff.txt749 bytesalexpott
#55 53-55-interdiff.txt2.12 KBalexpott
#55 config-load-multiple-1971158-55.patch19.29 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,537 pass(es).
[ View ]
#53 config-load-multiple-1971158-53.patch16.85 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,407 pass(es).
[ View ]
#53 config-load-multiple-1971158-53-interdiff.txt2.43 KBBerdir
#48 config-load-multiple-1971158-48.patch17.08 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 56,072 pass(es).
[ View ]
#48 config-load-multiple-1971158-48-interdiff.txt3.09 KBBerdir
#46 config-load-multiple-1971158-46.patch16.69 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 55,726 pass(es).
[ View ]
#45 config-load-multiple-1971158-45.patch16.69 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 55,733 pass(es).
[ View ]
#41 config-load-multiple-1971158-41.patch16.57 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 55,713 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#38 config-load-multiple-1971158-38.patch15.4 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 55,789 pass(es).
[ View ]
#33 config-load-multiple-1971158-33.patch16.27 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-load-multiple-1971158-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 config-load-multiple-1971158-30.patch12.79 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 56,795 pass(es).
[ View ]
#30 config-load-multiple-1971158-30-interdiff.txt1.68 KBBerdir
#27 config-load-multiple-1971158-27.patch11.9 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 56,736 pass(es).
[ View ]
#27 config-load-multiple-1971158-27-interdiff.txt1022 bytesBerdir
#25 config-load-multiple-1971158-25.patch11.37 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,360 pass(es), 68 fail(s), and 74 exception(s).
[ View ]
#25 config-load-multiple-1971158-25-interdiff.txt471 bytesBerdir
#20 config-load-multiple-1971158-20.patch10.91 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,820 pass(es), 84 fail(s), and 92 exception(s).
[ View ]
#20 config-load-multiple-1971158-20-interdiff.txt765 bytesBerdir
#18 1971158-replace_slow_glob.patch1.79 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#15 config-load-multiple-1971158-15.patch10.76 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,602 pass(es), 123 fail(s), and 105 exception(s).
[ View ]
#15 config-load-multiple-1971158-15-interdiff.txt2.63 KBBerdir
#13 config-load-multiple-1971158-13.patch10.73 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#13 config-load-multiple-1971158-13-interdiff.txt2.61 KBBerdir
#11 config-load-multiple-1971158-11.patch9.55 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

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.

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.

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

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

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

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.

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.

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?

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.

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

Status:Active» Needs review
StatusFileSize
new9.55 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.61 KB
new10.73 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Stupid mistake :(

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.63 KB
new10.76 KB
FAILED: [[SimpleTest]]: [MySQL] 52,602 pass(es), 123 fail(s), and 105 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.79 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new765 bytes
new10.91 KB
FAILED: [[SimpleTest]]: [MySQL] 53,820 pass(es), 84 fail(s), and 92 exception(s).
[ View ]

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.

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

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?

Issue tags:+D8 cacheability

Status:Needs work» Needs review
StatusFileSize
new471 bytes
new11.37 KB
FAILED: [[SimpleTest]]: [MySQL] 55,360 pass(es), 68 fail(s), and 74 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1022 bytes
new11.9 KB
PASSED: [[SimpleTest]]: [MySQL] 56,736 pass(es).
[ View ]

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().

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

this looks great!

<?php
+    // 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:

<?php
   
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.

StatusFileSize
new1.68 KB
new12.79 KB
PASSED: [[SimpleTest]]: [MySQL] 56,795 pass(es).
[ View ]

Something like this?

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

Completely untested.

Status:Needs review» Reviewed & tested by the community

yay, i think this is ready to go.

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

We need to test this stuff

Status:Needs work» Needs review
StatusFileSize
new16.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-load-multiple-1971158-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Assigned:Unassigned» subson

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

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.

Status:Needs work» Needs review
StatusFileSize
new15.4 KB
PASSED: [[SimpleTest]]: [MySQL] 55,789 pass(es).
[ View ]

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.

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

StatusFileSize
new16.57 KB
FAILED: [[SimpleTest]]: [MySQL] 55,713 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

added some tests for the listAll static caching.

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

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.

Status:Needs work» Needs review
StatusFileSize
new16.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,733 pass(es).
[ View ]

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

StatusFileSize
new16.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,726 pass(es).
[ View ]

derp derp derp.

+++ 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 :)

StatusFileSize
new3.09 KB
new17.08 KB
PASSED: [[SimpleTest]]: [MySQL] 56,072 pass(es).
[ View ]

Something like this.

Tests look great in #48.

Status:Needs review» Reviewed & tested by the community

yay, this looks RTBC to me. berdir++

Assigned:subson» Unassigned

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

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
new16.85 KB
PASSED: [[SimpleTest]]: [MySQL] 55,407 pass(es).
[ View ]

Here's the promised cleanup.

Status:Needs review» Reviewed & tested by the community

looks good!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new19.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,537 pass(es).
[ View ]
new2.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)

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.

StatusFileSize
new749 bytes
new19.62 KB
PASSED: [[SimpleTest]]: [MySQL] 55,417 pass(es).
[ View ]

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.

Issue tags:-Performance, -Needs tests
StatusFileSize
new19.8 KB
PASSED: [[SimpleTest]]: [MySQL] 55,056 pass(es).
[ View ]
new1.61 KB

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

Issue tags:+Performance

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

Status:Needs review» Reviewed & tested by the community

this looks good to go. again ;-)

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?

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.

Status:Needs work» Needs review
StatusFileSize
new3.29 KB
new19.84 KB
FAILED: [[SimpleTest]]: [MySQL] 57,128 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new732 bytes
new19.84 KB
FAILED: [[SimpleTest]]: [MySQL] 58,076 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.5 KB
new22.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-load-multiple-1971158-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new22.77 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new732 bytes
new22.77 KB
FAILED: [[SimpleTest]]: [MySQL] 58,505 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new638 bytes
new23.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,395 pass(es).
[ View ]

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.

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.

StatusFileSize
new23.79 KB
FAILED: [[SimpleTest]]: [MySQL] 56,871 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new4.3 KB

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.

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.

Status:Needs work» Needs review
StatusFileSize
new23.8 KB
PASSED: [[SimpleTest]]: [MySQL] 56,691 pass(es).
[ View ]
new761 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.

Status:Needs review» Reviewed & tested by the community

i think this is ready to go.

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.

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?

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.

Title:Add loadMultiple() and listAll() caching to (cached) config storageFollow-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.

Status:Active» Needs review
StatusFileSize
new1.54 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

here ya go.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] 56,576 pass(es), 185 fail(s), and 293 exception(s).
[ View ]

Missing a use statement...

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

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

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.

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.

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

digging around now.

StatusFileSize
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 58,917 pass(es).
[ View ]

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.

Status:Needs work» Needs review

derp derp durpal.

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

Status:Needs review» Reviewed & tested by the community

Looks good.

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

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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