Implement a key value store to optimise config entity lookups

Alternative storage for config such as mongodb shouldn't need to do this (it can query the config object natively), so would be good to ensure we don't add overhead for those implementations in terms of undoing what core does.

Denormalizing like this feels preferable to some of the other SQL-storage tweaks that have been suggested (like XML etc.).

/resurrect

This shows up in #2473983: [meta] Evaluate Entity Field API Scalability, which is partially a duplicate of #2423591: Optimize config entity import.

+++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
@@ -60,4 +67,137 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
+    foreach ($entity_type->getLookups() as $lookup_key => $type) {

Its a bit odd that contrib can't easily expand the lookup keys, not sure whether its worth to worry about, given its already quite a good improvement.

Yes since this affects any config entity you could easily run into it with combinations like views + aggregator + fields/bundles, and this is seconds of savings rather than milliseconds, so bumping to critical.

1. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -60,4 +67,137 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
   +      foreach($this->getKeys($lookup_key, $config) as $key) {
   +        if ($type == 'UNIQUE') {
   +          $config_key_store->set($key, $config->getName());
   +        }
   +        else {
   +          if ($type == 'NOT UNIQUE') {
   +            $values = $config_key_store->get($key, []);
   +            if (!in_array($config->getName(), $values, TRUE)) {
   +              $values[] = $config->getName();
   +              $config_key_store->set($key, $values);
   +            }
   +          }
   +        }
   

   NOT UNIQUE is a bit a special name for this, it sounds like it *must* not be unique (like assertNotUniqueText()).

   Do we really need this additional complexity? We're not actually enforcing that it is UNIQUE..

   Maybe we should just always allow multiple keys internally?

2. +++ b/core/modules/block/src/Entity/Block.php
   @@ -38,6 +38,9 @@
   + *   },
   + *   lookups = {
   + *     "theme" = "NOT UNIQUE"
     *   }
   

   So this would speed up #2479459: BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request as well, but adding caching there still makes sense I think.

Also it could seriously affects UI (machine name checks) with related #2091871: Add an ConfigEntityForm with an exists() method

Another way to manage uniqueness, so it needs to be swappable

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -146,4 +154,11 @@ protected function checkStorageClass($class) {
   +   */
   +  public function getDenormalizedKeys() {
   +    return $this->lookups;
   +  }
   

   lookup vs denormalized keys is a bit confusing, why go for a completely different name here?

   Does denormalized really make sense here, is that really what we're it means? The thing is that it's not actually typical denormalization that we're doing here.

   What about LookupKeys(), for both the method and property? That's what it means.. having a separate lookup mechanism for those.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   @@ -61,4 +61,12 @@
   +  /**
   +   * Gets the keys that are denormalized into a key value store for fast lookup.
   

   The description seems very specific, that we are using key value is kind of an implementation detail and not relevant here?

3. +++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
   @@ -34,12 +48,16 @@ class Query extends QueryBase implements QueryInterface {
       */
   -  function __construct(EntityTypeInterface $entity_type, $conjunction, ConfigFactoryInterface $config_factory, array $namespaces) {
   +  function __construct(EntityTypeInterface $entity_type, $conjunction, ConfigFactoryInterface $config_factory, KeyValueStoreInterface $key_value, array $namespaces) {
        parent::__construct($entity_type, $conjunction, $namespaces);
        $this->configFactory = $config_factory;
   +    $this->configKeyStore = $key_value;
      }
    
   

   As disussed yesterday in IRC, again the question of injecting they key value factory or the collection. In this case, it's probably more likely that we will use it.

4. +++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
   @@ -108,37 +126,44 @@ protected function loadRecords() {
   +            $keys = (array) $condition['value'];
   +            $keys = array_map(function ($value) use ($condition) {
   +              return $condition['field'] . ':' . $value;
   +            }, $keys);
   +            foreach ($this->configKeyStore->getMultiple($keys) as $list) {
   +              $names = array_merge($names, $list);
   +            }
   

   There's an issue to also support partial (STARTS_WITH, CONTAINS...) matches on the ID. That would be very useful to speed up thinks like field_entity_bundle_field_info(). I'm wondering if that would also be useful here, but I guess not so much.

   That issue is #2247379: Optimize config entity query conditions on ID and it's probably going to conflict badly with this.

5. +++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
   @@ -108,37 +126,44 @@ protected function loadRecords() {
   +        // We stop at the first restricting condition on name. In the (weird)
   +        // case where there are additional restricting conditions, results
   

   weird => rare? It's not *that* weird to e.g. have an entity query on entity_type + bundle for example, although for fields/displays, we mostly optimize it to load on the id.

6. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -60,4 +68,141 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
   +  protected function updateConfigKeyStore(ConfigEntityTypeInterface $entity_type, Config $config) {
   ...
   +  protected function deleteConfigKeyStore(ConfigEntityTypeInterface $entity_type, Config $config) {
   

   We will need a way to initialize they lookup collection, should we make those methods public for that purpose?

7. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -60,4 +68,141 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
   +  /**
   +   * @todo
   +   *
   +   * @param $key
   

   :)

1. +++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
   @@ -129,8 +137,9 @@ protected function loadRecords() {
   +      foreach ($conditions as $condition_key =>$condition) {
   

   missing space ;)

2. +++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
   @@ -175,4 +186,17 @@ protected function loadRecords() {
   +  protected function getConfigKeyStore() {
   +    if (!isset($this->configKeyStore)) {
   +      $this->configKeyStore = $this->keyValueFactory->get(QueryFactory::CONFIG_LOOKUP_PREFIX . $this->entityTypeId);
   +    }
   +    return $this->configKeyStore;
   +  }
   

   Note that the key value factory already maintains a "static" cache, so doing this here again shouldn't be necessary, the overhead should be negligible.

   Having a method is still useful, I should probably add that to entity manager, to avoid repeating the collection string many times.

3. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -129,12 +134,18 @@ protected function deleteConfigKeyStore(ConfigEntityTypeInterface $entity_type,
       * @param $key
   +   *   The configuration key to look for.
   

   might be fixed already, missing "string".

4. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -143,16 +154,23 @@ protected function getKeys($key, Config $config, $get_method = 'get') {
   +   *   Which position of $parts we are pocessing.
   

   typo in pocessing.

+++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
@@ -145,12 +145,34 @@ protected function deleteConfigKeyStore(ConfigEntityTypeInterface $entity_type,
+   * @throws \LogicException
+   *   Keys ending with a wildcard make no sense since you can not query against
+   *   this.

maybe restructure a bit to say that exception is thrown when such a key is passed in?

I think this is close.

A few nitpicks (sorry) was all I could find. There's still the upgrade path question.

The only thing I'm not sure about is whether this should somehow be more pluggable. But given that AFAIK the query service itself is pluggable, someone could just replace that if they want to provide an alternative implementation?

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -35,6 +35,13 @@ class ConfigEntityType extends EntityType implements ConfigEntityTypeInterface {
   +   * @var array
   
   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   @@ -61,4 +61,12 @@
   +   *
   +   * @return array
   +   *   The list of lookup keys.
   

   I think we document this as string[] now...

2. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -60,4 +75,183 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
   +  protected function updateConfigKeyStore(ConfigEntityTypeInterface $entity_type, Config $config) {
   

   I guess there's still the question of what to do with this for the upgrade path. I left @amateescu a tell, so he can have a look and decide himself what makes sense.

3. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -60,4 +75,183 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
   +   * @return array
   +   */
   +  protected function getValues(Config $config, $key, $get_method, array $parts, $start = 0) {
   

   Missing description.

4. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -60,4 +75,183 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
   +      if (!isset($parts[$start])) {
   +        // @todo consider throwing an exception?
   +        return NULL;
   +      }
   

   not exactly sure what that means, but we should probably either get rid of the todo by deciding what to do or have an issue reference as follow-up?

5. +++ b/core/lib/Drupal/Core/Field/BaseFieldOverrideStorage.php
   @@ -8,6 +8,7 @@
    
    use Drupal\Core\Entity\EntityTypeInterface;
   +use Drupal\Core\KeyValueStore\KeyValueFactoryInterface;
   

   no other change in this file, looks like a left-over.

6. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/Query/QueryFactoryTest.php
   @@ -0,0 +1,132 @@
   +   * @param $name
   +   *   The config name.
   +   *
   +   * @return \Drupal\Core\Config\Config
   

   string missing for @param and description for @return.

1. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -144,19 +144,21 @@ protected function deleteConfigKeyStore(ConfigEntityTypeInterface $entity_type,
   -   * @throws \LogicException
   +   * @throws InvalidLookupKeyException
   

   Should use the full namespace I think.

2. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/Query/QueryFactoryTest.php
   @@ -103,24 +102,31 @@ public function providerTestGetKeys() {
       * @param $name
       *   The config name.
   

   type still missing (@param string $name) :)

Test run was terminated somehow, lets try again...

I can't find anything to complain about anymore :) This is well tested and documented and provides huge performance improvements with a lot of configuration.

With the follow-up for the race conditions, I think this is RTBC.

Discussed the potential for race conditions. Since this is (mostly) pre-existing and we have a documented follow-up whereas we didn't before, I think that's OK. The probability of sites falling over due to long config lookups is higher than the probability of high-concurrency config writes causing a race condition.

Couple of whitespace issues I fixed on commit, otherwise I also couldn't find anything to complain about, and I'd like to see this in sooner than later just in case something does get flushed out before the next beta. So, committed/pushed to 8.0.x, thanks!

1. +++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.php
   @@ -60,4 +75,189 @@ public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
   +      foreach($this->getKeys($config, $lookup_key, 'get', $entity_type) as $key) {
   
   +++ b/core/modules/system/src/Tests/Entity/ConfigEntityQueryTest.php
   @@ -470,6 +471,64 @@ public function testCaseSensitivity() {
   +
   

   Nit, missing space. Fixed on commit.

2. +++ b/core/modules/system/src/Tests/Entity/ConfigEntityQueryTest.php
   @@ -470,6 +471,64 @@ public function testCaseSensitivity() {
   +
   +
   

   Double line, fixed on commit.

Committed/pushed to 8.0.x, thanks!