There are two ways to render an entity through the Entity API
Either do
$nodes = \Drupal::entityTypeManager()->getStorage('node')->loadMultiple([1]);
$build['nodes'] = \Drupal::entityTypeManager()->getViewBuilder('node')->viewMultiple($nodes, 'teaser');
or
$nodes = \Drupal::entityTypeManager()->getStorage('node')->loadMultiple([1]);
foreach ($nodes as $node) {
$build['nodes'][] = \Drupal::entityTypeManager()->getViewBuilder('node')->view($node, 'teaser');
}
The problem with viewMultiple() is that render cache is bypassed in a subtle way. Even though you won't see any changes, the pre_render callback will call all entity build/view/alter hooks .. so if anything is expensive going on there, well, you're doing it again even though you have a hit.
I've attached a simple module which you can use to see the problem.
1. turn on and make sure you have a node with id 1
2. turn off page and dynamic page cache, make sure no other caching mechanism is in place
3. surf to /test
4. check that the entity is render cache
5. open testing.module and uncomment the 'die' line (line 9)
6. reload page, everything is fine
7. open src/TestController and comment the viewMultiple() line and uncomment the foreach
8. reload: wsod ..
Comment | File | Size | Author |
---|---|---|---|
#58 | interdiff_53-58.txt | 1.85 KB | Prem Suthar |
#58 | 2843565-58.patch | 10.84 KB | Prem Suthar |
| |||
#54 | 2843565-53-interdiff.txt | 504 bytes | Berdir |
#54 | 2843565-53.patch | 10.72 KB | Berdir |
#51 | interdiff_50-51.txt | 2.43 KB | ranjith_kumar_k_u |
Comments
Comment #2
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedworking on test fail patch now
Comment #3
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedSo stepping through the render pipeline was already painful to uncover this bug, turns out writing a test isn't fun either ..
So very basic web test for now, which is simple and elegant in a way to prove the bug.
Comment #4
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedComment #6
BerdirI guess I know what the problem is.
viewMultiple() sets a #pre_render *outside* of the entities, but the render caches are inside. So we call that #pre_render even on cache hits.
We're basically lying out ourself here, we discussed and accepted the drawback that we lose multiple rendering by using render caching we just didn't really want to believe it :)
I guess the good thing is that views doesn't actually use viewMultiple() so views aren't affected and due to that, not much other than custom code probably is..
Here's a quickfix, passes the new test. We could consider to deprecate buildMultiple, implementing the same in build would only require 50% of the code or so wih all that optimization to group by view mode and so on. One problem is that by not using buildMultiple() anymore, we kind of make this an API change, entity view builders might override the method and expect it to be called. I know we do that in Paragraphs atm.
Comment #7
Berdir\Drupal\block_content\BlockContentViewBuilder is actually an example that we break with this change, but not sure what else we should do.
As a start, we could actually implement it like that (without a #pre_render) by default if render_cache is disabled for an entity type.
Also looking at viewMultiple() calls, aggregator calls it, comments call it, so we should be able to reproduce with comments in core, interesting that we didn't notice this earlier. But that's it, nothing else outside of tests.
Comment #8
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedQuickly tested the patch, works great.
Briefly talked with berdir in IRC, we both feel to to go for a minimal fix to get this in as quickly as possible and create a follow up to consider deprecating/dropping buildMultipel(). This is especially a great fix for comments for instance.
Comment #9
BerdirOk, updated BlockContentViewBuilder. It still worked, as it still explicitly calls buildMultiple(), just didn't properly unset the #pre_render callback anymore, so probably did it twice.
Also cleaned up the test and switched to use entity_test. Was thinking about using a kernel test, but I think it's actually good to have an actual web test for this to avoid anything messing with the behavior by doing shortcuts/workarounds.
Not too much point in an interdiff as renaming/changing shuffled around pretty much everything in the test. Also new test-only patch to ensure that it still fails.
Comment #11
jibranShould we move this as well? It seems like important bit of information.
Can we please add a @todo with follow up?
Comment #12
Berdir1. No, because this information is simply no longer relevant. It was a hack we needed because we weren't really thinking, now there is no hack anymore and single vs multiple viewing is the same.
2. Possibly, but I'm not exactly sure yet about what that todo would even be. BlockContentViewBuilder is one obvious example why dropping buildMultiple() isn't an option for Drupal 8 anyway.
Lets wait for some more reviews, failed/passed as expected I just messed up the patch order.
Comment #13
yched CreditAttribution: yched commentedYup, I remember @Berdir pointing out some time ago that, by moving the build step to #pre_render to benefit on render cache, we basically lost the benefits of multiple-entity view building. I don't remember that we identified that stray #pre_render to be problematic though, good catch :-/
Not only that, but the whole FormatterInterface::prepareView() step becomes moot, since its sole purpose was to allow front-loading of all EntityRefs across the multiple entities being viewed, rather than entity-by-entity (best we can have is multiple-loading of EntityRef fields of cardinality > 1 within each entity, for each entity)
EntityReferenceFormatterBase provides a default implementation for that, for all EntityRef (and subclassed field types : file, image, media, paragraphs) formatters to reuse, so arguably the existence of FormatterInterface::prepareView() didn't require any actual work from contrib/custom formatters to implement. I don't know if some formatter somewhere had to override that, I'm not sure there is a case (the one that comes to mind is entity_reference_revisions, but it seems they sidestepped the question)
Still, quite some convoluted multiple-handling logic between EntityViewBuilder::viewMultiple(), EntityViewDisplay::buildMultiple() and EntityReferenceFormatterBase, that exists for nothing :-)
But yeah, doesn't look like something that can be removed in 8.x...
Comment #14
yched CreditAttribution: yched commentedOn the patch itself, I'm too much out of the loop atm to offer a valid opinion - just :
So yeah, we probably have to keep the multiple-entity logic in EntityViewBuilder::buildMultiple() to avoid potential BC breaks on subclasses, and we also probably have to keep the method public.
But if it is in fact only ever called by ::build() with degenerate single-entity cases, it might be worth a comment explaining the historical reason for the "Dude, why so convoluted ?" WTF :-)
Comment #15
yched CreditAttribution: yched commentedOn the overall "entity render cache kills multiple-entity view building" issue :
We kind of accepted that tradeoff after we discovered it, on the grounds that the multiple-view optimization we lost only impacted render-cache misses. But that's assuming there is a reasonably high hit ratio on entity render cache :-)
If I'm not mistaken, that assumes :
- uncacheable / high-granularity formatters do the right job of being placeholdered
- the entities' cacheability (and notably the cacheability of 'view' AccessResults) is not too granular either.
Also, EntityViewBuilder only leverages render cache to begin with if "$this->isViewModeCacheable($view_mode) && $this->entityType->isRenderCacheable())", so this leaves out a few cases :-)
So the tradeoff is probably alright in the 80% cases, but I'm not sure we can safely say it's always good enough ?
OTOH :
- Retaining the multiple-view optimization for the cache misses, would require enriching the render cache API and code flow to allow something like : "your $element's children each have a ['#cache']['keys'], I got cache hits for N of those, here's a callback so that you can do something about the remaining ones as a whole".
That's me doing some heavy handwaving here, with no real clue on how it might actually look like and how easy/awful that might be :-), but feature-wise this doesn't seem totally unreasonable ?
- Views not using EVB::viewMultiple() anyway sure hinders any efforts in that direction :-/. I can't say I remember why Views only does single-entity view() though.
Comment #16
BerdirWe've discussed before about an ability to do a cache getMultiple() if there are multiple #cache definitions in multiple children. That would kind of go into the same direction. I'm a bit worried about introducing even more complexity into the render cache system though.
So if we do want to consider that, then I think we should at least do it step by step. Fix this, then cache get multiple for hits, then handling of multiple misses?
And then we're also back to #12.2. A @todo, but what exactly are we going to write? If we are considering to make it useful again, then we don't need a todo to remove it.
Maybe we should just write down facts about how it is now, without already trying to define a plan/idea for how we want it to be? That might be enough for this issue?
Comment #17
yched CreditAttribution: yched commentedOh totally, that sounds like the right thing to do right now. The issue at hand here should get fixed regardless of hypothetical render cache enhancements to come.
(I'd tend to think "multiple-render-cache-get for children" and "single build callback for multiple children cache misses" could be worth designing hand-in-hand even if actually done in separate steps, as they indeed seem related, at least to figure out what shape the API could look like)
+1. Just clarifying that the multiple-view code is kept around because BC even though we only single-view at the moment sounds good enough.
Comment #18
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedOk, let's see what core committers think of it.
Comment #19
alexpottThis change has tricky BC implications. Not sure if there is a BC way to go so that custom implementations of viewMultiple or view don't have to change. Regardless we probably need a CR here.
Comment #20
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedMaybe we should add a positive assert here as well, like check that we're actually seeing content of the entity ?
Comment #21
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedShy attempt at a change record at https://www.drupal.org/node/2846946 - needs a bit more verbosity obviously.
Comment #23
BerdirUpdated the documentation.
I added a @todo without an explicit issue reference, but since we do not actually know yet what we want/can/will do exactly, this seems like the best choice to me.
Comment #24
BerdirAdded more asserts and lots of verbosity to the change record.
Re #19: Yes, this is a bit a problem, but I'm not aware of a single other example in contrib or custom code doing what BlockContentViewBuilder does or anything else with #pre_render. Plus, even if you would do something like that, it is very likely that it would mostly still work, e.g. no tests were failing for BlockContent, we were just building twice.
Comment #25
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedOk, looks very good to me!
Comment #28
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!
Also opened #2852164: Multiple render caching to preserve the discussion.
Comment #29
jibranJust published the change record https://www.drupal.org/node/2846946.
Comment #30
BerdirAw. We noticed a problem with this in our project. this causes comment view builder to build each comment separately. But with nested comments, in relies on them being all built together.
Adding the same change as in BlockContentViewBuilder fixes it:
As I mentioned once here, I was wondering if we should actually do that by default if entity types are uncacheable, but the trick with comments is that they are only uncacheable if threading is enabled.
@alexpott is going to revert this, then we can fix it with test coverage. Or figure out something else for better BC.
Comment #32
alexpottAlso reverted this on 8.3.x - see 49092a8
Comment #34
marthinal CreditAttribution: marthinal at Bluespark commentedRerolled. See #30
Comment #35
yogeshmpawarComment #36
marthinal CreditAttribution: marthinal at Bluespark commentedComment #39
marthinal CreditAttribution: marthinal at Bluespark commentedRerolled
Comment #41
swentel CreditAttribution: swentel at eps & kaas for Dropsolid, MuseScore commentedReroll, plus the fix in comment (from #30, I basically copy/pasted the code from there).
Let's see what the testbot thinks.
Comment #43
swentel CreditAttribution: swentel at eps & kaas for Dropsolid, MuseScore commentedSo I get the drupal_set_message fail, but the other ones are curious, checking.
Comment #44
swentel CreditAttribution: swentel at eps & kaas for Dropsolid, MuseScore commentedfix the tests
Comment #50
AndyF CreditAttribution: AndyF at Fabb for Sport Obermeyer commentedReroll for 9.x. It removes the legacy test changes. Thanks!
Comment #51
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed CS errors
Comment #52
Wim LeersThanks! 🙏
Not yet passing tests though 😅 Bit more work needed!
Comment #54
BerdirIncompatible test module is what caused this to fail so badly.
Comment #57
nod_Patch fails lint on 10.1.x
Comment #58
Prem Suthar CreditAttribution: Prem Suthar at Srijan | A Material+ Company for Drupal India Association commentedSolved the #54 Custom CMD Failed Patch.
Also, Add the Interdiff Of the 53-58 patches.
Comment #59
Prem Suthar CreditAttribution: Prem Suthar at Srijan | A Material+ Company for Drupal India Association commentedComment #60
smustgrave CreditAttribution: smustgrave at Mobomo commentedTest coverage appears to be there (good)
Issue summary should be updated though with proposed solution, remaining tasks, any api changes, etc.