Filtering block plugins by context is slow

See also this issue #2992017: \Drupal\Core\Plugin\Context\EntityContextDefinition::getSampleValues() needlessly generates full sample entities

Ok here 2 different approaches.
without the patches doing the request for takes about 1000ms every time.

1. 2994550-3-cache_in-lb.patch caches the definitions directly in \Drupal\layout_builder\Controller\ChooseBlockController::build(). It decreases the 2nd request to 100ms. But this approach also would cache the results of the hooks fired in \Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions()
2. 2994550-3-cache_generic.patch caches the definitions in \Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions() and does cache the results of the hooks. It decreases the 2nd request to 200ms. It requires changes to FilteredPluginManagerInterface. The way to get around the change is would be to have a special "_cache_id" key in $extra

Both approaches feel much faster. I think the 2nd approach is much better because it would allows more flexibility because of the hooks. For instance you could use a hook in layout builde to check if any existing field has already be placed on the layout and only allow to be placed once. If we cached the hook calls you can't do that.

The 2nd approach also allows other modules to use this functionality. We could use it for the block UI also.

+++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
@@ -16,12 +16,25 @@
+    if ($cache_id) {
...
     else {
...
+      if ($cache_id) {

This logic seems off. The second if ($cache_id) will never fire.

I think I like the second approach more.

Hmmm. we need to invalidate this cache when the plugin definitions are cached.

1. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -16,12 +16,25 @@
   +      if (!empty($cached->data)) {
   +        $definitions = $cached->data;
   

   If you just return here

2. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -16,12 +16,25 @@
        else {
   -      $definitions = $this->getDefinitions();
   +      if (!is_null($contexts)) {
   +        $definitions = $this->getDefinitionsForContexts($contexts);
   +      }
   +      else {
   +        $definitions = $this->getDefinitions();
   +      }
   +      if ($cache_id) {
   +        $cache = \Drupal::cache();
   +        $cache->set($cache_id, $definitions);
   +      }
        }
   

   You can remove the else and this section starts making more sense and the cache actually gets populated(which isn't happening currently).

   You can also check that the object exists and re-use the load at the beginning.

#3 I don't think we could go with first approach, but second approach is more reasonable, as outlined by your conclusion Ted.

This cache would only expire if the plugin definitions are changed, which would rarely (if ever) happen on production? Correct?

This cache would only expire if the plugin definitions are changed, which would rarely (if ever) happen on production? Correct?

The problem is it won't even clear then. This need to integrated into plugin manager

We have \Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface::clearCachedDefinitions(), which is implemented by DefaultPluginManager.
Perhaps we can use that instead of Drupal::cache()?

#8.1
We can't just return there because the alter hooks have not been fired yet. I wasn't caching the later hooks because they have logic that depends on the current state of the layout suchs if a plugin is already the layout remove it from definitions list so it is not placed twice.

#11 yes we can use that by changing \Drupal\Core\Block\BlockManager to use a cache tag in addition to an id. So if we use the same cache tag, which we can ask the plugin manager for, our filtered definitions will also be cleared.

@todo I assume if do this we would need an update hook to either clear the block plugin cache or to reset it with the new tag. Not doing that right now.

Here is patch in this direction. I will self review to explain more

1. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -12,16 +18,38 @@
   +  use UseCacheBackendTrait;
   ...
   +    if ($this->useCaches) {
   

   We can use this trait here and if the plugin manager itself does not use the trait it will not set $this->useCaches so our caching logic will not run.

2. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -12,16 +18,38 @@
   +      if ($cache_tags = $this->getCacheTagsForFilterDefinitions()) {
   

   This seems to be the easy way to make sure our filter definitions are cleared when the the definitions themselves are cleared. We could update \Drupal\Core\Plugin\DefaultPluginManager::clearCachedDefinitions() but this patch should work for plugin managers that aren't extending DefaultPluginManager.

   Also clearCachedDefinitions() already contains logic to delete multiple caches using cache tags. So we can use this functionality.

3. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -67,22 +67,11 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   -    $cache_id = implode(':', [
   -      'layout_block_list',
   -      $section_storage->getPluginId(),
   -      $section_storage->getStorageId(),
   -      $delta,
   -      $region,
   

   We don't need to take any of this data in consideration for the cache id because it not passed to getDefinitionsForContexts() so the result won't be varied base on this. That is why we can move all of this caching logic inside FilteredPluginManagerTrait

4. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -72,4 +100,61 @@ protected function themeManager() {
   +    foreach ($contexts as $context_id => $context) {
   +      $context_unique_cache_parts = [];
   +
   +      $context_data = $context->getContextData();
   +      if ($context_data instanceof Language) {
   +        $context_unique_cache_parts[] = $context_data->id();
   +      }
   +      elseif ($context_data instanceof EntityAdapter) {
   

   I don't know if there is a better way than to react to known types of context data but this would get us pretty far.

5. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -72,4 +100,61 @@ protected function themeManager() {
   +      else {
   +        // If we can not handle all contexts then return NULL to avoid caching.
   +        return NULL;
   

   If we hit a context type that we can't handle we shouldn't make a cache id because we wouldn't have unique cached filtered definitions for every different combination of contexts.

Ok here is something I was just playing around with on top of #14. Probably will not include in subsequent patches because it feels hacky. but it does make it faster for the first "add block" click. 😜

But here I am registering a shutdown function that just calls getFilteredDefinitions() with the available contexts. Therefore saving them in cache if they haven't been saved.
Because it is shutdown function is called after the layout builder is actually presented. This gets around the problem the first call to "add" block will still be slow because the definitions aren't cached.

Of course though the alter hooks will be needlessly called because weren't actually using the results. For this reason I use "_none" as the consumer so no hooks will fired for plugin_filter_block__CONSUMER

General observation, are we concerned about putting caching directly into the FilteredPlugin trait? It should be all guarded behind the cache ID but could be surprising.

1. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -12,16 +18,38 @@
   +      if ($cache_tags = $this->getCacheTagsForFilterDefinitions()) {
   +        if ($cache_id = $this->getCacheId($contexts)) {
   

   Honest question, is core allowing assignments in if's? I thought this block of code was broken for quite a while till I sorted out what was going on.

2. +++ b/core/lib/Drupal/Core/Plugin/FilteredPluginManagerTrait.php
   @@ -72,4 +100,61 @@ protected function themeManager() {
   +        /** @var \Drupal\Core\Entity\EntityInterface $entity */
   +        $entity = $context_data->getValue();
   +        if ($entity instanceof RevisionableInterface && $entity->getRevisionId()) {
   +          $context_unique_cache_parts[] = $entity->getRevisionId();
   +        }
   +        else {
   +          $context_unique_cache_parts[] = $entity->id();
   +        }
   

   Entity can be null. In assume that's why things are failing

So this is actually a very ancient issue. Fago and I discussed this way back during the creation of the plugin context system. The OP clearly outlines how this is a TypedData validation issue and not a Plugin Context issue. Plugin Context of course uses Typed Data & its validation, so it's easy to get our wires crossed as to where to build a solution. I'd personally dig through TypedData's validation first to see if there's a place we can introduce a static cache for things we've already validated. It'd have to account for change as well, so it can't just check id, but something like a super fast and simple hashing of the toArray'd data of the entity might be enough to ensure that we don't double validate things we've already validated but still validate objects which have changed.

Taking this approach would be good for the whole of Drupal and benefit anything that does entity validation. Likewise it should speed up contextual filtering for all plugin types and benefit contrib code as well. If there's no obvious place to go fixing this for Typed Data, only then would I consider solutions specific to LB. That's just my thought on this.

Eclipse

Heavy work-around post coming here (for those who needs this asap)

Similar, but still difference to #20 here's how I did it:

So, the idea is to filter the definitions down, a lot before running it through ContextAwarePluginManagerTrait::getDefinitionsForContext().

1. Add a service provider to override BlockManager
2. Override getFilteredDefinitions, call the parent method, on those consumers you don't really target (to not break other things), so I target specifically layout_builder consumer. Use $this->getDefinitions();
3. Get the definitions and then filter for how you're use cases are fitting.
4. Now run it through ContextAwarePluginManagerTrait::getDefinitionsForContext() and pass your already filtered definitions for the second argument.

For me: 14 seconds to 600 ms (on Docker, macOS hence the extra sloweness)

@EclipseGc thanks for larger context. I will look into your suggestion

If there's no obvious place to go fixing this for Typed Data, only then would I consider solutions specific to LB.

The patch in #12 is no longer LB specific.

The patch #16 adds a LB enhancement but that should be separate issue

Trying to figure how improve the performance tried a couple other approaches.

First to get a baseline of performance I surround this line

$definitions = $this->blockManager->getFilteredDefinitions('layout_builder', $this->getAvailableContexts($section_storage), [
'section_storage' => $section_storage,
'delta' => $delta,
'region' => $region,
]);

in \Drupal\layout_builder\Controller\ChooseBlockController::build() with calls to microtime(). I uploaded
2994550-timer-tester.txt patch to which I used with all the patches to figure out timing.

1. for 8.7.x no changes
   (cold cache)1.6031801700592
   (warm cache)0.84004712104797
2. Patch in #12
   (cold)1.6019740104675
   (warm)0.0071160793304443
   but since this caches on entity ids
   For each new entity it jumps back to 0.84921383857727 even if you don't clear cache
3. first try was to look at @EclipseGc's suggestion in #19

   I am not super familiar with this code but it appears that \Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validate() will be called multiple times with same entity and constraints. This makes sense because for instance field blocks provided by Layout Builder will all be the same for all bundle fields.

   So I tried to keep a static variable to determine if a entity with a set of constraints has already be validated.
   (cold)0.91364502906799
   (warm)0.13158702850342
   This improvement doesn't reset with every new entity.

4. Then I tried to improve \Drupal\Core\Plugin\Context\ContextHandler::filterPluginDefinitionsByContexts()
   Looking at this method

   public function filterPluginDefinitionsByContexts(array $contexts, array $definitions) {
       return array_filter($definitions, function ($plugin_definition) use ($contexts) {
         $context_definitions = $this->getContextDefinitions($plugin_definition);
   
         if ($context_definitions) {
           // Check the set of contexts against the requirements.
           return $this->checkRequirements($contexts, $context_definitions);
         }
         // If this plugin doesn't need any context, it is available to use.
         return TRUE;
       });
     }
   

   checkRequirements() will be called once for every plugin definition. But actually there are going to be a lot of plugin definitions that return the same $context_definitions.

   For instance for \Drupal\layout_builder\Plugin\Derivative\FieldBlockDeriver::getDerivativeDefinitions() all the fields for each bundle will have the context definitions.
   $contexts is passed into the method so that will not change per call to the method.

   So really we only need to call checkRequirements() for every combination of $context_definitions. So for just field blocks this would reduce the calls from once per field on a bundle to once per bundle.

   This improves performance more.
   (cold cache)0.84928894042969
   (warm cache)0.083363056182861
   This improvement doesn't reset with every new entity.

   This seems like a simpler solution changing RecursiveContextualValidator and is more performant. Though someone how know the validator logic might be able to find a better fix there.

   But it doesn't mean we couldn't do both. I would say if my change to filterPluginDefinitionsByContexts() makes sense we should do that here and then look at improving at the validator level also.

Uploading the patches for these 2 new approaches and we will see what breaks.

So here is fix for the #23.4 patch for the test that failed.

It failed because the constraint value was string and not an array. Not sure if a constraint value could be anything.

One option for getRequirementsKey() would just be handle the case where it is possible to make the unique and return NULL otherwise. In the case where the key cannot be made just call checkRequirements()

A constraint is array|null, so that test is wrong (I wrote it).

'mismatched_constraint_name' => 'mismatched_constraint_value'
should be
'mismatched_constraint_name' => ['mismatched_constraint_value']

@tim.plunkett thanks for clarification. I didn't see that before I this patch. Maybe having the extra checking is not bad incase of bad data?

I changed the method name to getContextDefinitionsUniqueKey() and handle other values by returning a NULL for the key. So it still work if there other types of values it just won't have performance increase.

Also changed to hashing an array rather string concatenation. Actually is a little faster.

I checked with the Umami Profile

1. 8.7.x
   (cold cache)2.6849451065063 🙀
   (warm cache)1.7270698547363
2. patch in #28
   (cold cache) 0.99855804443359
   (warm cache)0.13207411766052 🎉

Also just a note I have tried each of these multiple times and times are very consistent.
cold cache = clicking "Add block" right after "drush cr"

Realized we probably don't need getContextDefinitionsUniqueKey() and can just use
$context_definitions_key = md5(serialize($context_definitions));

Same performance as #29

How many definitions do you currently have in your tests? It would be interesting to see Patch in #28 with a large number of definitions. In my current project, there are currently 400 (and growing)

@johndevman just checked and with the Umami profile there are 364 definitions. Mostly field blocks.
This is why there is such a big performance boast because it goes from calling checkRequirements() on every field block to only calling for every bundle because all the fields for the same bundle will have the same constraints.

So as you add more fields to bundle the time should not spike. Only if you added tons of bundles.

I had this sandbox project for adding tons fields programmatically for Drupal 7 but I haven't updated to Drupal 8 yet.

Re #33

My understanding of using a static variable within the closure is that it is re-initialized with each call to filterPluginDefinitionsByContexts().

Good question!
yes this is the case and this is intentional. it really simplifies have to worry about anything else changing between the calls to filterPluginDefinitionsByContexts()..

1. Would there ever be time based constraints where the static variable in the class would not be valid anymore? How would we know?
2. If we used static variable in the class then
   $context_definitions_key = md5(serialize($context_definitions));
   would no longer work because we would have to factor in $contexts because these would change between calls to this method.
   So you would have to do something like

   $context_definitions_key = md5(serialize([$contexts,$context_definitions]));
   

3. For the case of layout builder "Add bocks" dialog itself there would be no performance boost at all.

   This is because in the server request to create the dialog this method is only called once anyways. So keeping the static variable at the class level would have no benefit in this case.

   The other times I know it would called in core is showing the list of sections in the layout builder and loading the visibility conditions in the block form. It think it is only used in those places because there are the only plugin managers that use ContextAwarePluginManagerTrait.

   In both those instances in my debugging filterPluginDefinitionsByContexts(). is also only called once in the request.

   So at least in core this method is used to display a list of plugins to choose or configure on the form and there aren't multiple different types of plugins being shown to the user at the same time so caching on the class level won't help.

   Also at least for the all the plugins handle by LayoutPluginManager don't have constraints so checkRequirements() is called anyways.

So think fact that static is variable is within the closure and gets re-initilized everytime is actually a benefit because we don't have to worry about weird cache edge case that might be in contrib that we can't think of.

1. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -18,11 +18,18 @@ class ContextHandler implements ContextHandlerInterface {
   +      static $checked_requirements = [];
   

   Rather than introducing a static, it may be better to use @cache.static. That results in a more recognizable, formal pattern, which will make it easier to maintain this.

2. In the hypothetical case of subrequests, this would break. This should vary by current request, I think? If so, another reason to use @cache.static?
3. Using md5() should be avoided, because some silly government programs don't want a single mention of it, even when it's not used for non-cryptographic purposes.

4. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
   @@ -18,11 +18,18 @@ class ContextHandler implements ContextHandlerInterface {
   +        // Generate a unique key for the current context definitions. This will
   +        // allow calling checkRequirements() once for all plugins that have the
   +        // same context definitions.
   

   I am not familiar enough with the context system to know whether this is safe to do or not. Ideally, a Context System maintainer would review this.

   It seems like this could be an optimization to the context system itself. Oh, wait, this is a change to the context system!

   Probably @tim.plunkett is the best person to review this?

So Tim asked me to look at this. Per his own points to me, this would be better if it lived in the checkRequirements() method of the same class. That being said, the big issue with this code is that it only considers what contexts to return on subsequent invocations for the same context definition(s), but that's insufficient because different context arrays could be passed with a uniform set of definitions. So you really need a hash of the definitions AND the contexts in order to cache this properly.

Also, it's worth mentioning that a thinner version of the context definition might be preferable to build the hash from as things like "label" and "description" are totally irrelevant when determining context requirement matching. Preprocessing the definitions down to only the relevant bits used in requirements matching could make the matching more accurate. I don't know if it would be worth the cycles to do it though so I'm just pointing out that it's possible.

Also, given that the tests didn't catch the issue between contexts & definitions, we should probably add a test to ensure this logic never changes in such a way as to improperly cache the way this patch would.

Eclipse

Superb answer, thanks a lot! 👌

@Wim Leers , @EclipseGc thanks for the reviews.

re

That being said, the big issue with this code is that it only considers what contexts to return on subsequent invocations for the same context definition(s), but that's insufficient because different context arrays could be passed with a uniform set of definitions. So you really need a hash of the definitions AND the contexts in order to cache this properly.

But the static variable $checked_requirements is inside the anonymous function so it is not static on scope of filterPluginDefinitionsByContexts(). So for the life of static var the $contexts will never vary. So we don't need to take that into consideration for caching.
Everytime filterPluginDefinitionsByContexts() is called $checked_requirements will be reset to an empty array. I just double checked to be sure. So yes you are not getting the caching benefits from putting it somewhere else but is also very short lived so you don't have to worry about what might happen if filterPluginDefinitionsByContexts() called with different contexts. Still with just this there is a big performance improvement with layout builder block listing.

Rather than introducing a static, it may be better to use @cache.static. That results in a more recognizable, formal pattern, which will make it easier to maintain this.

So the static inside the anonymous function is more controlled a smaller scope to cache on.

I think we need a comment explaining some or all of that. But no functional changes.

1. Looking at my explanation in #39 and if I am correct(which I think I am), I wanted to see if the code could be simplified so that it doesn't actually need to have a comment for the scope of the static variable inside the anonymous function.

   But the static variable $checked_requirements is inside the anonymous function so it is not static on scope of filterPluginDefinitionsByContexts(). So for the life of static var the $contexts will never vary. So we don't need to take that into consideration for caching.

   If this is the intention I think we can just use move the $checked_requirements outside the anonymous function and make it non-static. Then we just use it in the anonymous function by reference.

   This way $checked_requirements will be reset have time filterPluginDefinitionsByContexts() is called but within 1 call we only call checkRequirements() once for any combination of $context_definitions.

   I think this is functionally the same but hopefully easier code to read.

2. re #36.3
   Changing to use hash('sha256', serialize($context_definitions));

   I copied this from \Drupal\aggregator\ItemsImporter::refresh()

I think this is _much_ clearer, thanks!

I confirmed with @tedbow that those numbers in the IS are whole seconds. This is significant enough to qualify as critical I think according to the priority definitions: https://www.drupal.org/core/issue-priority#critical-task

The patch will help all sorts of scenarios where we have lots of plugins being filtered by context. Since Layout Builder is exposing this to content authors in a big way, I think it also qualifies as perf gate for LB.

A 1.65 KB patch that improves performance in some circumstances by seconds. Awesome! 👏

Random fail

We're in commit freeze atm, but leaving a comment to say #41 looks great and really nice find.

Committed and pushed to 8.7.x. Thanks!

Does this need a backport? Is it safe for backport? It did not cherry-pick cleanly.

Oops, got overzealous. Need to know if it's something we should do in 8.6.x as well.

Discussed with @tedbow. This is probably safe for backport, and does not require a cache clear -- the performance improvement will just take effect the next time the cache is cleared. So I queued an 8.6.x test run.

Cherry-picked to 8.6.x. Thanks!