\Drupal\block_content\Plugin\Derivative\BlockContent::getDerivativeDefinitions could use an aggregate query instead of loadMultiple

Tested this on a large site with over 3000+ block content entities where I was running into OOM issues.

After saving a block and therefore having the Block definition caches cleared:

No patch: ~152MB memory usage
With patch: 76MB memory usage

This looks great! One point on the MR to add a code comment. I still think we should do 🐛 block_content block derivatives do not scale to thousands of block_content entities Needs work but this will stop a lot of sites crashing in the meantime.

Added a comment

Looks great. We've got the other issue(s) open to move away from derivatives, I think we could backport this to 10.2 as well.

What happens when you add translations? I suspect that a translation with a different info/label will result in a separate key, and then it gets unpredictable which one you actually end up with? Maybe add a default langcode condition?

FWIW, this feels like a misuse of aggregate queries to me, but it is obviously a big improvement, so...

FWIW, this feels like a misuse of aggregate queries to me, but it is obviously a big improvement, so...

Can you elaborate on why, I do this fairly regularly in contrib and custom code. Trying to understand the implications.

Maybe add a default langcode condition?

Done

> Can you elaborate on why, I do this fairly regularly in contrib and custom code. Trying to understand the implications.

We always say that the only correct way to interact with entities is to load them, so that the appropriate hooks are running which *might* alter the data or also things like language negotiation*. We do not support any partial loading of field values or direct DB queries and an aggregate query is essentially that. Usually an aggregate query is combined with an aggregation function, where you count all nodes grouped by type or some other field and don't access the raw values. This is different.

*I just realized that this isn't working at all right now anyway, block content labels always show up in the default language only (we kind of implicitly assume in most plugin caches that all labels are t() objects and translated when accessed so we don't cache per language), that's not the case here. This is already not working in any useful way in HEAD, but I could imagine something like a ContentTranslation object that you feed with all the translations of a given content entity that then acts like a TranslatableMarkup object.

We always say that the only correct way to interact with entities is to load them, so that the appropriate hooks are running which *might* alter the data or also things like language negotiation*

Thanks, I was thinking that might be the reason, so its good to get confirmation.

Does that mean that this approach is a BC break, because someone could be implementing a load hook for block content and modifying the titles? Do we need to put this behind a feature flag that's opt-in? Thoughts @catch with your RM hat on?

I'm not aware of how trash module works but I got a bug report for a contrib module that leads me to suspect it might unset entities in a load hook if they're trashed. That might be something that has a side effect here.

Seems 10.2.3 is out. Should this go to 10.3?

Per @larowlan in #12.

Patch doesn't seem to apply anymore.

Rebased and fixed the PHPCS issue, also updated the deprecation message and CR.

Random failures seem to be biting us hard here. Will leave it for now and rerun later.

Indeed was random, re-running fixed.

left 1 comment on MR.

Replying to the bc question from... this time last year.

The main case for block titles being altered would be previewing forward revisions, but workspaces will handle that when you're in a workspace via entity query rewrites.

For trash module I think that it will rewrite entity queries to not include the entity when it's trashed - but we should ideally check with @amateescu.

I think this is a sufficiently serious issue that we should not put the fix behind a feature flag but would be good to confirm the trash case first.

Thanks @catch, back to NR.

After debugging a Drupal website running version 10.4, we frequently encountered 502 errors due to an excessive number of reusable block content entities. However, enabling the MR seems to have resolved the issue. Thank you very much!"

We found an issue with aggregated approach, when we add a new block into Layout of a page the block's plugin is considered as "broken/missing".
The root cause is in query itself, it does work differently in comparison with default approach:

count($this->blockContentStorage->loadByProperties(['reusable' => TRUE])); // 5932

count($block_contents = $this->blockContentStorage->getAggregateQuery()
      ->condition('reusable', TRUE)
      ->condition('langcode', $this->languageManager->getDefaultLanguage()->getId())
      ->groupBy('uuid')
      ->groupBy('info')
      ->accessCheck(FALSE)
      ->groupBy('type')
      ->execute();
); // 6277

Confirmed #25

I was able to reproduce this with the following steps:

1. Install Drupal on demo_umami
2. Create a new block in Spanish, NOT English (do not translate to english either)
3. Add the block to a page
4. Checkout this branch
5. Clear cache
6. See broken/missing block.

This only happens if the block's original language is not the site's default language (i.e English for demo_umami)

I'll look at fixing this and adding test coverage.

Fixed and added test coverage.

Marking this ticket to "Needs work" again as there are still some issues for blocks that have few translations with different info (see previous comment to MR)

Speaking internally with @larowlan this might not be viable for multilingual sites (@berdir pointed this out way back in #8 and #11)

@larowlan suggested we could detect > 1 language and fallback to the current behavior so sites that aren't multilingual benefit from the huge performance boost.

Regardless we'll need extra test coverage for the new issues mentioned above #28

So, if aggregate query cannot be use, then let's try chunked loadMultiple?
E.g. we have 1000 blocks and we do load them in memory 50 per iteration.
Some iterators/generators stuff might help with memory utilization (see https://www.drupal.org/project/drupal/issues/2577417 📌 Add an entity iterator to load entities in chunks Needs work )