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
Comment | File | Size | Author |
---|---|---|---|
#12 | 2992017-entitycontext-12.patch | 4.08 KB | tim.plunkett |
#12 | 2992017-entitycontext-12-interdiff.txt | 899 bytes | tim.plunkett |
#5 | 2992017-entitycontext-4.patch | 3.83 KB | tim.plunkett |
#4 | 2992017-entitycontext-4-interdiff.txt | 2.52 KB | tim.plunkett |
#3 | Selection_188.jpg | 22.93 KB | neclimdul |
Comments
Comment #2
tim.plunkettComment #3
neclimdulI 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!
Comment #4
tim.plunkettThis adjusts the unit tests for the new method calls.
Comment #5
tim.plunkett/facepalm
Comment #7
neclimdulWe could probably improve the test even more by providing a test specifically for this case with
Comment #8
johnwebdev CreditAttribution: johnwebdev commentedYeah, 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.
Comment #9
neclimdulSince 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. :)
Comment #10
tim.plunkettSee also #2994550: Filtering block plugins by context is slow
Comment #11
tedbowI 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
Comment #12
tim.plunkettAgreed it's worth adding. Thanks!
Comment #13
tedbow@tim.plunkett thanks for the update. This is RTBC as long tests don't push it back. EntityContextDefinitionIsSatisfiedTest passes locally
Comment #14
neclimdulAgree with everything said, you guys rock! RTBC+1
Comment #16
webchickWow, 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!