Problem/Motivation

When evaluating plugin contexts, the context definition provides sample values for its context to use during comparison.

For entity contexts, this means creating an entity for each valid bundle.
Currently this uses \Drupal\Core\Entity\ContentEntityStorageInterface::createWithSampleValues(), which creates an entity and fills in sample values for every field.
However, those fields are not used during comparison, and are relatively expensive to create.

Proposed resolution

Switch from using a sample entity to a regular (empty) entity.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.31 KB
neclimdul’s picture

FileSize
22.93 KB

I found this on a site that was sometimes taking over a minute to show the block placement form and getting timeout errors. While its still not lighting fast, this fixes it in a big way!

Blackfile comparison with/without patch.

tim.plunkett’s picture

This adjusts the unit tests for the new method calls.

tim.plunkett’s picture

The last submitted patch, 2: 2992017-entitycontext-2.patch, failed testing. View results

neclimdul’s picture

We could probably improve the test even more by providing a test specifically for this case with

$content_entity_storage->createWithSampleValues($bundle)
  ->shouldNotBeCalled();
johnwebdev’s picture

Yeah, having the same issue with performance on this, and this patch gives me the same performance boost (-67%). From 9 seconds to 3. Still bad. But MUCH better. +1 on this.

neclimdul’s picture

Since this issue makes layout builder unusable or nearly unusable with real world block lists and content types I'm going to be bold and make this a blocker. Its definitely a significant performance issue. :)

tim.plunkett’s picture

tedbow’s picture

I think this looks good. I agree #7 we should add the extra assertion. but also put a comment as to why it is not called. So if someone changes it in the future they don't remove the assertion from the test with thinking about the performance implications.

Besides looks good for RTBC

tim.plunkett’s picture

Agreed it's worth adding. Thanks!

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@tim.plunkett thanks for the update. This is RTBC as long tests don't push it back. EntityContextDefinitionIsSatisfiedTest passes locally

neclimdul’s picture

Agree with everything said, you guys rock! RTBC+1

  • webchick committed 033f2a2 on 8.7.x
    Issue #2992017 by tim.plunkett, neclimdul, tedbow, johndevman: \Drupal\...
webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +MWDS2018

Wow, based on #3 / #8, this seems like a significant performance win for a very small number of lines of code (+test coverae), and @tim.plunkett pointed out this performance issue was introduced in 8.6, so it makes sense to fix it there as well.

Committed and pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

  • webchick committed 3813900 on 8.6.x
    Issue #2992017 by tim.plunkett, neclimdul, tedbow, johndevman: \Drupal\...

Status: Fixed » Closed (fixed)

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