summary: we need to fix config caching.

here is some lulz-inducing stuff from Berdir:

"Started looking into the preload issue and found some things that need to be fixed in the config override code. No specific order and not everything is equally problematic but there are some pretty serious performance regressions, so setting to critical for now.

a) Trivial and easy to fix performance problem. DrupalKernel::initializeContainer() calls $factory->get('system.module')->load()->get('enabled'), the explicit load() does an unconditional call to $cached_storage->read() but the config has already been loaded by the config factory at that point. As discussed with @beejebus, maybe load() should be protected but that seems like a separate issue to figure out.

b) Fixing that, I noticed that ConfigFactory/CachedStorage isn't good at dealing with non-existing config files. color.module tries to load a non-existing color.bartik (might be a bug on it's own), then fun things happen:

1. ConfigFactory::get('color.bartik')
2. ""::loadMultiple()
3. CachedStorage::loadMultiple(array('color.bartik', 'language.config.en.color.bartik'))
4. Cache miss on both of them.
5. FileStorage::readMultiple() => read() for both of them, still not existing.
6. Back to CacheFactory::get(), loadMultiple() did not return anything, so we go into the else
7. CachedStorage::read('language.config.en.color.bartik') ($this->storage->read($this->getLanguageConfigName($this->language->id, $name)); => that config file name helper method can return FALSE!?)
8. another cache miss, another attempt to load the file from FileStorage, obviously another fail :)
9. So no config and no overrides (suprise! ;)), we return an empty config object, but this is not the end, all good things are 10 ;)
10. Now color.module calls get('load'), that somehow thinks that it hasn't been loaded yet and just to be sure tries to load color.bartik again. So another failed cache get, another call to FileStorage. I don't see why we still have the lazy-load implementation inside Config if we pre-load everything again?

Conclusion: Trying to load a single non-existing config file results in 4 cache misses and 4 FileStorage::read() calls.

Suggestion: Apart from trying to clean that mess (sorry :)) up, we need to keep track of non-existing files in CachedStorage, maybe in a single cache?

c) Note that creating a new config file (e.g. saving a new config entity) goes through the same chain right now, this is something that whatever missing-config files cache we will have in CachedStorage needs to somehow being able to handle this, so a save() needs to make sure to update that information.

d) It also applies, even though not quite as severe ("just" a single cache miss + FileStorage::read()) to every single non-existing language override config file read(). See above about needing an efficient non-existing config object cache.

e) And lastly, I'm wondering why we're even trying to load language overrides on my system at all, because this is not a multilingual installation. We can skip the language overrides completely if languageManager->isMultilingual() is FALSE and we might be able to optimize more there but that is going to be a bit more complicated (config entity objects all have a langcode, we don't need to look for overrides if that matches the current language or if it doesn't have such a key and we are using the default language?)"

Files: 
CommentFileSizeAuthor
#42 2177739.42.patch14.36 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,368 pass(es). View
#42 38-42-interdiff.txt1.32 KBalexpott
#38 2177739.38-interdiff.txt785 bytesBerdir
#38 2177739.38.patch13.92 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 63,249 pass(es). View
#36 2177739.36-interdiff.txt692 bytesBerdir
#36 2177739.36.patch13.36 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 62,396 pass(es), 106 fail(s), and 47 exception(s). View
#34 2177739.34.patch13.36 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#23 2177739.23.patch13.39 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2177739.23.patch. Unable to apply patch. See the log in the details link for more information. View
#23 18-23-interdiff.txt17.69 KBalexpott
#20 remove-lazy-load-check-multilingual-2177739-18-interdiff.txt867 bytesBerdir
#20 remove-lazy-load-check-multilingual-2177739-18.patch23.22 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 59,761 pass(es), 0 fail(s), and 19 exception(s). View
#16 remove-lazy-load-check-multilingual-2177739-16-interdiff.txt3 KBBerdir
#16 remove-lazy-load-check-multilingual-2177739-16.patch23.12 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,890 pass(es). View
#13 remove-lazy-load-check-multilingual-2177739-13-interdiff.txt2.17 KBBerdir
#13 remove-lazy-load-check-multilingual-2177739-13.patch20.91 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 59,842 pass(es), 1 fail(s), and 0 exception(s). View
#11 remove-lazy-load-check-multilingual-2177739-11-interdiff.txt367 bytesBerdir
#11 remove-lazy-load-check-multilingual-2177739-11.patch19.33 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,365 pass(es), 417 fail(s), and 110 exception(s). View
#9 remove-lazy-load-check-multilingual-2177739-3.patch19.18 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#4 avoid-extra-language-config-lookup.txt5 KBGábor Hojtsy
#3 remove-lazy-load-check-multilingual-2177739-3.patch19.18 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

beejeebus’s picture

Issue tags: +Performance
Gábor Hojtsy’s picture

@berdir asked me to respond on language overrides. Here you go :)

And lastly, I'm wondering why we're even trying to load language overrides on my system at all, because this is not a multilingual installation. We can skip the language overrides completely if languageManager->isMultilingual() is FALSE

Yeah, this is true. Before the built-in overrides system, language overrides were in fact injected from the locale module which would not have been enabled on your site. So yeah, we can skip applying overrides if languageManager->isMultilingual() is FALSE. Indeed.

and we might be able to optimize more there but that is going to be a bit more complicated (config entity objects all have a langcode, we don't need to look for overrides if that matches the current language or if it doesn't have such a key and we are using the default language?)

Yeah theoretically it is possible to have a language override file even in the same language that the entity was originally saved in, but I see no use case for that :D Eg. locale and config translation will never create such files. If the config file has no top level langcode in it, locale and config translation will entirely ignore it for language support, since it has no idea what language the file may be in, so it cannot really provide translations (ie. it cannot tell the source language). So if the file had no langcode, then it is safe to assume that there are no language overrides possible. Yes.

Berdir’s picture

FileSize
19.18 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Getting started here:

- Ripping out the whole lazy load code in Config, when we always pre-load things in the ConfigFactory then there is no point in checking that
- Changed the language handling on the config factory, inject the manager so that we can check for multilingual, make setting a manual language for language overrides easier with enforceLanguage() & resetEnforcedLanguage().

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
5 KB

I also got started on an (untested) patch, here. It may have ideas to bring over to yours :) Eg. I think you are not checking if the source language equals the target language if I read your patch right. Although you are making way many more changes :D

Berdir’s picture

I thought about that, but the thing is that we load the actual file and an eventual override at the same time, so we don't know yet if we will need it or not. The part that you are changing only affects new config files.

I don't think we can inject the language manager directly, due to circular dependencies...

Berdir’s picture

I think it's not that bad that we can't compare the language, as we can cache the overrides. Assuming that we fix CachedStorage for non-existing config objects.

Also, speaking of caching, I currently can not to see how caching and module overrides could possibly work together, as we can not consider whatever those modules use to decide what overrides to apply?

Gábor Hojtsy’s picture

@Berdir: I believe the idea for module overrides was that it will be the module's responsibility to cache and return from the cache. Exactly because you cannot really tell the override criteria. The prior context system provided *some* of that criteria but could not really be used as authoritative source either, eg. if you have domain based overrides or OG based overrides, etc.

Berdir’s picture

Right, but the ConfigFactory has a static *and* persistent cache based on it's own cache key, a module has no chance to do anything about that?

Edit: I got confused, it is only a static cache, so not that bad. sorry for the noise.

Berdir’s picture

FileSize
19.18 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re-uploading my patch, testbot doesn't seem to be testing it.

Status: Needs review » Needs work

The last submitted patch, 9: remove-lazy-load-check-multilingual-2177739-3.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
19.33 KB
FAILED: [[SimpleTest]]: [MySQL] 58,365 pass(es), 417 fail(s), and 110 exception(s). View
367 bytes

This should work better. installer-error-handling--.

The last submitted patch, 11: remove-lazy-load-check-multilingual-2177739-11.patch, failed testing.

Berdir’s picture

FileSize
20.91 KB
FAILED: [[SimpleTest]]: [MySQL] 59,842 pass(es), 1 fail(s), and 0 exception(s). View
2.17 KB

Fixing the remaining direct calls to load(), not sure about the changes in ConfigFactory and Importer, but tests seem to pass...

As discussed with alexpott, the multilingual check doesn't work, at least not that easily, as we also want to translate default configuration through language overrides*. Not touching that part yet and there are test failures related to that.

* While doing some tests with that, I noticed that helper functions like the on that creates node body fields needs to force the langcode to en, as they have a hardcoded, non-translatable english label.

beejeebus’s picture

nice. i think we may need to add:

           $old_config->initWithData($this->storageComparer->getTargetStorage()->read($name));

to the importer changes, so that the $old_config object has it's data in memory.

Status: Needs review » Needs work

The last submitted patch, 13: remove-lazy-load-check-multilingual-2177739-13.patch, failed testing.

Berdir’s picture

Ok, removed the isMultilingual() check for now, added caching for non-existing configuration objects.

Confirmed that there are no calls to FileStorage::read() on a warm cache anymore. I always found the non-arrray checks in CachedStorage extremely weird, it makes no sense that your own cache could be something that we didn't put there, so glad that this can be cleaned up :)

Berdir’s picture

Status: Needs work » Needs review

The last submitted patch, 3: remove-lazy-load-check-multilingual-2177739-3.patch, failed testing.

Berdir’s picture

Added the initWithData() call to ConfigImporter.

Note that the language related changes are currently unnecessary as they don't change anything. So unless we can find a way to actually improve something there, I can revert those again.

Berdir’s picture

FileSize
23.22 KB
FAILED: [[SimpleTest]]: [MySQL] 59,761 pass(es), 0 fail(s), and 19 exception(s). View
867 bytes

Added the initWithData() call to ConfigImporter.

Note that the language related changes are currently unnecessary as they don't change anything. So unless we can find a way to actually improve something there, I can revert those again.

Status: Needs review » Needs work

The last submitted patch, 20: remove-lazy-load-check-multilingual-2177739-18.patch, failed testing.

Gábor Hojtsy’s picture

@Berdir: yeah, I found the language changes a bit confusing, so if they don't help, maybe best to remove :)

alexpott’s picture

Status: Needs work » Needs review
FileSize
17.69 KB
13.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2177739.23.patch. Unable to apply patch. See the log in the details link for more information. View

I agree the language changes should be removed - #2108599: Convert language_default to CMI ensures that language is alway set in the configuration factory and I think this is correct. Even if the site is a monolingual site we need to be able to use the language override system otherwise we have no way of translating default configuration.

Added some PHPUnit tests to CachedStorageTest that tests both CachedStorage::read() and CachedStorage::readMultiple().

Berdir’s picture

We never need to do language overrides if language.module is not enabled IMHO. And right now, if that is not there, then nobody does set the language(manager), so no overriding happens. Not sure if actually works like that right now, but I think would make sense.

alexpott’s picture

@Berdir yep you are correct - but I think we should handle the interaction between the config factory and language over in #2108599: Convert language_default to CMI

Status: Needs review » Needs work

The last submitted patch, 23: 2177739.23.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

23: 2177739.23.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2177739.23.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

The test failures appear to be unrelated.

Gábor Hojtsy’s picture

Looks good to me. I'm not sure I'm the best one to RTBC though. Would be good to have a once-over from @beejeebus.

Gábor Hojtsy’s picture

Title: fix inefficient config factory caching » Fix inefficient config factory caching
Status: Needs review » Reviewed & tested by the community
Issue tags: +Configuration system

All right, since @beejeebus does not seem to be available, I did a once-over again on the patch and the fixes look good. Especially the new test coverage to ensure that the loads are efficient. Looks great.

Berdir’s picture

23: 2177739.23.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2177739.23.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re-roll. Conflicted with renamed constant and a new line in the dropped load() method.

Status: Needs review » Needs work

The last submitted patch, 34: 2177739.34.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.36 KB
FAILED: [[SimpleTest]]: [MySQL] 62,396 pass(es), 106 fail(s), and 47 exception(s). View
692 bytes

Today is apparently stupid-typo-day for me.

Status: Needs review » Needs work

The last submitted patch, 36: 2177739.36.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
13.92 KB
PASSED: [[SimpleTest]]: [MySQL] 63,249 pass(es). View
785 bytes

Removing a new isLoaded() check.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

spent some time looking at this patch and discussing with berdir and alexpott.

if it's green, i think it's RTBC.

beejeebus’s picture

Assigned: Unassigned » catch
catch’s picture

  1. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
    @@ -99,9 +91,10 @@ public function readMultiple(array $names) {
    +        $this->cache->set($name, isset($list[$name]) ? $list[$name] : FALSE, Cache::PERMANENT);
    

    Minor but no need to explicitly set Cache::PERMANENT since it's the default.

  2. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
    @@ -68,22 +68,14 @@ public function exists($name) {
    +    $this->cache->set($name, $data, Cache::PERMANENT);
    

    And here.

alexpott’s picture

FileSize
1.32 KB
14.36 KB
PASSED: [[SimpleTest]]: [MySQL] 63,368 pass(es). View

Removed Cache::PERMANENT from \Drupal\Core\Config\CachedStorage where possible.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

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