Rebuilding field map with many bundles/fields is very slow

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +698,50 @@ public function getFieldMapByFieldType($field_type) {
   +    if (!isset($field_bundle_map[$field_definition->getName()])) {
   +      $field_bundle_map[$field_definition->getName()] = [
   +        'type' => $field_definition->getType(),
   +        'bundles' => [],
   +      ];
   

   This is a bit confusing, don't we want to always override existing entries?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +698,50 @@ public function getFieldMapByFieldType($field_type) {
   +    $this->cacheBackend->delete('entity_field_map');
   +    $this->fieldMap = [];
   ...
   +    $this->cacheBackend->delete('entity_field_map');
   +    $this->fieldMap = [];
   

   Should we have a dedicated reset method for the field map?

3. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionListenerInterface.php
   @@ -0,0 +1,47 @@
   +
   +
   

   nitpick: 2 empty lines

At one stage we were striving to reduce the size/complexity of EntityManagerInterface, but this seems to going against that trend? Is there somewhere else this could live?

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +698,50 @@ public function getFieldMapByFieldType($field_type) {
   +        'bundles' => [],
   ...
   +    $field_bundle_map[$field_definition->getName()]['bundles'][] = $field_definition->getTargetBundle();
   ...
   +    $key = array_search($field_definition->getTargetBundle(), $field_bundle_map[$field_definition->getName()]['bundles']);
   ...
   +      $field_bundle_map[$field_definition->getName()]['bundles'] = array_values($field_bundle_map[$field_definition->getName()]['bundles']);
   

   If we keyed these by bundle, we could save the array_search right?

2. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionListenerInterface.php
   @@ -0,0 +1,47 @@
   + * Defines an interface for reacting to field definition creation, deletion, and updates.
   

   nit > 80

3. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionListenerInterface.php
   @@ -0,0 +1,47 @@
   +   * @see purgeFieldData()
   

   Should this be a FQ reference?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -680,6 +698,70 @@ public function getFieldMapByFieldType($field_type) {
+    $this->getStorage($field_definition->getTargetEntityTypeId())->onFieldDefinitionCreate($field_definition);
+    $field_bundle_map = $this->keyValueFactory->get('entity.definitions.field_map')->get($field_definition->getTargetEntityTypeId());
...
+    $this->getStorage($field_definition->getTargetEntityTypeId())->onFieldDefinitionDelete($field_definition);
+    $field_bundle_map = $this->keyValueFactory->get('entity.definitions.field_map')->get($field_definition->getTargetEntityTypeId());

It would be useful to add an empty line here to show that the call to the storage handler is not related to the field map stuff below.

Also, in both methods, it looks like we can save a few stack calls if we initialize $target_entity_type_id with $field_definition->getTargetEntityTypeId() and $field_name with $field_definition->getName() from the start. For an install profile that adds 100 fields, it means we can save 800 stack calls.

+++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
@@ -68,6 +68,21 @@ function entity_schema_test_entity_bundle_field_info(EntityTypeInterface $entity
+    // Notify the entity storage that our field is gone.
+    \Drupal::entityManager()->onFieldDefinitionCreate($field_definitions['custom_bundle_field']);

The comment doesn't sound right :)

+1 on the concept, that's probably our best way out.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -642,19 +645,34 @@ public function getFieldMap() {
   +            $base_fields = $this->getBaseFieldDefinitions($entity_type_id);
   +            foreach ($base_fields as $field_name => $base_field_definition) {
   

   I would tend to keep the inline var, but matter of taste, feel free to ignore

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -642,19 +645,34 @@ public function getFieldMap() {
   +              $this->fieldMap[$entity_type_id][$field_name]['type'] = $base_field_definition->getType();
   +              $this->fieldMap[$entity_type_id][$field_name]['bundles'] = array_combine($bundles, $bundles);
   

   For clarity, this could be

   $this->fieldMap[$entity_type_id][$field_name] = [
     'type' => ...,
     'bundles' => ...,
   ];
   

   since this is initializing the entries ?

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -642,19 +645,34 @@ public function getFieldMap() {
   +          foreach ($field_bundle_map as $field_name => $field_bundles_type) {
   

   $field_bundles_type is not a great var name ;-) It is indeed an array with the 'bundles' and 'type' entries for the field, but well, that's a bit litteral (and coupled to the current content)

   What about $map_entry ?

4. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +698,83 @@ public function getFieldMapByFieldType($field_type) {
   +    $field_bundle_map = $this->keyValueFactory->get('entity.definitions.field_map')->get($entity_type_id);
   

   Missing a comment for the code block ?
   '// Update the field map collection" ?

   Related : the collection name (entity.definitions.field_map) and the var names that refer to its content ($field_bundle_map, $entity_field_bundle_map) are a bit confusing with respect to $this->fieldMap.

   The former is the subset of the latter for non-base fields, right ? Not sure how we could make that relationship clearer, but calling them both field_map is slippery :-)

5. +++ b/core/modules/field/src/Entity/FieldConfig.php
   @@ -160,7 +160,7 @@ public function preSave(EntityStorageInterface $storage) {
   -      $entity_manager->getStorage($this->entity_type)->onFieldDefinitionCreate($this);
   +      $entity_manager->onFieldDefinitionCreate($this);
   
   @@ -174,7 +174,7 @@ public function preSave(EntityStorageInterface $storage) {
   -      $entity_manager->getStorage($this->entity_type)->onFieldDefinitionUpdate($this, $this->original);
   +      $entity_manager->onFieldDefinitionUpdate($this, $this->original);
   
   @@ -221,7 +221,7 @@ public static function postDelete(EntityStorageInterface $storage, array $fields
   -        \Drupal::entityManager()->getStorage($field->entity_type)->onFieldDefinitionDelete($field);
   +        \Drupal::entityManager()->onFieldDefinitionDelete($field);
   

   <3

re : entity.definitions.field_bundle_map / $field_bundle_map

right, so consistency between the two is cool, but the name is not too telling either :-)

We have "the field map", which is internally built on top of a smaller map stored in a collection and manually updated for non-base fields (= configurable fields ? bundle fields ?)
--> maybe bundle_field_map ? config_field_map ?

Discussed with @catch, @effulgentsia, @webchick and @xjm. This meets the performance criteria for a critical. The performance improvement is measured in seconds once there are more than a few hundred fields.

Thanks for bearing with me :-)
This looks ready to me.

Let's do eet :)

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +699,85 @@ public function getFieldMapByFieldType($field_type) {
   +    $this->keyValueFactory->get('entity.definitions.bundle_field_map')->set($entity_type_id, $bundle_field_map);
   

   Still struggling with this being in key/value and not cache, especially given the gets/sets are right next to cache gets/sets. A cache being expensive to build isn't sufficient reason to put something into key/value for me - if the issues is invalidations we can and have been trying to reduce the frequency of those.

   If it really, really does have to be in key/value, then it needs a comment explaining why and I couldn't see one.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +699,85 @@ public function getFieldMapByFieldType($field_type) {
   +    // If the field map is initalized, update it as well, so that calls to it
   

   Nit: initialized.

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +699,85 @@ public function getFieldMapByFieldType($field_type) {
   +    // If the field map is initalized, update it as well, so that calls to it
   

   Same typo on initialized.

Overall this seems good, but also it'd be good to know numbers here now that the config issue is in.

Nice work @Berdir.

Yep I profiled the patch a couple of days ago wrt to config entity delete - it's now checking dependencies that makes this expensive.

The new documentation is really helpful. Let's see if @catch feels the same way :)

@Berdir : I didn't really get what were the "this->fieldMap" modifications mentioned in your recent comments
("let's try to move it to clearCachedDefinitions()" in #24, "removed the fieldMap part again" in #31), I wasn't able to see them in the interdiffs ?

Other than that, nitpicks :

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +707,90 @@ public function getFieldMapByFieldType($field_type) {
   +    if (!isset($bundle_field_map[$field_name])) {
   +      // This field did not exist yet, initialize it with the type and empty
   +      // bundle list.
   +      $bundle_field_map[$field_name] = [
   +        'type' => $field_definition->getType(),
   +        'bundles' => [],
   +      ];
   +    }
   +    $bundle_field_map[$field_name]['bundles'][$bundle] = $bundle;
   +
   +    // Update the field map collection.
   +    $this->keyValueFactory->get('entity.definitions.bundle_field_map')->set($entity_type_id, $bundle_field_map);
   +    $this->cacheBackend->delete('entity_field_map');
   

   Nitpick, code grouping looks a bit off : all the code up to $this->cacheBackend->delete('entity_field_map'); (excluded) falls under the "// Update the bundle field map, used by EntityManager::getFieldMap()." comment ?

   Then, the line about $this->cacheBackend is a bit on its own, could deserve its own comment ?
   (side note, it's a bit misleading that this cache delete() looks disconnected from the corresponding cache read / writes in getFieldMap(), since those use a shortcut API from DefaultPluginManager. Could getFieldMap() use regular $this->cacheBackend->get() / ->set() too ?)

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -680,6 +707,90 @@ public function getFieldMapByFieldType($field_type) {
   +    // Update the bundle field map, used by EntityManager::getFieldMap().
   +    $bundle_field_map = $this->keyValueFactory->get('entity.definitions.bundle_field_map')->get($entity_type_id);
   +
   +    // Unset the bundle. If there are no bundles left, remove the field from
   +    // the map.
   +    unset($bundle_field_map[$field_name]['bundles'][$bundle]);
   +    if (empty($bundle_field_map[$field_name]['bundles'])) {
   +      unset($bundle_field_map[$field_name]);
   +    }
   +    $this->keyValueFactory->get('entity.definitions.bundle_field_map')->set($entity_type_id, $bundle_field_map);
   +    $this->cacheBackend->delete('entity_field_map');
   

   Likewise re: code grouping and comments

Thanks @Berdir, code looks good.

Regarding where we should reset $this->fieldMap, in the end the current patch leaves it untouched in clearCachedFieldDefinitions().
So a FieldConfig CRUD :
- updates the collection's partial map and the static full map through onFieldDefinitionXxx(),
- and then its postSave()/postDelete() wipes the static map though clearCachedFieldDefinitions() anyway, so it will be built again from the collection if we need it again.
?
Not that I have major complaints (not ideal, but doesn't necessarily have to block this patch here), just checking I got things right.

Although, this made me notice : the existing EM::onFieldStorageDefinitionXxx() methods currently all internally take care of calling clearCachedFieldDefinitions(). For consistency, shouldn't it be the same for the similar onFieldDefinitionXxx() methods added here ?

So I have two concerns with using key/value as opposed to just having the cache item:

1. If the key/value store gets out of sync (a failed write or similar), then neither drush cr nor rebuild.php will reset it - could be very difficult to track down/debug in those cases - you'd have to manually compare the data in each to figure out the problem probably.

2. We're using key/value here specifically because we clear caches too often. In theory it ought to be possible to have a write-through cache that's never cleared during the normal lifetime of a site - unless someone runs drush cr.

#1 isn't really resolvable here - we just have to decide whether to accept that risk vs. the risk of a site going down because of a lengthy rebuild - that is probably an OK trade-off.

#2 is a good goal, but not sure if we'll get there for 8.0.0.

Issue like #2487287: Optimize/clean up cache clears when saving/deleting FieldConfig entities should get us closer to that though, and then we could remove the k/v store in a later minor release if core itself doesn't flush the cache entry any longer.

Given all that, I've gone ahead and committed/pushed the patch to 8.0.x, but would really like to see us get to the point where it's not necessary again so we can rip it out.