Problem/Motivation

Initially, this was reported because validateKeys() was slow. It's a lot of low level calls, so only small optimizations would be possible.

However, we then quickly identified that we are validating the keys for config data loaded from the storage/cache. Which is pointless, as we already know that it is validated, we only need to care about manually set/imported new configuration.

On my bigger site, with large config entities like page_manager pages, this validation took up to 30ms/10+% of a render-cached response.

Proposed resolution

Add a new optional argument to setData() to disable keys validation. Set that in initWithData(), where we can assume that it is only already validated data.

The result:

Remaining tasks

User interface changes

API changes

New optional argument, no other change.

Original report by @dawehner

If you look onto a warm page as an anonymous user, standard install, you see
that ConfigBase::validateKeys()needs around 2ms which is basically a little bit more than 1% of the request.

Maybe we can improve that.

Use the array cast trick to save maybe 500ms ... sure its not that much but its small bits which actually might matter much more if you have a big configured site

An alternative could be to not validate the keys on runtime, but just on save/import.

CommentFileSizeAuthor
#10 config-set-data-validate-2380607-10-interdiff.txt815 bytesberdir
#10 config-set-data-validate-2380607-10.patch2.27 KBberdir
#9 config-set-data-validate-2380607-8-interdiff.txt680 bytesberdir
#9 config-set-data-validate-2380607-8.patch2.26 KBberdir
#7 config-set-data-validate-2380607-7-interdiff.txt450 bytesberdir
#7 config-set-data-validate-2380607-7.patch2.14 KBberdir
#5 config-set-data-validate-2380607-4.patch2.22 KBberdir
#4 setData.png61.41 KBberdir
#1 2380607-1.patch603 bytesdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
StatusFileSize
new 2380607-1.patch603 bytes
damien tournoud’s picture

damien tournoud commented

The bytecode analysis is not very exciting: http://3v4l.org/U8hdK

Clearly the new version will be much slower on HHVM (which has a dedicated opcode for is_array), and I doubt it is actually faster outside of a development environment on legacy PHP (because xdebug, xhprof, uprofiler and friends tend to slow down function calls).

dawehner’s picture

dawehner
Primary language German
commented

I'm pretty sure I had xdebug disabled, but yeah uprofiler might have been problematic.

This is all quite tricky, but I wonder in general whether we really have to validate the keys on runtime.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Microoptimize ConfigBase::validateKeys » Do not call ConfigBase::validateKeys() for data loaded from storage
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new setData.png61.41 KB

It's 5% for me on an actual site, with 75% calls.

Yes, I don't think the problem is that it is slow, the problem is that we should only call it when you manually call $config->set() and not validate data that's coming from the storage?

So changing the title.

Also, it looks like we are calling setData() twice?

Config extends StorableConfigBase:

  /**
   * {@inheritdoc}
   */
  public function initWithData(array $data) {
    parent::initWithData($data);
    $this->settingsOverrides = array();
    $this->moduleOverrides = array();
    $this->setData($data);
    return $this;
  }

StorableConfigBase:

  public function initWithData(array $data) {
    $this->isNew = FALSE;
    $this->setData($data);
    $this->originalData = $this->data;
    return $this;
  }

Is that really necessary?

The attached patch adds a $validate_keys option to setData() and sets it to FALSE in initWithData().

I don't know how much of this is overhead, but I think the screenshot below explains why this is major :)

And it doesn't matter much anymore if we call it twice or not.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new config-set-data-validate-2380607-4.patch2.22 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -80,7 +80,7 @@ class Config extends StorableConfigBase {
+    $this->setData($data, FALSE);

You're right - I think this setData() is superfluous - afaics.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new config-set-data-validate-2380607-7.patch2.14 KB
new config-set-data-validate-2380607-7-interdiff.txt450 bytes

Only one way to find out. Won't make much of a difference now, though.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -157,6 +157,8 @@ public function get($key = '') {
    *   The configuration object.
@@ -164,8 +166,10 @@ public function get($key = '') {

@@ -164,8 +166,10 @@ public function get($key = '') {
    * @throws \Drupal\Core\Config\ConfigValueException
    *   If any key in $data in any depth contains a dot.
    */
-  public function setData(array $data) {
-    $this->validateKeys($data);
+  public function setData(array $data, $validate_keys = TRUE) {
+    if ($validate_keys) {
+      $this->validateKeys($data);
+    }
     $this->data = $data;

We still should document this parameter

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new config-set-data-validate-2380607-8.patch2.26 KB
new config-set-data-validate-2380607-8-interdiff.txt680 bytes

Improved the comment a bit.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new config-set-data-validate-2380607-10.patch2.27 KB
new config-set-data-validate-2380607-10-interdiff.txt815 bytes

Added the missing (optional).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
alexpott’s picture

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

Nice work.

alexpott’s picture

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

I've reviewed all the calls to initWithData - they all should be receiving configuration that has already been validated.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Wow, amazing find!

  • catch committed a943f58 on 8.0.x 
    Issue #2380607 by Berdir, dawehner: Do not call ConfigBase::validateKeys...
catch’s picture

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

Nice find. Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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