Problem/Motivation

#2671964: ContextHandler cannot validate constraints brought in a major degradation in performance for pages that load large number of definitions. Example: Layout Builder, Panelizer's "Manage Content" tab, etc.

To Reproduce

  1. Install Drupal Site with standard
  2. Install Layout Builder
  3. Update article to allow for layout to be used
  4. Drupal\Core\Plugin\Context\ContextHandler::filterPluginDefinitionByContexts() Add start and end timers. Ex:
        $start = microtime(true);
        $filtered = array_filter($definitions, function ($plugin_definition) use ($contexts) {
          // If this plugin doesn't need any context, it is available to use.
          if (!isset($plugin_definition['context'])) {
            return TRUE;
          }
    
          // Check the set of contexts against the requirements.
          return $this->checkRequirements($contexts, $plugin_definition['context']);
        });
        $end = microtime(true);
        $diff = $end - $start;
        return $filtered;
    
  5. In \Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validateNode() comment out the recursive bits
  6. Run a comparison of times.

OR:

  1. Revert Patch and run similar comparisons to that above.

Proposed resolution

Fix validator to recurse wisely?

CommentFileSizeAuthor
#71 Screenshot 2022-07-07 at 5.35.51 PM.png1.32 MBShishir Kumar
#55 2981889-55.patch3.42 KBacbramley
#55 2981889-55-TEST-ONLY.patch1.52 KBacbramley
#51 Screen Shot 2019-10-01 at 12.27.59 pm.png58.71 KBSam152
#49 landing page.gif1.86 MBeiriksm
#47 After applying d8_entity_validation.patch profile - Blackfire 2019-08-22(1).png85.55 KBtim.plunkett
#47 Before applying d8_entity_validation.patch profile - Blackfire 2019-08-22.png83.05 KBtim.plunkett
#44 d8_entity_validation.patch1.9 KBfago
#42 recursive-validator-new-relic-stack-trace-3.png222.79 KBjosephdpurcell
#42 recursive-validator-new-relic-stack-trace-2.png245.24 KBjosephdpurcell
#42 recursive-validator-new-relic-stack-trace-1.png231.89 KBjosephdpurcell
#40 2981889-40.patch1.91 KBbkosborne
#24 newrelic.png173.47 KBbkosborne
#20 layout_builder_performance-2981889-20.patch2.25 KBNebel54
#18 layout_builder_performance-2981889-18.patch2.25 KBNebel54
#16 layout_builder_performance-2981889-16.patch2.11 KBNebel54
#10 layout_builder_performance-2981889-10.patch881 bytesNebel54
#10 profile_without_lb.png90.41 KBNebel54
#10 profile_with_lb.png94.52 KBNebel54
#6 revert-change-from-2671964.patch1.36 KBtimodwhit
#6 timings.png35.33 KBtimodwhit
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timodwhit created an issue. See original summary.

timodwhit’s picture

This issue will also show itself on the Block placement page.

EclipseGc’s picture

@timodwhit

Hey so, yeah any time we have to validate a context, it's slow. This actually has nothing to do with the context system and everything to do with entities and their validation. Context is just dependent upon it because that's the system that was implemented and has been in core since the days of 8.0.0. I'd LOVE to see a validation method that's less heavy handed... simple instanceof checks would suffice for 99% of the context system's use case, however, the abstractions that exist prevent us from easily introducing that, and beyond that there are use cases outside that situation for which the existing validation approach future proofs us a bit.

These regressions should mostly exist administratively, and in fact your own report seems to indicate that is the case. Are you seeing regressions after page caching has happened for the rendered result of these things? I've not experienced any administrative slowness that wasn't directly related to some misconfiguration of my own local environments.

Eclipse

timodwhit’s picture

@Eclipse

Thanks for the response. Totally get that this is the best case scenario to future proof us.

As for page caching, I'm not sure if that would help much in the sense that each page that is loaded would get a new cache. Example: node/123 would get 1 cache insert for the layout builder and node/345 would get another one.

With the validation times we have been seeing with standard, this is a fairly significant load increase for TTFB alone because of the heavy handed approach. This then beckons the question of is the 99% use case enough and the rest should be a custom implementation?

Just for an example of how dramatic the load differences were: Panelizer for us went from 1-2 sec loads to over 30-45sec loads.

EclipseGc’s picture

That is very strange. No one else is complaining of that sort of speed difference to the best of my knowledge. Anything else in your scenario that could be contributing?

timodwhit’s picture

Yeah, wanted to run some tests on simplytest.me and get some TTFB stats. The times are derived from standard with panelizer and dependencies installed. The times were grabbed from the article content type with layout builder when clicking the "Add Block" button. The node types added were simple nodes with a body field named Type 1, Type 2, etc.

timings

If you notice that (as expected) the more node types you add the greater the time. Unfortunately, on simplytestme the use of 8.4.8 is breaking and not installing properly, so I could not get the times from 8.4.8. If someone could test reverting ContextHandler to the 8.4.8 state and getting some numbers it could be valuable to see the time difference.

Locally, that massive of a slow down is some inclination of another issue, however the massive variance in speed between the two is intriguing.

Couple reasons we might not hear about this:
1: Layout Builder is experimental and not in the wild too much
2: Panelizer/Block Layout aren't known for their speed, also amongst sites that are set up. They might not be in the common editorial workflow.
3: Maybe the number of node types, blocks, etc, is low enough that the plugins loaded aren't recursing over a lot of data.

timodwhit’s picture

Didn't mean to attach that...

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Nebel54’s picture

We are experiencing major performance problems after enabling the layout builder module - even without activating it for content types. I am not entirely sure why it slows down our site so much, because I was not able to reproduce the problem with a vanilla install and generated content.

Our page has currently:
Nodes: 31.323
Content-Types: 26
Active languages: 37

For benchmarking I did a curl call to an overview page with multiple paragraphs & entity reference fields. The paragraphs also contain entity references (and some kind of inheritance). In total are 16 paragraphs shown on this page. Drupal Version is 8.7.1, render caches are disabled (except for the twig cache), XDebug enabled.

Command: time curl https://page.docksal/langing-page > /dev/null

1st run (layout builder is disabled):
real 0m4.694s
2nd run (after drush en layout_builder):
real 1m16.202s
3rd run (no changes, but with rebuilt registry):
real 0m52.327s

After doing a bit of profiling, I noticed that RecursiveContextValidator->validateNode & RecursiveContextValidator->validateConstraints are executed 118,836 times, which seems to be the showstopper.

I think the underlying problem is that LayoutBuilderEntityViewDisplay always tries to load possible layout_builder sections for each entity, even if the layout builder is not enabled for a specific entity-type and view-mode. The attached patch solved the problem for us through not loading the layout_builder contexts if the layout builder is not active. But I think there are more issues with the validator because I could not figure out why it called itself so often for 30-50 related entities.

Nebel54’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: layout_builder_performance-2981889-10.patch, failed testing. View results

Nebel54’s picture

I just took a look at the failed test: The test checks if the section storage for a specific node still works AFTER the layout builder has been disabled for a node-type. It is not possible within the UI to disable layout builder as long as node-specific layouts exist. My plan is to adopt (remove) this test case if there are no objections.

grantkruger’s picture

@Nebel54 I'm in the same boat with fewer content types, less than half the content and no translations. I'm working locally on Lando (docker wrapper) on a Mac and that has its own issues, but I can still compare before and after. All I have to do is enable the layout builder module and my site's performance immediately becomes unworkable and the app container gets overloaded and does not recover for a while. I'm not doing performance tests but rather I'm simply running a backstopjs visual regression test suite that tests 50 pages (25 regular and 25 mobile). Here's my flow.

  1. Set my local bd to a copy of live.
  2. Run my test suite 3 times. All run fine without errors in 10 to 15 seconds.
  3. Enable the layout builder module and its dependency Layout builder module
  4. Run my test suite once. 10 tests go through (5 x reg and mobile) and then every subsequent test fails with a ENGINE ERROR: Navigation Timeout error.
  5. My sysadmin looks and docker is going nuts but it is because the application container is overwhelmed and takes an age to recover. Everything else is as he'd expect.

The failure point is similar from my local site if I navigate it as normal, e.g. working with views or creating content, etc. Response times quickly slow and then I start getting either very very slow loading pages, failing ajax calls or 503 errors from Varnish.

I have a couple of things to do first but then I'm going to try two things.

  1. Core 8.7.2 came out today and it includes a critical layout builder bug fix. In going to update to that and see if it helps.
  2. If that fails I'll try your patch. Thanks for sharing.

I'll report back when I get the chance, but in the meantime I wanted to reply to say thanks and that you might want to try 8.7.2 too.

EDIT: 8.7.2 does not fix the problem.

grantkruger’s picture

Patch at #10 works for me on 8.7.2. Per my comment at #14 after applying the patch my response time is little different to what it was before, maybe just a bit slower. Thank you Nebel54.

Nebel54’s picture

Status: Needs work » Needs review
FileSize
2.11 KB

@grantkruger: Thank you for your feedback!

I have updated the patch and removed the failing test. I did this not out of convenience but because I think that it does not make sense that layout sections are still rendered after the layout builder has been disabled globally in the content-type configuration. I would assume, that the rendering falls back to normal behaviour in that case.

Status: Needs review » Needs work

The last submitted patch, 16: layout_builder_performance-2981889-16.patch, failed testing. View results

Nebel54’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

Fixing the broken test…

Status: Needs review » Needs work

The last submitted patch, 18: layout_builder_performance-2981889-18.patch, failed testing. View results

Nebel54’s picture

Fixing the broken test again… this time it succeeds locally

Nebel54’s picture

Status: Needs work » Needs review
grantkruger’s picture

In my case I can see a huge performance hit merely by enabling the modules and your patch stopped that issue. I'm not sure why it's affecting some of us and not others. In our case we have a lot of complex layouts full of blocks and paragraphs and I wonder if that is a part of it. No time to dig right now, just glad this gets Layout Builder to work for me because I'm really enjoying the tool so far, even though it is a bit buggy.

Nebel54’s picture

bkosborne’s picture

FileSize
173.47 KB

This hit me today as well. I upgraded a site from Drupal 8.6.x to 8.7.x, and immediately noticed a performance issue in New Relic, specifically on views page that listed hundreds of results. The first page load is cached, but there are exposed filters that filter the results via AJAX. Those requests took 30-40 seconds to load.

Here's the trace summary in New Relic:

New relic trace summary

For me, this patch works great, but I think I'm lucky because the view is outputting results using fields instead of rendered data. The entities that are being returned DO have layout builder enabled, but not for the "_custom" view mode that Views is using for rendering the row output, so this patch works.

tim.plunkett’s picture

Component: plugin system » layout_builder.module
Issue tags: +Blocks-Layouts

See also #3023220: Performance: Prevent extra Layout Builder code from running when rendering fields in isolation (Views results, FieldBlock, etc).

The test change here is incorrect and will break contrib. I haven't read every patch and comment to see when it was decided to undo the test coverage, but we can't do that...

tim.plunkett’s picture

In fact I think this is a duplicate of that issue?

bkosborne’s picture

Component: layout_builder.module » plugin system

Sort of, but the fix in 3023220 checks if the view mode is custom and backs out, whereas this patch backs out if layout builder is enabled for the display. What about legit (non-custom) view modes that don't have layout builder enabled? Patch is 3023220 won't handle that.

I understand that that calling isLayoutBuilderEnabled() breaks contrib though and cannot be used.

bkosborne’s picture

Component: plugin system » layout_builder.module
Nebel54’s picture

I am aware of the altering-the-test-problematic, but I am unable to think of a reason why the old test expected a layout-builder-layout when the layout builder has been disabled on the content-type level. I would suspect that a feature is disabled after it was disabled…

tim.plunkett’s picture

Because LB defaults (the one controlled via Manage Display) is only one implementation of SectionStorageInterface.

See TestStateBasedSectionStorage for a test-only implementation that is controlled independently of that checkbox.
This is to simulate contrib solutions like Layout Library that are similarly not controlled by that checkbox.

#27 is correct.
This issue should be repurposed to focus on allowing entity types to opt-out of these LB checks for their full entity rendering, as 3023220 fixes the main perf hit of rendering individual fields.

Nebel54’s picture

@tim.plunket: Thank you for the explanation!

Sam152’s picture

grantkruger’s picture

So we have issues 2981889 (this one), 3023220 (mentioned by tim.plunkett) and 3043319 (mentioned by Sam152) all for similar issues and all having different solution patches. Which do we apply? Sounds like bkosborne did both this one and 3023220.

fago’s picture

Coming to the party from #3060985: Layout Builder attempts to builds section to determine if it is disabled - thanks to tim.plunkett for the pointer.

I noticed also that layout builder triggered entity validation as part of checking whether contexts are satisfied. I think this is wrong and at least partially the reason for the performance degression: it wants to verify whether the entity matches the context definition, e.g. entity type and possible other custom cosntraints in their match, but that should involve validating the entity as such imo.

tim.plunkett’s picture

I don't know how else to check constraints without calling $validator->validate($value, $constraints).
See \Drupal\Core\Plugin\Context\ContextDefinition::isSatisfiedBy()

Any pointers here would be appreciated

grantkruger’s picture

This patch worked fine until I tried updating to 8.7.3. It still works, but Now my site is slower again...occasionally, most noticeably sometimes after clearing caches. As before, run my test suite, see issues. Then keep running them and it all clears up. It's better than before but mildly terrifying to have new issues introduced with new versions of core that are reportedly fixing Layout Builder bugs. Is anyone working on these broader issues? I can keep adding patches from other issues, but this seems to be affecting many folks and appears to be a critical issue. My D8 chops and tools are not up to snuff for helping with an issue like this one, but I sure can test things.

tim.plunkett’s picture

There was nothing added between 8.7.0 and 8.7.3 that would degrade LB performance.
And 3023220 was committed in between 8.7.2 and 8.7.3 and greatly *improved* performance...

So without any concrete info on what 'slower again' means, idk what is actionable

grantkruger’s picture

Fair enough. I'll try and tie it down. Thanks.

bkosborne’s picture

Ok, so I think there are still two issues we're dealing with here:

1) LayoutBuilderEntityViewDisplay::buildMultiple attempts to build layout builder section render data even if the entity view display is NOT using layout builder. But based on comment in #30, I guess we can't actually perform this check due to the abstraction of the section storage layer?

2) Even if the first issue were to be resolved, there is still a big performance penalty for rendering entities that have layout builder enabled on the display that's being rendered. This is most problematic when rendering multiple entities at once, like in views results. There is a patch in #3043319: Real world rendering of a layout builder page is much slower with calls to ::filterPluginDefinitionsByContexts in SectionStorageManager::findByContext that passes tests but not sure it's correct?

bkosborne’s picture

Here's @Sam152's patch from the issue he created. If this gets committed, please be sure to credit him. This patch addresses the 2nd point I made above in #39.

As he said before, this patch no longer filters the section storage plugins by context. However, it still runs "isApplicable" on each one of them to determine which ones should be allowed.

Is it necessary to filter the plugin definitions at all since we have that isApplicable check?

Status: Needs review » Needs work

The last submitted patch, 40: 2981889-40.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

josephdpurcell’s picture

Regarding whether anyone is seeing a performance issue with Layout Builder, I can confirm we are seeing consistent 10 second TTFB on some requests that appear to be related to `\Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validateNode()` as described in the ticket. I'm attaching some stack traces from New Relic that might be helpful for comparison if anyone (a) sees a similar trace, and (b) one of the patches in this thread solves the issue.

I have not tested a patch and I have not done a comparison with Layout Builder disabled vs enabled to ensure what I'm seeing in these traces is in fact related to Layout Builder.

tim.plunkett’s picture

That is indeed the problem caused by the TypedData system, as exposed by Layout Builder.

But, the FieldPluginBase part of that screenshot makes me wonder if you're running 8.7.3 or something older than that, as this should have been improved by #3023220: Performance: Prevent extra Layout Builder code from running when rendering fields in isolation (Views results, FieldBlock, etc)

fago’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

I also ran into this severe problem - pages loaded multiple seconds due to this.
While debugging I figured problem is that entity validation is kicked off while the code only validates a few constraints against a certain entity. This is due to the typed data validator recursing by default, what makes sense for field items etc, but not when validating against an entity. Thus, attached patch disabling the recursing when some constraints are validated against an entity. IT's only disabled when custom constraints are passed in, so it should be safe (even though I cannot think of a case where entity objects should be validated in a nested way).

Attached patch solves the issue.

Update: sry, patch is against 8.7. - but should apply against 8.8. as well

tim.plunkett’s picture

Component: layout_builder.module » typed data system
Issue tags: -performance issue +Performance, +needs profiling

That looks promising!
Can you provide any profiling data for the before/after?

gnikolovski’s picture

We also ran into this problem.

After applying the patch provided in #44 site works much faster.

Here's the profiling data before and after (profiles will expire in a day):

https://blackfire.io/profiles/2b2da7d8-e088-46f8-a9bf-46c40b19cc67/graph

https://blackfire.io/profiles/d95c4951-69de-4b1c-a6c7-2e814db298fb/graph

tim.plunkett’s picture

tim.plunkett’s picture

Hmm, I think there is an issue with the "after" profiling. RecursiveContextualValidator::validateNode() doesn't appear in the callstack at all, which is the method that was changed here. Perhaps the after had some caching kick in.

eiriksm’s picture

FileSize
1.86 MB

Just to chime in with some initial testing of this patch. It's a MAJOR difference for us.

Our client has a huge landing page consisting of rendered entities, banners all kinds of things. Attached a gif of me scrolling down the layout tab for an idea of the amount (also because even if i zoomed out as much as it was possible it was not possible to get a screenshot of all the blocks).

This takes ~28s on cold caches on my machine.

With this patch applied it takes ~11s.

Here is the data for this:

eiriks-iMac:my-project eirikmorland$ drush cr && time curl http://my-project.localhost/ > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  11532      0 --:--:--  0:00:27 --:--:-- 82506

real	0m27.061s
user	0m0.006s
sys	0m0.007s
eiriks-iMac:my-project eirikmorland$ drush cr && time curl http://my-project.localhost/ > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  10438      0 --:--:--  0:00:29 --:--:-- 67508

real	0m29.912s
user	0m0.006s
sys	0m0.010s
eiriks-iMac:my-project eirikmorland$ drush cr && time curl http://my-project.localhost/ > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  11427      0 --:--:--  0:00:27 --:--:-- 77485

real	0m27.313s
user	0m0.007s
sys	0m0.009s
eiriks-iMac:my-project eirikmorland$ cd drupal/
eiriks-iMac:drupal eirikmorland$ curl https://www.drupal.org/files/issues/2019-08-21/d8_entity_validation.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1946  100  1946    0     0   5474      0 --:--:-- --:--:-- --:--:--  5481
patching file core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
eiriks-iMac:drupal eirikmorland$ cd ..
eiriks-iMac:my-project eirikmorland$ drush cr && time curl http://my-project.localhost/ > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  27561      0 --:--:--  0:00:11 --:--:-- 76068

real	0m11.335s
user	0m0.006s
sys	0m0.007s
eiriks-iMac:my-project eirikmorland$ drush cr && time curl http://my-project.localhost/ > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  28768      0 --:--:--  0:00:10 --:--:-- 67464

real	0m10.862s
user	0m0.006s
sys	0m0.009s
eiriks-iMac:my-project eirikmorland$ drush cr && time curl http://my-project.localhost/ > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  29620      0 --:--:--  0:00:10 --:--:-- 72346

real	0m10.546s
user	0m0.006s
sys	0m0.007s

What can I do to help this issue moving forward? Do we need more profiling? I can create some xhprof files if that helps?

eiriksm’s picture

...also tested with a luke warm (patent pending) cache.

First request /user/login to get some initial cache filled. Then request the landing page.

Without the patch:
~26s

With the patch:
~8s

Here are the methodology and data for those numbers:

eiriks-iMac:my-project eirikmorland$ drush cr && curl http://my-project.localhost/user/login > /dev/null && time curl http://my-project.localhost > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 65842    0 65842    0     0  22807      0 --:--:--  0:00:02 --:--:-- 22806
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  38494      0 --:--:--  0:00:08 --:--:-- 80153

real	0m8.115s
user	0m0.006s
sys	0m0.005s
eiriks-iMac:my-project eirikmorland$ drush cr && curl http://my-project.localhost/user/login > /dev/null && time curl http://my-project.localhost > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 65842    0 65842    0     0  19381      0 --:--:--  0:00:03 --:--:-- 19382
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  37816      0 --:--:--  0:00:08 --:--:-- 77331

real	0m8.261s
user	0m0.006s
sys	0m0.006s

# ...and removing the patch produces this:

eiriks-iMac:my-project eirikmorland$ drush cr && curl http://my-project.localhost/user/login > /dev/null && time curl http://my-project.localhost > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 65842    0 65842    0     0  15374      0 --:--:--  0:00:04 --:--:-- 15376
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  11597      0 --:--:--  0:00:26 --:--:-- 85678

real	0m26.909s
user	0m0.006s
sys	0m0.007s
eiriks-iMac:my-project eirikmorland$ drush cr && curl http://my-project.localhost/user/login > /dev/null && time curl http://my-project.localhost > /dev/null
 [success] Cache rebuild complete.
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 65842    0 65842    0     0  21754      0 --:--:--  0:00:03 --:--:-- 21758
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  304k    0  304k    0     0  12303      0 --:--:--  0:00:25 --:--:-- 76217

real	0m25.366s
user	0m0.006s
sys	0m0.007s
Sam152’s picture

Just some more anecdotal evidence to add to the pile, here is a graph from new relic of a site in production running LB. Can you guess when the patch was applied? :)

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Thanks!

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

As much as I want to commit this - its a bugfix, so as such should come with some test coverage - can we add a test here to prevent a future regression?

acbramley’s picture

Assigned: Unassigned » acbramley
Status: Needs review » Needs work

Taking a look at writing a test

acbramley’s picture

Naming, comments, and the rest are probably off here but this is a super simple test to demonstrate the fix.

The last submitted patch, 55: 2981889-55-TEST-ONLY.patch, failed testing. View results

shaal’s picture

@larowlan pointed me to this issue.
When comparing #3086583: Installing layout_builder module after Umami content import completed and this issue, results are very similar, so it works great for reducing Umami installation time.

Morbus Iff’s picture

@eiriksm pointed me here after I reported a 3 to 4s AJAX response within Commerce and its dynamic product variation field support within a Layout Builder layout. After applying the patch in #55, those same responses are now 1 to 1.2s. I don't know the innards enough to provide a code review, but from a UX perspective, this is a win for me.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

The patch is still good, and the test does what it needs to do. Thanks!

The last submitted patch, 55: 2981889-55-TEST-ONLY.patch, failed testing. View results

josephdpurcell’s picture

Yay RTBC!

I want to add more anecdotal evidence that the patch from #44 works exceptionally well. On Acquia, the longest profiled requests on PROD went from 38 sec before the patch to just 5 sec after. On DEV env, comparing uncached response time for a given URL before and after the patch showed an elimination of 15 million function calls and a 75% improvement in performance!

Thank you EVERYONE who helped make this patch happen!!!

larowlan’s picture

larowlan’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 57aabd2 and pushed to 9.0.x. Thanks!

c/p to 8.8, 8.9

Flagging for backport to 8.7

larowlan’s picture

Queued a test on 8.7.x of previous patch

  • larowlan committed 57aabd2 on 9.0.x
    Issue #2981889 by Nebel54, acbramley, timodwhit, bkosborne, fago,...

  • larowlan committed 58e17e0 on 8.8.x
    Issue #2981889 by Nebel54, acbramley, timodwhit, bkosborne, fago,...
  • larowlan committed 6e78c4d on 8.9.x
    Issue #2981889 by Nebel54, acbramley, timodwhit, bkosborne, fago,...
larowlan’s picture

Status: Patch (to be ported) » Fixed

passed on 8.7 too

backported to 8.7.x

  • larowlan committed e9181e7 on 8.7.x
    Issue #2981889 by Nebel54, acbramley, timodwhit, bkosborne, fago,...
xjm’s picture

Priority: Major » Critical

Belatedly promoting to critical; pretty sure it meets the criteria for a performance critical.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Shishir Kumar’s picture

Version: 8.7.x-dev » 9.1.x-dev
FileSize
1.32 MB

@tim.plunkett Layout builder is taking 50+sec to load. Please refer to attached xhprof test screenshots

Shishir Kumar’s picture

andypost’s picture

Version: 9.1.x-dev » 8.7.x-dev

Please file new issue instead of changing fixed ones