Layout Builder attempts to builds sections to determine if it is disabled

Because the checkbox that corresponds to that method is only for defaults. (And technically overrides, but only because of how they are tied to defaults).

Layout Library is an example of a module that ties into this pipeline regardless of that checkbox, and this change will break the test in place confirming its support.

This issue is a dupe of #2981889: Performance Degradation in Layout Builder and other places likely . But that one hasn't been rescoped since 3023220 has landed...

I think we need a hook or event or something in this pipeline here to allow custom/contrib code to more directly affect this without going down the context path (which leads to typed data, which is the performance culprit)

Also, CustomElementsLayoutBuilderEntityViewDisplay, wow that's interesting

Here is try at basically allow section storage to determine if a view mode for bundle is supported.

1. \Drupal\layout_builder\SupportAwareSectionStorageInterface::isSupported() for section storage that implements this then they can tell if the bundle/view mode would be supported, not in use but supported. So defaults it would just check if the display is enabled.
2. \Drupal\layout_builder\SectionStorage\SupportAwareSectionStorageManagerInterface::notSupported() choose instead of isSupported() on the manger level because we can't always know that.

   Basically it checks to make sure all storage type implement SupportAwareSectionStorageInterface if not return false because we can't know. Then it loops throw all storage types to find $type:isSupported() === TRUE if now support it then we know it is not supported.

   This allows us to not check contexts if we know it won't be supported

   foreach ($entities as $id => $entity) {
         if (($this->sectionStorageManager() instanceof SupportAwareSectionStorageManagerInterface && $this->sectionStorageManager()->notSupported($entity->getEntityTypeId(), $entity->bundle(), $this->mode))) {
           continue;
         }
         $build_list[$id]['_layout_builder'] = $this->buildSections($entity);
   

Just took a quick look through patch in #6. Not sure if it is worth cross-linking to #3008924 or not, but on the chance it might, I thought I'd cross link it just in case it helps think forward in cases like these.

Corrected the link I meant to cite.

Not currently passing tests.

Here I have added a patch, lets wait for testbot response.

Still not passing tests
@ravi.shankar next time please include an interdiff to show what changes you made

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
@@ -99,13 +99,13 @@
-    $assert_session->waitForElementVisible('css', '.go', 9494949494949494);
...
+    $assert_session->waitForElementVisible('css', '.go', 9494949494949494);

Moving this won't help. Someone added that for local debugging and it needs to be removed. It timed out the bot.

I've rewritten the issue summary.
I think the original bug report is similar to #3008943: Clean up todo in InlineBlockEntityOperations::handleEntityDelete()
Downgrading this because it should always be possible to run the code that is in HEAD, this would only be a performance improvement, not a true bug fix.
If benchmarking shows a significant improvement, this can go back to major.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -272,7 +273,12 @@ public function buildMultiple(array $entities) {
   +    /** @var  \Drupal\Core\Entity\EntityInterface $entity
   +     */
   

   Unnecessary

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -445,4 +447,31 @@ public function setContext($name, ComponentContextInterface $context) {
   +    $storage = $this->entityTypeManager->getStorage('entity_view_display');
   +    $display = $storage->load($id);
   +    if (empty($display)) {
   +      $display = $storage->load("$entity_type_id.$bundle.default");
   +      if (!$display) {
   +        return FALSE;
   +      }
   +    }
   +    $supported[$id] = $display->isLayoutBuilderEnabled();
   

   This should be replaced with $this->isLayoutBuilderEnabled() (which already exists on the class)

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -438,4 +439,22 @@ public function isOverridden() {
   +    if ($view_mode !== 'default') {
   +      return FALSE;
   +    }
   

   Needs docs

4. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -438,4 +439,22 @@ public function isOverridden() {
   +    $storage = $this->entityTypeManager->getStorage('entity_view_display');
   +    $display = $storage->load("$entity_type_id.$bundle.$view_mode");
   +    $supported[$id] = $display->isLayoutBuilderEnabled() && $display->isOverridable();
   

   This feels very wrong to include logic about the Defaults within the Overrides.

   This should instead use getDefaultSectionStorage() (which will require context be set up)

5. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -129,4 +130,26 @@ public function loadFromRoute($type, $value, $definition, $name, array $defaults
   +  public function notSupported($entity_type_id, $bundle, $view_mode) {
   ...
   +        return FALSE;
   ...
   +        return FALSE;
   ...
   +    return TRUE;
   

   False is good here and true is bad, which is odd. But I get why the choice was made. But each of these return values should be documented

6. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -129,4 +130,26 @@ public function loadFromRoute($type, $value, $definition, $name, array $defaults
   +    $storages = [];
   

   Elsewhere in this class, $plugin is used, to help differentiate between definitions and plugins

7. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -129,4 +130,26 @@ public function loadFromRoute($type, $value, $definition, $name, array $defaults
   +    foreach ($storage_types  as $storage_type) {
   

   Extra space

8. +++ b/core/modules/layout_builder/src/SectionStorage/SupportAwareSectionStorageManagerInterface.php
   @@ -0,0 +1,21 @@
   + * Interface for section manager that knows whether it supports view modes.
   + */
   +interface SupportAwareSectionStorageManagerInterface extends SectionStorageManagerInterface {
   
   +++ b/core/modules/layout_builder/src/SupportAwareSectionStorageInterface.php
   @@ -0,0 +1,11 @@
   + * Interface for section storage that knows whether it supports a view mode.
   + */
   +interface SupportAwareSectionStorageInterface extends SectionStorageInterface {
   

   I have to read the implementation to understand what these interfaces are for.

9. +++ b/core/modules/layout_builder/src/SectionStorage/SupportAwareSectionStorageManagerInterface.php
   @@ -0,0 +1,21 @@
   +   * Whether a view mode is not support by any storage.
   +   *
   +   * @param $entity_type_id
   +   * @param $bundle
   +   * @param $view_mode
   +   *
   +   * @return bool
   +   */
   

   Incomplete docblock

Found this issue while investigating performance issues with Diff module's visual_inline diff. After profiling with Blackfire it showed 23,400 calls to Drupal\Core\Plugin\Context\ContextDefinition::isSatisfiedBy via buildSections.

This patch dropped the page load time of a particularly large revision diff (Node with ~100 paragraphs) from roughly 21 seconds uncached to 16 seconds uncached.

Unfortunately Blackfire's profiling time seems to be off locally as it's reporting the exact same time pre and post patch (and even with caches warmed).

There is certainly a huge performance benefit with this patch though, even just by looking at the page load time via Chrome dev tools.

I will work on the review above and post a new patch soon.

1. Removed
2. Fixed
3. Added
4. Still TODO - unsure what this change would look like
5. Added
6. Changed to $plugins
7. Fixed
8. Not sure what needs to change here. I agree the naming of classes and functions could be clearer.
9. Fixed

Over to NR to see if the docs and what not are OK and get feedback on 4.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
@@ -462,15 +462,7 @@
-    $storage = $this->entityTypeManager->getStorage('entity_view_display');
-    $display = $storage->load($id);
-    if (empty($display)) {
-      $display = $storage->load("$entity_type_id.$bundle.default");
-      if (!$display) {
-        return FALSE;
-      }
-    }
-    $supported[$id] = $display->isLayoutBuilderEnabled();
+    $supported[$id] = $this->isLayoutBuilderEnabled();

This change breaks things with the following exception:

Drupal\Component\Plugin\Exception\ContextException: The 'entity:entity_view_display' context is required and not present. in Drupal\Core\Plugin\Context\Context->getContextValue() (line 73 of core/lib/Drupal/Core/Plugin/Context/Context.php).

$this->isLayoutBuilderEnabled() is being called on a bare plugin (one created via SectionStorageManagerInterface::loadEmpty(). It doesn't have the context applied, so it can't introspect that. Which is the whole point, as this is when contexts are being determined.

Perhaps there's a middle ground here. Instead of adding static caching AND a whole new paradigm to checking these plugins before instantiating them, why not start with some static caching?

Just adding some info about this issue :

We'in investiguating performance issues on our side, and seeing that the buildSections() is called for every entity in a menu or a view seems to slows down our site by a lot.

Re-rolled #21 but reverted #19.2 for now until I have time to re-visit.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Reroll #29 against 9.5.x and added some defense for OverridesSectionStorage::isSupported.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
@@ -407,4 +409,31 @@ public function setContext($name, ComponentContextInterface $context) {
+    if ($bundle) {
...
+    }

I feel like this could be its own function, e.g. getSectionId()?

Added patch fixed CCf #33

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

As a bug this will need a test case showing the issue.

Rerolled onto an MR and fixed up a few typing/code style things.

Rebased, fixed some minor issues, and added test coverage.

Seems already been reviewed and feedback has been addressed. Didn't see anything additional.

Agreed with @kimpepper's feedback on the MR. Didn't do an in-depth review of everything.

Much nicer solution, thanks for the links. I didn't see any decisions on #3047289: Standardize how we implement in-memory caches with how these memory cache services should be setup wrt. service id names or how specific/generic they should be but I've loosely tried to follow what other things are doing in core already with cache.asset_memory and system.module_admin_links_memory_cache

I've tried passing the display's cache tags into the set() call so, in theory, it should be invalidated automatically when the display is saved but that doesn't seem to be the case. Must be missing something?

EDIT: I guess this is why, ConfigEntityBase only invalidates list cache tags on save

  protected function invalidateTagsOnSave($update) {
    Cache::invalidateTags($this->getListCacheTagsToInvalidate());
  }

Whereas EntityBase invalidates both list tags and the entity's specific tag.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased MR 7340

This has gone through multiple rounds of review, all threads have been resolved. I think it's ready to be RTBC.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

@oily thank you for rebasing many of my issues, it would be great if you could also ensure tests pass and follow up any failures in the future.

@acbramley I am sure it would.

@oily I don't appreciate the snark mate. I'm just trying to give some advice.

I've seen you rebasing issues a lot over the past few days, some of which didn't need rebasing. If you're trying to help move an issue along and gain credit, doing unnecessary rebases is listed under Examples of what will usually not receive credit on https://www.drupal.org/about/core/policies/maintainers/how-credit-is-gra....

This one did have a conflict in OpenTelemetryNodePagePerformanceTest, which it seems like you fixed, but then the fix failed tests.

You were dressing up with a thin veneer of politeness impudent and unwelcome advice. Now that the veneer is removed. And you try to discredit my contributions and my professional image in front of a potentially wide audience. I have no problem with the rules.

@oily except that @acbramley's advice here is appreciated and welcomed, and correct.

@tim.plunkett Not interested in your opinion on working relations between @acbramley and myself.

RE: #54 'This one did have a conflict in OpenTelemetryNodePagePerformanceTest, which it seems like you fixed, but then the fix failed tests.'

I do not know how to fix the tests. Anything wrong with that?

I tried to resolve a merge conflict in this issue. Anything wrong with that?

If people want to build a list of black marks against my contributions please do review my Rebases over the last 24 hours. I believe they were nearly all 90+ commits behind 11.x when I merged. A few slipped through which were < 90 but of those one or two had merge conflicts which I tried to fix. The reason why I tried to rebase 90+'s is because I saw @smustgrave do it recently and assumed without consulting the rules that that would be a safe figure, since I regard him as a stickler for the rules.

In any case I imagine core maintainers are aware of the rules and they will award credit accordingly.

@oily I think you've misunderstood where I'm coming from here.

I do not know how to fix the tests. Anything wrong with that?

No there's not, I'm happy to help if you would like to understand that more.

I tried to resolve a merge conflict in this issue. Anything wrong with that?

Of course not, I was referring to the issues that did not have conflicts.

If people want to build a list of black marks against my contributions

Nobody is doing that.

I believe they were nearly all 90+ commits behind 11.x when I merged.

This is what I was trying to explain, there is no need to rebase random issues that are X commits behind HEAD. As I understand it, you are looking for issues to work on to gain credits, that's fine, but rebasing issues will not achieve a credit. I rebase issues that I am working on myself when I come back to them and they are behind, there is no reason for me to go searching for issues to click the rebase button on, that is just wasted effort.

I would really recommend you find something that you are interested in working on, either specific issues or a specific subsystem, and try to contribute in other, more valuable ways. If you give me an idea on what you are interested in then I can try to find some issues that may suit. For example you've mentioned a few times that you don't know how to write tests, we could look for some issues with the Needs tests tag and go through how to write a test for them.

#59 "..To ensure that all participants are heard and respected, we encourage a brief pause from the conversation to gain perspective.."

So why #60?

Which states, among other things, "For example you've mentioned a few times that you don't know how to write tests."

That is untrue. I have never said that. The fact is, I have created a number of tests and been credited for them. Nor have I ever said I can fix any test whatever test it be. I DID say I was unable to fix a particular test/ set of tests, the 'opentelemetry' or opentelemetry-related tests.

#60 makes other incorrect assumptions and is not welcome in any case because it followed #59.

I am following #59, which I regard as good advice. I hope the others involved do the same.

Changing to needs work after creating a review of the code. A couple of perhaps nitty suggestions on code comments. I have given alternative wordings so the 'work' required is to accept my alternative wordings of the comments, to ignore them or to vary them as seen fit. I did review all the code itself in most of the MR files but did not see anything even to be nitty about though my local env is not well set up yet and without being able to apply xdebug etc deep analysis is harder.

Regarding my comment suggestions, PHP.net speaks about 'objects' 'implementing' interfaces. We know that classes implement interfaces but on PHP.net it is cool to say objects do it too..

I don't agree with the suggestion on the added interface, the other one is for existing documentation so needs a separate issue.

Re: #64 and "I don't agree with the suggestion on the added interface.." Can you please explain why. Please reference the comments I have made. A reasoned response is much appreciated.

Left some comments on the MR
I realise both @kim.pepper and @catch agreed that using memory cache over static cache was preferred, but given we already have a memory cache for entities and the isLayoutBuilderEnabled call is fairly light weight, I'm not sure whether we're doing premature optimisation here at the cost of added complexity.

It would be good to see what impact just using the existing memory cache has

@acbramley pointed out that the choice of notSupported vs isSupported was mentioned in #6, so feel to disregard that feedback - I don't feel that strongly about it

All feedback has been addressed, thanks for the review @larowlan

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased

Attempt to simplify the title after reading through the comments.

Although the IS mentions sort of 'knock-on' effects of the main issue, might help to distinguish which of those 'knock-on's' getting 'knocked on the head' by this issue. Are there any follow-ups required or is this a 'catch-all'?

I think the previous title was a bit more succinct in what we're fixing here.

The "knock on" effects are exactly what we're fixing here, the performance benefits of simply not attempting to build any sections if layout builder is not enabled are the fix.

I've updated the IS a bit.

@acbramley Okay. I should have said 'knock-on' effects described in comments #1 - #7 involving 'contrib' modules and various hooks. Perhaps those comments are not relevant now? They date from 2019. Or are they addressed in the MR? Also, is a CR required? e.g. for creators of new contrib modules of a kind that might cause the performance hit described in the IS?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fix merge conflict in MR !7340.

Have a couple of questions on the MR.

I was also wondering that since the notSupported()/isSupported() are only called from LayoutBuilderEntiyViewDisplay::buildMultiple(), that the arguments to those methods should be notSupported(EntityViewDisplayInterface $display) and isSupported(EntityViewDisplayInterface $display) instead, since we already have the display entity loaded (as $this). This would save loading the display entity again, since entity view display entities are not statically cached.

If the expectation is that these methods might be called other places (in contrib/custom code) where the display has not been loaded, then it's fine.

This has gone through multiple rounds of reviews, we have passing tests.

I've given it one last sweep and it looks good, lets try get this in to save @acbramleys sanity from needing to reroll this one every couple of days when the performance metrics cause a merge conflict.

Had similar questions to @larowlan about the ::isNotSupported() but then also went back all the way to #6 and didn't really have better ideas. This is all pretty internal to layout builder and doubt much other code will be interacting with it.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

No credit despite at least #87. Seems I upset the drupal.org 'mafia' on this issue? : )