Updated: Comment #35
Problem/Motivation
Right now EntityRenderController::viewMultiple() calls entity_get_render_display() once per bundle, resulting in separate entity loads.
We should optimize this, collect all involved bundles and then load the corresponding displays in a single load.
Proposed resolution
- entity_get_render_display($entity, $view_mode) --> static EntityDisplay::collectRenderDisplays($entities, $view_mode)
(a "single-mode" collectRenderDisplay($entity, $view_mode) shortcut is provided)
- entity_get_render_form_display($entity, $view_mode) --> static EntityFormDisplay::collectRenderDisplay($entity, $view_mode)
(no need for multiple rendering on the form side, but the code is moved similarly for consistency, and also optimized a bit on its own)
Remaining tasks
Reviews :-)
User interface changes
None
API changes
See above.
Note that entity_get_render_display() / entity_get_render_form_display() are only used by code that actually performs entity rendering / entity form building, so very little code should actually be impacted.
Comment | File | Size | Author |
---|---|---|---|
#74 | 2090509-follow-up.patch | 1.62 KB | yched |
#72 | 2090509-follow-up.patch | 2.55 KB | yched |
#70 | 2090509-follow-up.patch | 2.72 KB | yched |
#64 | 2090509-follow-up.patch | 2.72 KB | yched |
#63 | 2090509-follow-up.patch | 2.35 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedInitial patch.
I guess we should do something similar for entity_get_render_form_display() (move it to EntityFormController, but probably no need for making it "multiple"), but I might have no time before I leave on vacation. Posting what I have.
Comment #3
yched CreditAttribution: yched commentedFix BlockRenderController, which didn't extend the base EntityRenderController class so far.
Checked with @tim.plunkett, this shouldn't be needed anymore.
Comment #5
swentel CreditAttribution: swentel commentedShould fix some tests
Comment #7
yched CreditAttribution: yched commentedReroll
Comment #9
yched CreditAttribution: yched commentedA couple fixes
Comment #10
amateescu CreditAttribution: amateescu commentedThe patch looks very good, should we do the same for form displays?
Edit: even if not for performance, as we don't usually display more than one entity form on a page, at least for consistency and getting rid of
entity_get_render_form_display()
.Comment #11
yched CreditAttribution: yched commented@amateescu: Yes, the patch needs to do the same on the forms side too. There is no "multi-entities" processing needed for forms, but we should definitely maintain consistency.
Comment #12
yched CreditAttribution: yched commentedReroll, + switch functional calls to service code.
Still @todo : form_display
Comment #13
yched CreditAttribution: yched commentedSimilar for form_display
Comment #16
yched CreditAttribution: yched commentedLast night was the night of silly mistakes...
Comment #18
yched CreditAttribution: yched commentedSilly is not over it seems.
Comment #20
yched CreditAttribution: yched commentedHm. Remaining fails, of course, are because patch replaces entity_get_render_form_display() by EntityFormController::collectRenderDisplay() without converting the remaining calls in edit.module...
This in fact shows that, while EntittyViewBuilder is actually a perfect place to move entity_get_render_display() on the view side, the same is not true for EntityFormController / entity_get_render_form_display():
- EntittyViewBuilder is just a "helper object holding logic for viewing entities", and any code can fetch it through the EntityManager, and call one of its methods. (and #2134887: move field_view_field() / field_view_value() to methods is moving the last remaining external calls to entity_get_render_display()/EVB::collectRenderDisplay() to EVB itself, so this makes even more sense)
- But EntityFormController is not a "helper object", it's the actual entity form, and it makes no sense to create that object if it's not to build that actual form. So edit.module, that currently calls entity_get_render_form_display() to build a one-off form for one single field, cannot call collectRenderDisplay() if it's placed on EFC...
Not sure where those methods would best go then :-/
Maybe the EntityManager ? but then again they only make sense for ContentEntities...
Or maybe static methods on EntityDisplay / EntityFormDisplay, which would also be a good fit for the other functions (entity_get_display() / entity_get_form_display()) - but then again, I liked the idea of moving entity_get_display() and entity_get_render_display() apart from each other, because although they look similar, they are really about different use cases.
Well, unless someone has a better proposal, I'll probably scope back the patch to just the optimization part, keeping the code as sad old functions in entity.inc for now, and leave the question of how we can OO them to a separate discussion...
Comment #21
yched CreditAttribution: yched commentedAlso, raising to major, because the current code is pretty bad perf-wise.
(aside from not allowing multiple-load of displays across the multiple entities being views, it also currently does one entity_load too many even for a single view)
Comment #22
fagoI think here lies the problem - if edit implements an entity form it should do so via the regular API for entity forms? I'd not mind an entity form having just a single field.
Comment #23
yched CreditAttribution: yched commentedIn short - can edit.module's "single field" form extend EntityFormController somehow ?
I pinged @wim for feedback.
Comment #24
Wim LeersReroll now that #2031725: Move all entity display interfaces to the core component has landed. Several of the changes being made here were already introduced there. The only change I made is also getting rid of
entity_get_render_form_display()
's docs, which wasn't being done yet in the latest patch.Comment #25
Wim LeersNow, the Edit module questions.
So: the form ID must be different.
EditFieldForm
.EditFieldForm::init()
callsentity_get_render_form_display()
is because of an unimplemented@todo
: the idea is that you could configure Edit module to automatically use those field widgets that are configured in a specific form view mode — but that's currently not used at all!Comment #27
yched CreditAttribution: yched commentedRight. Thanks for the reminder, @Wim.
Well, on the 'view' side, modules that add stuff in rendered entities using hook_entity_view() need to check the view mode or the EntityDisplay to see if they really should. I guess the same could apply on the 'form' side. "If form_mode is 'in_place_edit', don't pollute it".
But agreed that that would be probably easy to overlook, and thus would make "edit in place" very brittle.
[edit: moved the rest in the next comment for clarity]
Comment #28
yched CreditAttribution: yched commentedActually, I think we have a choice whether:
a) we try to push for treating EditFieldForm as a subcase of EntityFormController (aka an official entity form), and struggle with the quirks like the one above.
b) we treat EditFieldForm as "one of those forms that are not 'official entity forms'", but need to include widgets for all / some fields" - i.e. those that currently use field_attach_form_*() directly, and deal with processing the submits themselves. And we know this is something we need to keep supporting for "inline entity forms".
So my feeling is we might as well go with b), since it provides a core case for the "inline entity forms"-like case.
Meaning, yes, we need to find the right replacements for field_attach_form(), in a place that's not only accessible if you are an EntityFormController. That might mean an EntityFormBuilder - a helper "handler" class like EntityViewBuilder, that is *not* a form, but that the actual forms (EntityFormControiller, EditFieldForm, InlineEntityForm...) would use.
Comment #29
yched CreditAttribution: yched commentedAdded more thoughts in #1728816-12: Ensure multiple entity forms can be embedded into one form
Comment #30
Wim Leers#28: that sounds sensible :)
Comment #31
yched CreditAttribution: yched commentedReboot after the discussions in
#2167267: Remove deprecated field_attach_*_view()
#2095195: Remove deprecated field_attach_form_*()
entity_get_render_display() -> static method on EntityDisplay.
We'll see for forms later, just posting for the testbot for now.
Comment #32
yched CreditAttribution: yched commentedSilly refactor typo, good thing I forgot to set to "needs review" :-p
Comment #34
yched CreditAttribution: yched commentedNot done with the typos...
Comment #35
yched CreditAttribution: yched commented- Optimize the case of "no display exists in config at all" :
do not call entity_get_display(), which tries to do an additional and useless config load, but create the runtime object directly.
- Streamlined code a bit.
- Applied similar treatment on the form side:
entity_get_render_form_display() -> EntityFormDisplay::collectRenderDisplay()
I'll be leaving entity_get_display() alone in the issue.
Can be discussed separately, and including that here would make the patch size explode.
Comment #36
yched CreditAttribution: yched commentedUpdated issue summary.
Comment #37
andypostDisplay could not exists...
when no IDs found this will load all displays
Comment #38
yched CreditAttribution: yched commented@andypost: Nope, it's only loadMultiple(NULL) that loads all entities, loadMultiple(array()) is just a no-op (furtunately - that would be a dangerous WTF)
Comment #39
yched CreditAttribution: yched commentedAlso, the EFQ step guarantees that all $load_ids correspond to existing config entries, so no need to check for existence after loading.
Comment #40
yched CreditAttribution: yched commentedFilled in missing phpdocs.
I think this should be ready.
Comment #41
amateescu CreditAttribution: amateescu commentedThe patch looks great to me. There's only minor nitpick that can totally be fixed at commit-time :)
Over 80 chars.
Comment #42
amateescu CreditAttribution: amateescu commentedActually, one other thing: shouldn't we add (at least) collectRenderDisplay() to the interface?
Comment #44
yched CreditAttribution: yched commentedInterface : not sure, I would think not The point is that you call collectRenderDisplay() on the EntityDisplay class, specifically, not on a different one. It's not part of the interface.
Comment #45
yched CreditAttribution: yched commentedFix the wrapping error mentioned in #41.
Comment #47
yched CreditAttribution: yched commentedNo way, bot, interdiff is a comment change...
45: get_render_display_multiple-2090509-45.patch queued for re-testing.
Comment #48
yched CreditAttribution: yched commentedBack to RTBC
Comment #49
yched CreditAttribution: yched commentedReroll after #2165835: Rename EntityDisplay to EntityViewDisplay in accordance with its interface
Comment #51
yched CreditAttribution: yched commentedComment #52
yched CreditAttribution: yched commentedEr - oops.
Comment #53
yched CreditAttribution: yched commentedForgot a couple 'entity_display' / 'entity_view_display' renames.
Comment #54
yched CreditAttribution: yched commentedGreen, back to RTBC
Comment #55
catchThis looks like it's going to be more expensive than the entity_load_multiple() it replaces. Is this going to run whenever a form is rendered?
Same question here.
So I'm not sure without profiling data whether this results in a performance improvement or not - we're replacing individual loads with the EntityQuery on config entities.
Comment #56
yched CreditAttribution: yched commentedNo, that was the point of #2163919: Optimise Config EntityQueries in case there are conditions on the 'id'.
For N bundles, we need to look at Nx2 candidate display entities, and select N entities depending on the 'status' property on the candidates.
Doing so by :
- reading Nx2 config records and checking their 'status' (this is what the EFQ does after #2163919: Optimise Config EntityQueries in case there are conditions on the 'id')
- entity_load_multiple(the N selected entities), for which the config records have already been loaded / statically cached by the EFQ
is going to be more performant than:
- entity_load_multiple(Nx2 candidate entities)
- ditch the ones we don't need
Comment #57
catchOh nice! I missed that issue entirely...
Comment #58
catchCommitted/pushed to 8.x, thanks!
Needs a change notice.
Comment #59
andypostneeds follow-up to fix type-hint
Comment #60
andypostThe patch
Comment #61
amateescu CreditAttribution: amateescu commentedShouldn't we tipe-hint with ContentEntityInterface?
Comment #62
andypostContentEntityInterface
makes sense, Berdir pointer about that somewhere but I'm not sureComment #63
yched CreditAttribution: yched commentedOops, sorry for the typehints - I can't even pretend that's because they were absent from the old functions, they were not :-/
re: ContentEntityInterafce - yes, @Berdir asked for that in #2167267-78: Remove deprecated field_attach_*_view(), and he's right.
Comment #64
yched CreditAttribution: yched commentedSigh. Forgot one. Tired :-/
Comment #65
yched CreditAttribution: yched commentedCreated https://drupal.org/node/2193219
Updated
https://drupal.org/node/2130757
https://drupal.org/node/2104695
Comment #69
BerdirI guess those test fails are related to #2095195-46: Remove deprecated field_attach_form_*() / #2181511: Throw an exception when creating an EntityDisplay for a non-ContentEntity ? :(
Comment #70
yched CreditAttribution: yched commentedHm, yes, it's actually too early for that typehint on the Form side, EntityFormController still tries to load an EntityFormDisplay for non-Content entity types.
That is being cleaned up in #2095195: Remove deprecated field_attach_form_*(), but meanwhile best we can do is typehint on ContentEntityInterface only on the "view" side.
Comment #72
yched CreditAttribution: yched commentedYeah, I was really tired. #70 was just the same patch as #64.
Comment #73
amateescu CreditAttribution: amateescu commentedThen why don't we just update EntityViewDisplay and leave EntityFormDisplay to #2095195: Remove deprecated field_attach_form_*()? :)
Comment #74
yched CreditAttribution: yched commentedRight, updated patch in #2095195: Remove deprecated field_attach_form_*() now adds the right typehint to EntityFormDisplay, so we might as well leave it alone here.
Comment #75
amateescu CreditAttribution: amateescu commentedGreat!
Comment #76
webchickWell that was confusing. :) However, committed/pushed that docs clean-up to 8.x.