BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request

I'm curious, how much of the actual request do we talk about? I see 25ms gain, which is a LOT, but especially a lot from the measured 100ms we had ...

Yeah I guess we would need a block_list tag and invalidate that on save time.

Adding some related issues. I think we should explore whether or not we can make config entity retrieval faster.

We have a config:block_list cache tag, that's the list cache tag of block config entities.

Yes, I've seen this too in profiling, obviously, this will get slower the more blocks you have in the system, as we need to load them all. I still think it was a bad idea to remove the forced theme ID prefix like fields have for example from blocks. We have optimizations in place for config entity queries on (partial) ID matches, that would be much faster.

This adds the list cache tag, at least one test was fixed by this, let's see about others.

Fixing the unit test and adding coverage for the change.

Working on the \Drupal\block\Tests\BlockSystemBrandingTest fails.

Oooh, bad test, modifying config directly like that...
Nice to know that using the correct API fixes the problem :)

+++ b/core/modules/block/block.services.yml
@@ -24,4 +24,4 @@ services:
   block.repository:
     class: Drupal\block\BlockRepository
-    arguments: ['@entity.manager', '@theme.manager', '@context.handler']
+    arguments: ['@entity.manager', '@theme.manager', '@context.handler', '@cache.data']
diff --git a/core/modules/block/src/BlockRepository.php b/core/modules/block/src/BlockRepository.php

Entities are usually in the entity bin, is there a reason that data was used here?

IMHO this code is looking great, especially because the approach is simple, understandable etc. especially compared to the other suggestions out there.

Knowing you thought about it and decided for cache.data works for me.

I just added the cache tag, it has been reviewed by others already. so I think it's OK if I RTBC this.

Agreed with @dawehner: A nice performance improvement that is very simple in itself, thanks to cache tags :)

Patch looks great; doing a quick round of benchmarking.

My benchmarking (as UID 2, frontpage) shows that this doesn't actually gain us much. The IS has nice before vs. after XHProf screenshots, but they only look at BlockRepository::getVisibleBlocksPerRegion(), not the overall request. My benchmark results suggest that the majority of the cost was actually in initializing some things, which are now just being initialized later/elsewhere.

Before

Requests per second:    12.39 [#/sec] (mean)
Time per request:       80.735 [ms] (mean)
Time per request:       80.735 [ms] (mean, across all concurrent requests)
Transfer rate:          147.23 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    75   81   1.5     81      89
Waiting:       67   71   1.4     71      78
Total:         75   81   1.5     81      89

After

Requests per second:    12.38 [#/sec] (mean)
Time per request:       80.797 [ms] (mean)
Time per request:       80.797 [ms] (mean, across all concurrent requests)
Transfer rate:          147.12 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    74   81   1.6     81      95
Waiting:       66   72   1.5     71      86
Total:         74   81   1.6     81      96

Conclusion

In any case, I cannot see the big gain described in the IS.

Back to NR per #23.

Looking at it with XHProf:

1. 40848 -> 40040 function calls (minus 808)
2. \Drupal\block\BlockRepository::getVisibleBlocksPerRegion() remains around 8-9 ms, because unlike catch, the loading of the entities only took 1.5-2.5 ms on my machine, the cache get takes 0.75 ms.

That, combined with some code being loaded later in the request, is probably why I'm not seeing a performance improvement.

This may still be worth it for the reduction in number of function calls, but in practice, the gain is very small in any case.

Note: I tested with the Standard install profile, catch tested with Minimal.

Try it with 20, 30, 40 blocks and enable a few different themes.

I didn't expect a huge gain in profiling, and certainly not with a default installation.

Instead of execution time, try to compare the amount of database queries. I'd expect we save at least the query on the config table there, and we have a single cache get instead of a cache get multiple. But as @catch said, the downside is that those cache gets are currently in APC, while this would be the database.

One thing that might offset the improvement from the caching here is that we're moving from APCu/chainedfast for cache.config to database for cache.data - putting this in a chained backend would probably make more sense.

+1

Works for me.

In fact, I think we should consider that config entity caching should use the cache_config table as well instead of cache_entity, for the exact same reason. We do use that for roles for example...

Even if this doesn't have a big impact on performance, I think we should still go ahead with this. We are definitely saving queries now, and that is worth it IMHO. The impact of this will likely be more visible on sites under load. queries are always very fast when testing with ab, because then the query caches are warm, that's not always the case.

Oh, I'm confused, roles/config entities only use static cache. Forget what I said :)

Pretty sure this needs to be profiled again after #2430219: Implement a key value store to optimise config entity lookups. Should be quite a bit faster now in HEAD, especially if you have many blocks.

(In theory) Easy rebase, git auto merged it :)

+++ b/core/modules/block/src/BlockRepository.php
@@ -74,7 +85,18 @@ public function getVisibleBlocksPerRegion(array $contexts) {
+    if ($cached = $this->cacheBackend->get($cid)) {
+      $blocks = $cached->data;
+    }
+    else {
+      $blocks = $this->blockStorage->loadByProperties(['theme' => $theme]);

Should we document that this gets rid of a keyvalue store and all the overhead of config queries, and so provide a even better way to serve Drupal without a database on a warm cache?

Thank you

+++ b/core/modules/block/block.services.yml
@@ -24,4 +24,4 @@ services:
+    arguments: ['@entity.manager', '@theme.manager', '@context.handler', '@cache.config']

I'm really uncertain about putting something that's not config in the config cache. Especially just because it is quicker.

It kind of is config :)

We can also use a different cache bin that uses apcu by default. Both discovery and bootstrap would be fine with me.

How about adding a @cache.config_query service, to make that distinction clear? Wouldn't that address Alex' concerns, and point D8 contrib in the right direction as well?

#45 makes me think - should we make caching expensive queries part of ConfigEntityStorage and introduce a special version of loadByProperties to do... something like CachedLoadByProperties()

#46: But if we make it too easy, won't that fill up APCu too quickly? We want it to be a very very very conscious choice to put things in APCu, I think?

Re #47 thats why I was suggesting a new method.

Right, but I'm wondering if that's not still "too easy". But I guess docs of such a new method could give sufficient warning :) So +1 to cachedLoadByProperties().

+++ b/core/modules/block/src/BlockRepository.php
@@ -55,7 +66,20 @@ public function getVisibleBlocksPerRegion(array $contexts) {
+      $this->cacheBackend->set($cid, $blocks, Cache::PERMANENT, ['config:block_list']);

Not too mad about hard coding 'config:block_list'] here.

Here's a patch that adds cachedLoadByProperties to ConfigEntityStorage and we can still mark the query service as lazy. I'm happy if we choose to split this into a separate issue - if everyone else thinks that is the way to go.

I think the patch looks great.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorageInterface.php
   @@ -88,4 +88,33 @@ public function loadOverrideFree($id);
   +   * Load entities by their property values from cache, if available.
   

   s/Load/Loads/

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorageInterface.php
   @@ -88,4 +88,33 @@ public function loadOverrideFree($id);
   +   * If there is no cache entry available the method falls back to
   

   s/entry/item/

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorageInterface.php
   @@ -88,4 +88,33 @@ public function loadOverrideFree($id);
   +   * Over-use of this method is discouraged since, by default, it stores entries
   

   s/entries in APC/data in APCu, which has a very limited amount of available storage space/

Thanks for the review @Wim Leers.

I wanted to RTBC, but then found my own profiling in #29 that shows the gain is not noticeable. But looks like the much more recent #37 and #38 show a different story.

Do we want more profiling before committing this?

Great :)

This patch looks great. Very nice implementation!

1. +++ b/core/core.services.yml
   @@ -152,6 +152,12 @@ services:
   +  cache.config_entity_storage:
   

   We don't use the "_storage" suffix for other cache bins, why do so here? Will this bin be intended for caching individual config entities too (if we ever choose to cache those)? If so, perhaps just "config_entity"? If not, perhaps "config_entity_query" or similar?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -111,12 +120,13 @@ class ConfigEntityStorage extends EntityStorageBase implements ConfigEntityStora
   -  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager) {
   +  public function __construct(EntityTypeInterface $entity_type, ConfigFactoryInterface $config_factory, UuidInterface $uuid_service, LanguageManagerInterface $language_manager, CacheBackendInterface $cache_backend) {
   

   Since this will break every contrib module that implements a config entity type with its own storage class that has its own constructor, this could use a change record. Also, a beta evaluation listing that as a (acceptable) disruption. And, an @param doc should be added here and in all the subclasses.

3. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
   @@ -856,6 +866,34 @@ public function testDeleteNothing() {
   +    // This to cachedLoadByProperties() will fall through to loadByProperties().
   ...
   +    // This to cachedLoadByProperties() will fall through to loadByProperties().
   

   s/This/This call/?

Right, but I'm wondering if that's not still "too easy".

Just brainstorming, feel free to reject, but if we want to make it less easy/accidental, and also avoid the BC break of #58.2, could we put cachedLoadByProperties() (along with a setCacheBackend()) in a trait, and let individual storages opt-in by using the trait (and calling the setter from within their createInstance() method)?

along with a setCacheBackend()

Clearly I wasn't thinking straight. There's no reason for the trait to require a setter. It can still be constructor injected. So really, the question is whether it makes sense for cachedLoadByProperties() to be its own interface/trait rather than requiring every subclass to have it and the corresponding $cache_backend dependency. Problem with that kind of opt-in though is that maybe it's the users of a config entity type that should decide if/when to benefit from cachedLoadByProperties() rather than it being the entity type's decision to make. So alternatively, would it make sense to have a separate service (e.g., ConfigEntityQueryCache) rather than baked into the storage classes?

I'm also OK with either. So this issue is Needs Work on either the CR, beta eval, and @param docs mentioned in #58.2, or on changing the approach to use a service that would make the CR and @param docs unnecessary. Leaving the decision to whoever wants to roll the next patch. If the next re-roll can also improve the name of the cache bin per #58.1 (I think config_entity makes most sense, since I think it'll make sense to use that for individual config entity caching as well, but am open to arguments against that), that would also be great, but not a blocker.

Doing some profiling on a site.

Caching the loadByProperties() is nice and what not, but in my case, that's just 2ms. However, in the same method, I'm seeing 9ms spent in $block->access() (If you don't see those numbers, try adding a few custom blocks to your site, those are extremely expensive).

The thing is, we actually have the cacheability for those access checks now. What if we just cache the whole method, using that cacheability metadata? In total, that's 11.5ms or 8% on a node page in my case.

Ah, I guess we'd need cache redirects for the contexts to work, which we currently only have for render caching.

Does SmartCache fix #64 / #65, or is there still a common scenario even with SmartCache where 5-10% of time is spent on $block->access()?

Yes, smartcache will cache that as well.

The question I guess is how many variations there are. If you have blocks that vary by path then block access needs to be checked by path, just like smartcache and then we don't gain much. Except smartcache might also end up caching per user on many pages.

I've discussed a completely different solution with Wim in IRC. The basic idea is to move the access check to #pre_render. Then we can cache access as part of the normal block render array. There are a few problems with that, though:

a) It would mean to remove access checking from BlockRepository, which is an API change and a tricky one because it could expose security issues in contrib/custom code which used it and isn't updated. We could solve that with a new optional argument to disable access checking? But that would also affect how the whole cacheability stuff works.
b) We need to solve the problem of regions with blocks but no blocks is displayed on a specific page, so they end up empty, that would then break if (region) checks in templates, add empty wrappers and so on.
c) And possibly the most problematic one (only just thought of this again). It would mean to vary the block result by the access cacheablity. If you have a custom block and only show that on a single page, then block access varies by url. And we'd have t' re-check access on every URL and also cache the built block for every URL even though the block output itself never changes.

So... I don't think that's a good solution, actually :) (Writing down things helps)

#67

a) Could be solved by keeping the existing API, and just adding a new API.
b) That's #953034: [meta] Themes improperly check renderable arrays when determining visibility. Region checks in templates are broken by design; they're fundamentally incompatible with lazy rendering.
c) This is true. But many blocks aren't shown on a single page, of course. So I'm not sure how big of a problem that'd actually be.

Not sure what you mean with your response to c). It is actually very common to have blocks that are only shown on certain pages through the path visibility condition. I can totally imagine sites with dozens of those blocks that are shown on special pages (e.g. a contact form or a certain node) and we have to be able to handle that in an efficient way. And node type visibility condition is per route as well.

Here's a pretty ugly but working implementation for this. This could also easily live in a wrapper of the block repository if we decide to not have it in core for now.

I think it's only ugly because it reimplements cache redirection (also incompletely, but I'm sure you know that, you said it's a PoC). Once we'd have #2551419: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way, this would be able to just reuse that.

But the principle looks sound to me. This is exactly what I've been saying we should/hoping we would have for a while :)

Fix the serialization problem and actually using the cache now. the failing aggregator test passed now.

Fixing those test fails.

Regarding #71: Berdir convinced me that we shouldn't do #2551419: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way first; that it's better to first have multiple implementations and then do #2551419.

This looks very close! :)

1. +++ b/core/modules/block/src/BlockRepository.php
   @@ -52,10 +86,22 @@ public function __construct(EntityManagerInterface $entity_manager, ThemeManager
   +        $cacheable_metadata = $cache_redirect->data['cacheable_metadata'];
   

   s/cacheable_metadata/cacheability/

2. +++ b/core/modules/block/src/BlockRepository.php
   @@ -52,10 +86,22 @@ public function __construct(EntityManagerInterface $entity_manager, ThemeManager
   +    $overall_cacheablity_metadata = new CacheableMetadata();
   

   s/cacheablity/cacheability/

   Perhaps: s/overall/page/ is slightly better?

3. +++ b/core/modules/block/src/BlockRepository.php
   @@ -80,7 +126,38 @@ public function getVisibleBlocksPerRegion(array &$cacheable_metadata = []) {
   +    foreach ($cacheable_metadata as $cacheable_metadata_region) {
   

   s/cacheable_metadata/cacheability/

   s/cacheable_metadata_region/region_cacheability/

I changed as mentioned by wim leers.

https://evolvingweb.ca/blog/speed-up-drupal-8-block-rendering-blackfire-io

It's worth pointing out that a TON of the work that this stuff is doing would be dramatically reduced if we had an index of blocks that appear on pages, in other words, PageManager. The blog's example of 140+ blocks on a single site is TOTALLY reasonable, and I've been making this point for literally years now. That's not to say that there's not an incremental improvement to be had by fixing the existing approach, but this is not the final solution. We need to build pages of blocks and execute and cache those pages. Just my opinion.

Eclipse

Adding D7 Bean.module performance as a related issue:
https://www.drupal.org/node/2734693

https://github.com/vasi/block_access_records

block_access_records
--------------------

Blocks are super powerful in D8. The page title can be a block, messages are a block, contact forms can be a block… This means your site may have dozens or even hundreds of blocks!

Unfortunately, checking which blocks should be visible on a page gets really slow when you have a lot of blocks. Drupal insists on loading all the blocks, then checking each and every one individually. I've seen it take up to 130 ms!

This module implements a fast way of checking, inspired by the node_access system. It generates a single query that does most of the work, and then only loads the blocks that are likely to be visible. On the site mentioned above, it cut over 90 ms off the time taken.

It currently works with all the visibility conditions in Drupal core: path, language, user role, and node type. It's also extensible, so you can add plugins that handler other types of conditions.

Caveats:
* If you have custom conditions, you'll have to extend this module. This module does at least ensure that you don't add conditions that aren't supported.
* Entity or block access hooks are only invoked on blocks whose conditions seem to pass, since we don't want to load other blocks. There's a new hook_block_visibility_alter() so you can add blocks whose conditions failed.
* This is not well tested, so beware of bugs.

There is a core issue about the slowness of core's block visibility checking: https://www.drupal.org/node/2479459 . I'm not sure the solutions there are ideal, because blocks may vary by path, which changes all the time. So this is my attempt at a better solution.

There is a core issue about the slowness of core's block visibility checking: https://www.drupal.org/node/2479459

That... that's this issue :)

There's an updated patch in the D7 version of bean.module , this might help.
see: #2734693: Add persistent caching to bean_block_info() to avoid loading all beans at run-time when using Panels / CTools content type block plugin

Since I'm not sure that caching this wouldn't just end up with a per-user/per-route cache context on all sites seventually, I'm not sure this will actually ever happen.

So I started a new approach to address the main performance issue that I'm seeing with block access checking: #2878673: Cache loadEntityByUuid() calls for block_content

#2878673: Cache loadEntityByUuid() calls for block_content is in now. Does that mean this is no longer needed? I think this at least needs a new round of profiling, because #2878673 will cause the numbers to change.

Yes, I'm not sure if this is still needed. Possibly keep it open but change to a normal issue until we have numbers that proof that it is still useful?

The main problem of this is IMHO that quite likely, the cache would end up being per-user per-url and then at that point fairly useless for a lot of cases, especially now with dynamic page cache.

Possibly keep it open but change to a normal issue until we have numbers that proof that it is still useful?

👍 Done!

Hi everyone, I found this issue after noticing a very long running `getVisibleBlocksPerRegion` call in my Blackfire trace. After profiling with and without this patch, I think there's still a need for this. I've included the 2 screenshots of `Blackfire's BlockPageVariant::build`.

This is with Drupal v8.9.16

patch #83 needs a complete reroll!

Bumping the priority up - In comment #93 I mentioned that this was fixed in the D7 bean module several years ago.

And #98/99 agreed to downgrade it to normal because we did other improvements, the config entity query is using an index, the content blocks are using a cache for the uuid lookup.

Unless someone shows numbers that makes this worth it, I don't think this as major. And it's a service, so a contrib module could override it and experiment with caching.

This is very different to the D7 bean module, and as pointed out, the cache would likely be basically end up with the same contexts as dynamic page cache.

Just found this again due to #3249710: hook_theme_suggestions_alter needs to statically cache results of visible blocks. Maybe it's a wrong approach, but their code is calling getVisibleBlocksPerRegion about 41x times and costing up to 2s.