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
Files: 
CommentFileSizeAuthor
#16 2411689.16.patch6.59 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,430 pass(es). View
#14 2411689.14.patch6.59 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2411689.14.patch. Unable to apply patch. See the log in the details link for more information. View
#5 2411689.5.patch7.04 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2411689.5.patch. Unable to apply patch. See the log in the details link for more information. View
#5 1-5-interdiff.txt2.06 KBalexpott
#1 2411689.1.patch6.91 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,243 pass(es). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
6.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,243 pass(es). View

Here's a patch.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Wim Leers’s picture

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

Status: Needs work » Needs review
FileSize
2.06 KB
7.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2411689.5.patch. Unable to apply patch. See the log in the details link for more information. View
  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

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

#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

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

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

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

tstoeckler’s picture

Status: Fixed » Reviewed & tested by the community

Seems like only half of the patch was committed?

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2411689.14.patch. Unable to apply patch. See the log in the details link for more information. View

The missing part... weird.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,430 pass(es). View

Fixing the patch.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

As per the original rtbc...

xjm’s picture

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.