Problem/Motivation

We read all the configuration data in StorageComparer and statically cache in that object. This means that any events and the importer have to reread the configuration.

Proposed resolution

We can wrap the source and target storages with CachedStorage so that we don't have to re-read the data.

Remaining tasks

Review
Commit

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it not fixing a bug - just a small internal refactor of StorageComparer
Issue priority Major because during configuration import we read a lot of data from disk and the db anything we can do to minimise the amount of times we have to read from disk or the db is a good thing. Additionally as we add configuration import validators they will need to interact with incoming data - we shouldn't be reading everything twice.
Prioritized changes The main goal of this issue is performance.
Disruption none
CommentFileSizeAuthor
#16 2411689.16.patch6.59 KBalexpott
#14 2411689.14.patch6.59 KBalexpott
#5 2411689.5.patch7.04 KBalexpott
#5 1-5-interdiff.txt2.06 KBalexpott
#1 2411689.1.patch6.91 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
2411689.1.patch6.91 KB

Here's a patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Related issues: +#1842956: [Meta] Implement event listeners to validate imports
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -80,22 +81,18 @@ class StorageComparer implements StorageComparerInterface {
+   * A memory cache backend to statically cache source configuration data.
...
+   * A memory cache backend to statically cache target configuration data.

@@ -365,6 +371,9 @@ public function moveRenameToUpdate($rename, $collection = StorageInterface::DEFA
+    // Reset the static configuration data caches.

@@ -399,14 +408,15 @@ protected function getAndSortConfigData($collection) {
+    // Prime the static caches.

    Is "statically" still correct technically speaking? 

  2. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -108,8 +105,17 @@ class StorageComparer implements StorageComparerInterface {
+    // Wrapper the sourceStorage in a storage that statically caches reads.

    s/Wrapper/Wrap/

    :) 

  3. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -258,8 +261,10 @@ protected function addChangelistCreate($collection) {
+        if (isset($source_data['uuid']) && $source_data['uuid'] != $target_data['uuid']) {

    Unchanged logic compared to HEAD, which is good. But: why not use strict comparison, while refactoring this already? 

  4. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -399,14 +408,15 @@ protected function getAndSortConfigData($collection) {
-    $this->targetData[$collection] = $target_storage->readMultiple($target_names);
-    $this->sourceData[$collection] = $source_storage->readMultiple($source_names);
...
+    $target_data = $target_storage->readMultiple($target_names);
+    $source_data = $source_storage->readMultiple($source_names);

    How does this prime the memory-backend-backed cached storages? AFAICT it just assigns them to local variables.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
1-5-interdiff.txt2.06 KB
2411689.5.patch7.04 KB
  1. Yep the StorageComparer is wrapping any storages passed to it with a CachedStorage storage backend which uses a MemoryBackend as the cache layer. The memory backend is a static cache.
  2. Fixed
  3. Sure - fixed
  4. Improved comment
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Done some profiling on top of the patch in #2416109: Validate configuration dependencies before importing configuration. The profile is for running ConfigImporter::initialize() when importing the standard profile over the minimal profile - so there is a tonne of configuration to validate. The results are available here: https://blackfire.io/profiles/compare/4b09cf95-469d-40d5-9767-bb3e87b697...

tldr; This patch represents a significant performance boost. The amount of time spent is almost 50% less and we are talking over 1 second saved.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#6: awesome!

#5.1: wouldn't it be more accurate and less confusing to not say "static caching" but "cache in memory"?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

My remark about #6.1 is probably just because I'm a native speaker — @alexpott says that to him "static caching" and "caching in memory" are the same thing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Use a MemoryBackend in StorageComparer so that configuration import validators don't have to reread data from disk of the db » Use a MemoryBackend in StorageComparer so that configuration import validators don't have to reread data from disk or the db

  • catch committed e32e6ff on 8.0.x 
    Issue #2411689 by alexpott: Use a MemoryBackend in StorageComparer so...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented
Status: Fixed » Reviewed & tested by the community

Seems like only half of the patch was committed?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Reviewed & tested by the community
FileSize
2411689.14.patch6.59 KB

The missing part... weird.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Reviewed & tested by the community
FileSize
2411689.16.patch6.59 KB

Fixing the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

As per the original rtbc...

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Confirmed that this is indeed the missing bit of the patch by comparing with a rebase from before the previous commit. Committed and pushed to 8.0.x., thanks!

  • xjm committed 3a65c26 on 8.0.x 
    Issue #2411689 followup by alexpott: Use a MemoryBackend in...

Status: Fixed » Closed (fixed)

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