Configuration being imported by the ConfigImporter sometimes has stale original data

Somewhat surprisingly this seems to speed up installing standard from config by a second!

WITH MR

time vendor/bin/drush si  --existing-config -y 
 You are about to:
 * DROP all tables in your 'drupal8alt' database.

 // Do you want to continue?: yes.

 [notice] Starting Drupal installation. This takes a while.
 [notice] Performed install task: install_select_language
 [notice] Performed install task: install_select_profile
 [notice] Performed install task: install_load_profile
 [notice] Performed install task: install_verify_requirements
 [notice] Performed install task: install_verify_database_ready
 [notice] Performed install task: install_base_system
 [notice] Performed install task: install_bootstrap_full
 [notice] Performed install task: install_config_import_batch
 [notice] Performed install task: install_config_download_translations
 [notice] Performed install task: install_config_revert_install_changes
 [notice] Performed install task: install_configure_form
 [notice] Performed install task: install_finished
 [success] Installation complete.  User name: admin  User password: 5NA5Xeo5DU

________________________________________________________
Executed in    6.51 secs    fish           external
   usr time    3.71 secs    0.09 millis    3.71 secs
   sys time    0.76 secs    5.33 millis    0.75 secs

HEAD

time vendor/bin/drush si  --existing-config -y
 You are about to:
 * DROP all tables in your 'drupal8alt' database.

 // Do you want to continue?: yes.

 [notice] Starting Drupal installation. This takes a while.
 [notice] Performed install task: install_select_language
 [notice] Performed install task: install_select_profile
 [notice] Performed install task: install_load_profile
 [notice] Performed install task: install_verify_requirements
 [notice] Performed install task: install_verify_database_ready
 [notice] Performed install task: install_base_system
 [notice] Performed install task: install_bootstrap_full
 [notice] Performed install task: install_config_import_batch
 [notice] Performed install task: install_config_download_translations
 [notice] Performed install task: install_config_revert_install_changes
 [notice] Performed install task: install_configure_form
 [notice] Performed install task: install_finished
 [success] Installation complete.  User name: admin  User password: PVtQqXFK5r

________________________________________________________
Executed in    8.49 secs    fish           external
   usr time    4.96 secs    0.09 millis    4.96 secs
   sys time    0.88 secs    4.78 millis    0.88 secs

Here's more evidence of why we're saving so much time...

Sorry - I tried to paste html it... but it doesn't work so this is an image... comparing installing standard install with an without the MR applied.

As you can see there is a significant difference in the number of function calls. I've not quite managed to explain why this is so but I think it is because the storage comparer expects to be able to do stuff to the memory backend but it can't when there's yet another memory backend hidden from it.

I mean...I see no reason not to do this. That's a simple change and it only affects the installer.

6% speed decrease in function calls and execution time for a Standard profile install! For complex install profiles, this would likely be more.

❌ I tested the wrong thing, see the issue summary!

Then I tested the right thing:

Indeed a very significant difference! 👏

Suggested some explanatory comments on the MR.

I am wondering if we could also get some improvement if in the storage comparer we only wrap the storages if they are not instanceof memory or cached storage.

But yes! this is huge for all CI which installs sites. so drupal sites that test with existing site test traits will see a difference!

Ie this bit:

   // Wrap the storages in a static cache so that multiple reads of the same
    // raw configuration object are not costly.
    $this->sourceCacheStorage = new MemoryBackend();
    $this->sourceStorage = new CachedStorage(
      $source_storage,
      $this->sourceCacheStorage
    );
    $this->targetCacheStorage = new MemoryBackend();
    $this->targetStorage = new CachedStorage(
      $target_storage,
      $this->targetCacheStorage
    );

But that could be a follow up issue too.

So the more I think about it the more I wonder about problems caused by wrapping the target storage even when not at install time. I think any secondary config writes are going to cause problems because the memory cache storage that the storage comparer has added is going to be unaware.

Also I don't like making the underlying active storage public during the install.

on second thought..
we can do that for memory storages and that would improve tests that use memory trorages in tests since the storage comparer is used in the storage copy trait.

For cached storages we would have to check what it is cached with. since on some this could still result in more queries than necessary and that is harder to check (ie we would have to add a method there that tells us the class name for the cache.. ) so the solution proposed here will get a big win without that extra complexity.

We introduced this in #2411689: Use a MemoryBackend in StorageComparer so that configuration import validators don't have to reread data from disk or the db → I think the use-case is still valid. So I think what needs to happen is that we disable the memory cache on the target storage in storage comparer once we enter the write phase of a configuration import.

We also need to care about what happens in install_config_revert_install_changes() too.

I don't really understand #12, does that mean this issue needs to go back to needs work so we can implement the disabling of the memory cache, or is that something that would be considered a followup?

@borisson_ I working on understanding the problem here. I think we need to address the underlying problem - it's actually not just about the ConfigImporter during the installer (although it is most definitely worse there).

Here's what's happening as far as I can tell. The config importer is using the storages injected into the storage comparer for reading a writing. When the config importer installs a module, the module installer will rebuild the container and write to core.extension using the newly created config.storage service. At this point the storage in the storage comparer and the container are out of sync. Then once all the modules are installed we go into the importing config module. Normally this only handles configuration entities but simple config config is dealt with by making the module installer use the storage comparer's source storage. The core.extension configuration is special however, because it does not belong to any module. For some reason the target storage's version of the core.extension configuration is completely out-of-date so when we re-save it, it looks like we're installing all the modules again - so any event listener that reacts to a module install via the config save is doing a lot of unnecessary work. An example of this is \Drupal\Core\Update\UpdateRegistry::onConfigSave().

alexpott → closed merge request !5732

I've posted a new MR that results in the same speed-ups for installing from config but will also speed up importing configuration during regular runtime.

HEAD

$ time vendor/bin/drush si  --existing-config -y
________________________________________________________
Executed in    8.79 secs    fish           external
   usr time    5.02 secs    0.08 millis    5.02 secs
   sys time    0.88 secs    2.91 millis    0.87 secs

WITH MR

$ time vendor/bin/drush si  --existing-config -y
________________________________________________________
Executed in    6.76 secs    fish           external
   usr time    3.73 secs   84.00 micros    3.73 secs
   sys time    0.75 secs  734.00 micros    0.75 secs

Now to write a test...

I think the analysis in #15 is spot on!
And the solution in the new MR makes sense to me. I left a comment about the interface.

I've tweaked the issue summary and title to be about what is actually being fixed here. We still have double memory caches in the installer but I think we should fix that after we fix this as this is more important.

Created 📌 Using the ConfigImporter during installation results in a double memory cache Active to address the original issue.

I don't think needs a Change Record, because it is such an internal piece of the puzzle, the new Issue Summary really explains the problem well, because I now understand the actual problem. The fix seems like a good solution and the test coverage seems sufficient as well.

Since the changes I made involved adding return type hints I was going to set this to Needs review. However, the new test additions are failing, bisect tells me it failing at this commit.

Therefor setting to NW.

Fixed the test. As all the changes were to test code setting back to RTBC.

FYI: We plan to remove the interface - see 📌 deprecate StorageComparerInterface Active

I just checked again and: RTBC +1!

I am wondering now if this would also fix a bug in Config Split I have had trouble getting to the bottom of. I think it might!

I asked @alexpott whether trying to move these two classes into the service container would help with this situation (because the memory cache would be destroyed on container rebuild) and he said no it would be worse, and worse than the module installer. The logic and comments here all look fine and as good as we can do, and the speed-up is great. Committed/pushed to 11.x (which will also become 10.3.x), thanks!

Here's a patch for 10.1 and probably 10.2...

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

I'm using the patch in comment #32, thanks @alexpott for posting it for 10.2

Patch in #32 works great on 10.2.7, however our team noticed that there is a .cspellcache in the patch that references the patch exporters directory structure, so this patch is the patch in #32 without the .cspellcache. Also great that this is fixed in D11! Looks like that MR did not get the same file.