Leverage entityDisplay to provide fast rendering for fields

#1852966: Rework entity display settings around EntityDisplay config entity should hopefully get in soon.

I'm not sure where this would fit in the views rendering flow, but algorithmically this would be something like this :

After query results :

// Split results by bundle, create one display object per bundle:
foreach bundle in the result set {
  // create a $display object: 
  $displays[$bundle] = entity_create('entity_display', array(
    'targetEntityType' => $entity_type, 
    'bundle' => $bundle,
    'viewMode' => arbitrary (view name + display name ?)
  );
  // Configure display options for the fields in the view:
  foreach field {
    $displays[$bundle]->setComponent($field_name, $options);
  }
}
// Run prepare_view on all result entities :
// $entities should be keyed by entity ID, and $displays by bundle name.
field_attach_prepare_view($entity_type, $entities, $displays);

Once per row :

// Build render arrays for all fields in the entity.
$output[$entity->id()] = field_attach_view($entity_type, $entity, $displays[$entity->bundle()]);

Once per field :

// Grab the correct render array
$field_output = $output[$entity->id()][$field_name];

Also, one drawback is this works once per field. If the field is present more than once in the view (e.g first time with 1st value, 2nd time with other values), then the non-first occurrences need to be displayed with the good old field_view_field().

From a quick look at the patch:

$displays[$bundle]->setComponent($this->definition['field_name'], $this->options['settings']);

Should rather be something like :

$displays[$bundle]->setComponent($this->definition['field_name'], array(
  'label' => 'hidden',
  'type' => $this->options['type'],
  'settings' => $this->options['settings'],
));

(basically, the array you currently pass to field_view_field())

So currently it prepares the entity by removing items and then render it. Wouldn't it possible to render first and then remove the items from the render array?

I don't think I get the question actually :-).
field_attach_prepare_view() lets field types and formatters massage the field values before doing the rendering entity by entity? This is where stuff like "load the term/file corresponding to the tid/fid" happens (on multiple entities because it's more efficient for loading stuff). It usually adds rather than remove stuff.

(yched) Also, one drawback is this works once per field. If the field is present more than once in the view (e.g first time with 1st value, 2nd time with other values), then the non-first occurrences need to be displayed with the good old field_view_field().
(dawehner) So currently it prepares the entity by removing items and then render it. Wouldn't it possible to render first and then remove the items from the render array?

Ah, sorry, I think I get what you meant now. Like instead of rendering 1st value and "other values" separately, could we render all values once, and dispatch sub render array pieces accordingly ?
That wouldn't work, because the two occurrences of the field might be configured to use a different formatter, with different settings, etc.. Rendering needs to happen separately.

However, instead of resorting to field_view_field() for fields that are present more than once in the view, I guess another solution would be to open up new entity_display objects for those, and run f_a_prepare_view() / f_a_view() on each separately.
Like : open N entity_display objects, and put the Nth occurrence of each field in the Nth display. Thus, each display has each field at most once, and you get as much display objects as the field that is most repeated.
Code will probably be a bit more complex :-(, but should still be way faster than individual f_v_f() calls.

entity_display and f_a_*view() can only handle a given field once per entity. The ability to have a field rendered more than once (that goes along with the ability to limit the display to some deltas) is only offered by Views, not by the regular "Manage Display" flow.
It would be interesting to alleviate that limitation on the Field UI / Field API side, and I'm keeping an eye towards that in the EntityDisplay work, but it's still highly hypothetical :-).

"figure out first which fields could be rendered together": Maybe - dunno if it's really worth the trouble though :-)

Not sure I fully get the differences between the use cases you mention, but other than that, pretty much agreed :-)

I think it would be better to have a single code not special cases in the code.

Yup, +1.

view:display:field_0
  - field_A_0
  - field_B_0
(etc)

This describes the various entity_display objects, right ? If so, yes, that should work.
What are the "view:display:field_0, view:display:field_1, ..." strings ?

- relationships: right, the field values that get displayed in a row do not necessarily belong to the same entity, so the steps described in #1 probably have to be wrapped into:
"foreach relation on which there are fields, separately load the entities / build the displays / field_attach_[prepare]_view() them"...
Although probably less convoluted, I guess the current code has to account at least for related entity loading, since field_view_field() currently requires an $entity ?
- I'm less familiar with groupby and its implications :-/

Bumping, @msonnabaum cried at the perf of field-based Views in #2346763: Improve views field rendering performance by caching entity display objects.

Patch over there brings valid improvements that make sense for the generic case of "some code needs to do individual calls to $item->view() repeatedly on the same Field with different values", but I think the plan we had for view here still stands and brings more improvements.

Updated the issue summary with a brief version of the actual plan :-)

It is a fact that this never made it to the top of anyone's todo list so far :-), but other than that I don't think there's any reason for it being marked "postponed" ?

If this can actually improve performance, it'd be major at least. Tentatively marking it as such.

Besides performance, I think this could significantly unify/simplify the Views render pipeline.

Took me a few readings, and a chat with Wim, but I now understand whats being proposed here. Sounds like a nice refactor to me. Don't think I have enough command of Views and Entity render systems to bring it home, but I'm happy to review patches.

Yes, that's probably why this issue has been sitting here so long. It requires pretty intimate knowledge of the Views and Entity rendering pipeline, and never reached the top of the todo list any of the interested parties :-)

This sounds great to me. I'd probably leave it major even if it doesn't improve performance much, standardizing Views rendering would be really valuable by itself.

Just wanted to point out:
#2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage
Marking related... not sure they're the same but there's definitely some commonality. On that other issue, there's been some discussion about how to unify the language display settings for Field and Entity based views, and I think it relates to this discussion? Maybe anyway.

#18: great catch; seems like there is commonality indeed!

Raising to critical given the numbers we got in #2342045-92: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency.

Another possible optimization: #2450151: Don't try to render all fields (including hidden ones) for single entity display.

#21 To clarify: #2450151: Don't try to render all fields (including hidden ones) for single entity display is an additional optimization, not an alternative to this issue, right?

Yes, additional :)

Here is a first draft. It addresses only the basic scenarios: fields displayed multiple times and relationships still to do. This is roughly based on #2, but the amount of lines changed is so high that I'm not posting an interdiff.

Let's see if we can get at least some green test ;)

Looking great already :)

Initial review.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -114,6 +114,27 @@ class Field extends FieldPluginBase implements CacheablePluginInterface, MultiIt
   +   * All fields being rendered.
   

   Actually a static cache for ::getRenderableFieldNames().

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +   * Renders all fields of all rows using entity display objects.
   

   Could use an @see to \Drupal\Core\Entity\Entity\EntityViewDisplay.

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +        foreach ($field_names as $field_name) {
   +          if ($entity->hasField($field_name)) {
   +            foreach ($entity->get($field_name) as $item) {
   +              $item->_attributes = array();
   +            }
   +          }
   +        }
   

   This didn't exist before. What is this doing exactly? It's resetting $item->_attributes for every field that we're going to render. Why?

4. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +        $bundle = $entity->bundle();
   +        if (!isset($bundles[$bundle])) {
   +          $bundles[$bundle] = $bundle;
   +          $bundle_fields[$bundle] = array_intersect($field_names, array_keys($entity->getFieldDefinitions()));
   +        }
   

   This determines the fields to render per bundle, so we can reuse the EntityViewDisplay objects across rows whose associated main entity use the same bundle.

   So, in the optimal case, this allows us to build only a single EntityViewDisplay for the entire view, if all rows in the view have entities of the same bundle.

5. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +  protected function getRenderableFieldNames() {
   

   Would getViewFieldToEntityFieldMapping() be clearer? That unambiguously communicates what this is about.

6. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +   *   A view mode name.
   

   A custom view mode name.

7. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +   * @param string[] $bundles
   

   Is this parameter really necessary? AFAICT it can be derived by doing array_keys($bundle_fields?

8. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +   *   nested array is keyed by Views field plugin name.
   

   s/by/by a/

9. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,174 @@ public function renderItems($items) {
   +    $displays = array();
   

   s/array()/[]/

   :)

10. +++ b/core/modules/views/src/Plugin/views/field/Field.php
    @@ -700,6 +721,174 @@ public function renderItems($items) {
    +    // Create on entity display object per bundle, so you can render entities
    +    // in different bundles at the same time.
    

    Not really "render", but *prepare*.

11. +++ b/core/modules/views/src/Plugin/views/field/Field.php
    @@ -700,6 +721,174 @@ public function renderItems($items) {
    +      // @todo: Here you would sort of iterate over all of the fields.
    +      foreach ($bundle_fields[$bundle] as $view_field_name => $field_name) {
    

    Isn't this already doing that? :)

Thanks for the review!

3: It's required to call entity prepare view, see the follow-up above
4: Yep :)
5: Mmh, I'm not very convinced about that: actually the important thing is that it returns a list of fields to act on, the fact that they are keyed by field plugin name is only an optimization

Test failures still to be addressed.

Sad that we're kind of forced into this by the threat of a massive perf regression, but psyched to see this move forward :-)
@plach+++++++++++++

Couple remarks after a cursory look.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,170 @@ public function renderItems($items) {
   +  protected function preRenderEntityFields(array &$values) {
   

   Nitpick : it's called by preRender(), but if it does the rendering, shouldn't it be called renderEntityFields() rather than preRender...() ?
   You cannot pre-render a Field API field, you can only render it ;-)

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,170 @@ public function renderItems($items) {
   +        // FIXME One problem with using process_entity before rendering is that
   +        //   this might interfere with other instances of this field on the
   +        //   view.
   +        $entity = $this->getEntity($row);
   +        $entity = $entity ? $this->process_entity($row, $entity) : FALSE;
   

   Yeah, I guess that is the trickiest part of the task here, one arbitrary field handler is in charge of rendering all the other fields in the view, probably leads to a weird unbalance.

   Since we tend to move to "all displayed components of a ContentEntity view are fields", I wonder if it means the logic of "render all the fields in the view" could move one level up to the row handler ?
   (I'm not really familiar with the views pipeline, maybe this doesn't really make sense)

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -709,43 +894,37 @@ public function renderItems($items) {
   +        foreach (Element::children($build) as $count) {
   

   for consistency with what we generally do with field items and rendered items, can we s/$count/$delta ?

4. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -709,43 +894,37 @@ public function renderItems($items) {
   +          // FieldItemListInterface::view() adds an #access property to the
   +          // render array that determines whether or not the current user is
   +          // allowed to view the field in the context of the current entity. We
   +          // need to respect this parameter when we pull out the children of the
   +          // field array for rendering.
   +          if (isset($build['#access'])) {
   +            $items[$count]['rendered']['#access'] = $build['#access'];
   +          }
   

   Comment needs to be adjusted, we don't call FieldItemList::view() anymore but EVD::build().

   It does set an #access property on $build (i.e $output[$entity_id][$field_name]) that we need to push down to each delta, so the code doesn't look like it needs to change.

5. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -316,8 +316,7 @@ public function buildComponents(array &$build, array $entities, array $displays,
   -    // Invoke hook_entity_prepare_view().
   -    $this->moduleHandler()->invokeAll('entity_prepare_view', array($this->entityTypeId, $entities, $displays, $view_mode));
   +    $this->prepareView($entities, $displays, $view_mode);
   
   @@ -329,6 +328,14 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +  public function prepareView($entities, $displays, $view_mode) {
   +    // Invoke hook_entity_prepare_view().
   +    $this->moduleHandler()->invokeAll('entity_prepare_view', array($this->entityTypeId, $entities, $displays, $view_mode));
   +  }
   
   +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -700,6 +721,170 @@ public function renderItems($items) {
   +      $view_builder->prepareView($entities, $displays, $this->getCustomViewMode());
   

   No biggie, but it feels a bit weird that we need an EntityViewBuilder just for that ?

   More importantly : I guess "per-field views" didn't call hook_entity_prepare_view() before, and now they do. Not sure of the exact consequences ?

This should fix some failures and (partially) address #30.

@yched:

2: I cannot vouch on that too, I can only say it will involve many changes. @Daniel, what do you think?
5: I think we need entity prepare view when displaying fields using their native template, otherwise for instance RDF won't work. I didn't test this yet, though.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -709,43 +900,44 @@ public function renderItems($items) {
   -    if (!$entity) {
   -      return array();
   ...
    
   ...
   +    if ($entity && isset($output[$values->index][$field_name])) {
   +      $build = $output[$values->index][$field_name];
   

   Can we leave the early return please.

   That makes the function nicer to read.

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -709,43 +900,44 @@ public function renderItems($items) {
   +    $entity = $this->getEntity($values);
   +    $output = $this->getFieldOutput();
   +    $field_name = $this->definition['field_name'];
   ...
   +    // Make sure entity fields are actually rendered for the current row.
   +    if (!isset($output[$values->index])) {
   +      $this->renderEntityFields([$values]);
   +      $output = $this->getFieldOutput();
   ...
   -    $render_array = $entity->get($this->definition['field_name'])->view($display);
   

   I can see why its needed, but I think I would prefer a function that makes the caching of the EntityDisplay implicit rather than explicit, e.g.

   $output = $this->renderField($entity, [$values]);

   which then takes all the logic of rendering the entity and fields.

   And if nothing from this _field_ is needed here (e.g. if another class can access the field information needed to build the display), this could even be another class that is called in this function.

   $output = $this->entityRenderer->renderFields($entity,[$values]);
   return $output

   The advantage is that all the logic is encapsulated and theoretically changeable.

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -709,43 +900,44 @@ public function renderItems($items) {
   +        foreach (Element::children($build) as $delta) {
   +          $items[$delta]['rendered'] = $build[$delta];
   

   Thanks for using $delta, using $count made me look three times, before I got that an index or delta was meant :).

Overall looks really nice already. I think most logic should live in another class, which caches the information in the view's output and lazy draws the fields.

This makes it also easier to re-factor it so that the logic lives earlier in the chain later.

I agree with fabianx that it would be great to collect this logic into its own class. ... I think it would also make it easier to understand, this already gigantic file, but I disagree with putting that onto the row plugin, as @yched suggested

Sure, suggesting the row handler was just a shot in the dark, since the logic here is about "collect data and do the rendering for all the rows, then assign the right pieces row by row"

Wherever out of the field handler is fine by me :-)

Unfortunately the gap cannot be changed that way, because in the 850 ms already 94 ms of BubbleableMetadata::merge() are spent, so its "only" 40 ms more due to PATCH (2342045) ...

Still nice to see that this gives a 150 ms improvement in total already now, so its then only a 330 ms regression and no longer a 480 ms. :)

I think we could remove the sort and unique from the merging functions and only do it on cache set or when added to the final render array that is returned as thats where the debug should be shown anyway ... Not sure what that would save though ...

This should fix the last failures and address #33.1 and #34.3.

I'm fine with #33.2 but I'd like to keep it as the last change, once we are sure everything else is working correctly.

@#34.2: $field_names holds all fields used in the view, but only some of them may belong to the bundle; OTOH only some of the bundle fields may be used in the view, hence we need the intersection of the two sets.

I'm not a fond of early returns, but I guess Fabian was meaning this actually.

Wrong patch, sorry.

Tracking the parent ...

This should perform better.

This should support also relationships.

@Fabian: Daniel and I are seeing different numbers while profiling this, could you do a couple of runs too?

I'd be interested in a comparison between HEAD, HEAD + 2342045, and HEAD + 2342045 + this. Attached you can find a test db dump.

This fixes a failure we got when using the default_langcode field.

Code looks great, and the optimization makes sense. But I agree with @dawehner at #34 that there is a lot of logic in here - I think we need unit tests for all the new logic (sorry).

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -114,6 +114,35 @@ class Field extends FieldPluginBase implements CacheablePluginInterface, MultiIt
   +
   

   nit, extra line

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -698,6 +727,225 @@ public function renderItems($items) {
   +  public function preRender(&$values) {
   ...
   +  protected function renderEntityFields(array $values) {
   ...
   +  protected function getRenderableFieldNames() {
   ...
   +  public function getRenderField() {
   ...
   +  protected function getCustomViewMode($entity_type_id) {
   ...
   +  protected function getBundleDisplays($entity_type_id, $bundle_field_names) {
   ...
   +  public function getFieldOutput() {
   

   This is fairly complex code, and looks to be unit-testable. Can we add some unit-tests for it?

3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -698,6 +727,225 @@ public function renderItems($items) {
   +      // Skip this row if has already been processed.
   

   nit: if » if it

4. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -698,6 +727,225 @@ public function renderItems($items) {
   +      // We store a field name for each relationship toi make it easier
   

   nit: toi » to

5. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -698,6 +727,225 @@ public function renderItems($items) {
   +        // We use self here, so theoretically you could inherit this class for a
   +        // particular field.
   

   Looks like we're using static and not self?

6. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -698,6 +727,225 @@ public function renderItems($items) {
   +    // Create on entity display object per bundle, so you can prepare entities
   

   nit: on » one

Here is my numbers for #48 with DB from #47:

### FINAL REPORT

=== 8.0.x..8.0.x compared (5503b2bf8227d..5503b8086e8a7):

ct  : 362,577|362,577|0|0.0%
wt  : 474,454|473,945|-509|-0.1%
mu  : 26,757,672|26,757,672|0|0.0%
pmu : 27,284,952|27,284,952|0|0.0%

=== 8.0.x..perf-entity-display compared (5503b2bf8227d..5503b8442aaee):

ct  : 362,577|370,127|7,550|2.1%
wt  : 474,454|476,966|2,512|0.5%
mu  : 26,757,672|27,128,480|370,808|1.4%
pmu : 27,284,952|27,656,600|371,648|1.4%

=== 8.0.x..perf-views-base-fields compared (5503b2bf8227d..5503b8cb68f48):

ct  : 362,577|581,631|219,054|60.4%
wt  : 474,454|744,188|269,734|56.9%
mu  : 26,757,672|27,459,856|702,184|2.6%
pmu : 27,284,952|28,000,800|715,848|2.6%

=== 8.0.x..perf-views-base-fields-and-entity-display compared (5503b2bf8227d..5503b93a1f5a9):

ct  : 362,577|558,823|196,246|54.1%
wt  : 474,454|682,880|208,426|43.9%
mu  : 26,757,672|28,754,032|1,996,360|7.5%
pmu : 27,284,952|29,277,736|1,992,784|7.3%

=== SUM: 8_0_x-summary..perf-entity-display-summary compared (5503b954c1ae1..5503b95b1cb5e):

ct  : 36,261,178|37,016,274|755,096|2.1%
wt  : 48,134,912|48,470,531|335,619|0.7%
mu  : 2,676,372,072|2,713,376,488|37,004,416|1.4%
pmu : 2,729,108,328|2,766,201,824|37,093,496|1.4%

### XHPROF-LIB REPORT

+-------------------------------------------+------------+------------+------------+------------+------------+
| namespace                                 |        min |        max |       mean |     median |       95th |
+-------------------------------------------+------------+------------+------------+------------+------------+
| Calls                                     |            |            |            |            |            |
|                                           |            |            |            |            |            |
| perf-vbf-and-entity-display |    558,823 |    562,445 |    558,859 |    558,823 |    558,823 |
| perf-views-base-fields                    |    581,631 |    585,157 |    581,666 |    581,631 |    581,631 |
| perf-entity-display                       |    370,127 |    373,701 |    370,163 |    370,127 |    370,127 |
| 8_0_x                                     |    362,577 |    366,055 |    362,612 |    362,577 |    362,577 |
|                                           |            |            |            |            |            |
| Wall time                                 |            |            |            |            |            |
|                                           |            |            |            |            |            |
| perf-vbf-and-entity-display |    682,880 |    746,898 |    697,731 |    697,906 |    703,545 |
| perf-views-base-fields                    |    744,188 |    790,561 |    756,503 |    756,479 |    764,560 |
| perf-entity-display                       |    476,966 |    555,423 |    484,705 |    484,519 |    489,298 |
| 8_0_x                                     |    473,945 |    534,991 |    481,349 |    480,558 |    487,271 |
|                                           |            |            |            |            |            |
| Memory usage                              |            |            |            |            |            |
|                                           |            |            |            |            |            |
| perf-vbf-and-entity-display | 28,754,032 | 29,061,672 | 28,759,016 | 28,754,032 | 28,754,344 |
| perf-views-base-fields                    | 27,459,312 | 27,929,608 | 27,464,759 | 27,459,856 | 27,459,856 |
| perf-entity-display                       | 27,128,416 | 27,468,352 | 27,133,765 | 27,128,480 | 27,128,480 |
| 8_0_x                                     | 26,757,672 | 27,185,432 | 26,763,721 | 26,757,672 | 26,757,882 |
|                                           |            |            |            |            |            |
| Peak memory usage                         |            |            |            |            |            |
|                                           |            |            |            |            |            |
| perf-vbf-and-entity-display | 29,277,736 | 29,588,096 | 29,282,958 | 29,277,736 | 29,278,504 |
| perf-views-base-fields                    | 27,998,504 | 28,472,960 | 28,005,273 | 28,000,800 | 28,000,800 |
| perf-entity-display                       | 27,656,480 | 28,003,496 | 27,662,018 | 27,656,600 | 27,656,600 |
| 8_0_x                                     | 27,284,952 | 27,715,704 | 27,291,083 | 27,284,952 | 27,285,108 |
|                                           |            |            |            |            |            |
+-------------------------------------------+------------+------------+------------+------------+------------+

@Fabianx:

Thanks, those are more or less the same numbers I'm seeing locally. Just wanted to outline that from my measurements #48 is slightly faster than #31, so things didn't get worse in the process.

The test view above is probably one of the worst scenarios for the comparison with HEAD: it's a table view with 9 base fields and a single configurable field, this explains why this patch is slightly slower than HEAD. However if we were displaying 10 configurable fields this would be faster than HEAD.

Basically this provides a 9% ~ 12% performance improvement on views displaying ~ 10 fields, which should be a common scenario. However callgraphs clearly show that we should look for more improvements on the render side, hence I think the only hope we have to reduce the performance penalty introduced by #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is #2450897: Cache Field views row output.

Working on test coverage now.

This should address #33.2, tests still to be added. Too many line changes, no interdiff.

This should fix test failures and address #49. I'm not sure we need unit tests for this code: we have many tests covering exactly most of the cases it handles, only relationships were not well covered. I'm afraid we would be basically duplicating the existing tests.

@Daniel:
What do you think?

does this need another performance analysis,
or just reviews?
change record?

Reviewed the interdiffs since my last patch:

1. +++ b/core/modules/views/src/Tests/ViewResultAssertionTrait.php
   @@ -109,7 +109,15 @@ protected function assertIdenticalResultsetHelper($view, $expected_result, $colu
   +          $row[$expected_column] = $value[$expected_column];
   

   Don't we need to cast this to string too?

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -690,6 +691,74 @@ public function renderItems($items) {
   +   * Therefore this method filters out limits the items, reorders the items
   +   * as well as takes into account an offset.
   

   This sentence doesn't look correct to me.

3. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php
   @@ -553,6 +555,82 @@ public function testQueryWithGroupByForConfigField() {
   +  public function providerTestPrepareItemsByDelta() {
   

   Missing PHP doc.

4. +++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
   @@ -320,7 +376,72 @@ public function testRevisionRender() {
   +   * Tests the output of a revision view with base fields and configurable fields.
   

   Exceeded 80 chars limit.

5. +++ b/core/modules/views/src/Tests/ViewResultAssertionTrait.php
   @@ -112,8 +112,11 @@ protected function assertIdenticalResultsetHelper($view, $expected_result, $colu
   +        // This case is about fields with mulitple values.
   

   Typo

Daniel and I did pretty distinct work: I focused on the refactoring and he focused mostly on test coverage. I guess once both of us is happy with the respective changes either of the two could RTBC the patch. However a third person performing a final review would be welcome.

It would be good if @Fabian could provide updated numbers to confirm I did not introduce a performance regression.

Reviewing today or tomorrow ...

I found nothing in my review today.

I will review and hopefully RTBC again tomorrow.

Thanks Fabian, if you could profile this again to ensure I didn't introduce any significant regression in the last iterations it would be great.

Here we go:

### FINAL REPORT

=== 8.0.x..8.0.x compared (550f82638311b..550f838143987):

ct  : 600,174|600,174|0|0.0%
wt  : 764,153|763,582|-571|-0.1%
mu  : 27,275,888|27,275,888|0|0.0%
pmu : 27,819,448|27,819,448|0|0.0%

=== 8.0.x..issue-1867518 compared (550f82638311b..550f84122f50a):

ct  : 600,174|565,627|-34,547|-5.8%
wt  : 764,153|687,814|-76,339|-10.0%
mu  : 27,275,888|28,435,448|1,159,560|4.3%
pmu : 27,819,448|28,964,488|1,145,040|4.1%

=== SUM: 8_0_x-summary..issue-1867518-summary compared (550f8440b2d3e..550f84474de28):

ct  : 60,020,803|56,566,199|-3,454,604|-5.8%
wt  : 77,588,946|70,259,099|-7,329,847|-9.4%
mu  : 2,728,032,936|2,843,973,168|115,940,232|4.2%
pmu : 2,782,455,912|2,896,887,072|114,431,160|4.1%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-1867518     |    565,627 |    569,126 |    565,662 |    565,627 |    565,627 |
| 8_0_x             |    600,174 |    603,577 |    600,208 |    600,174 |    600,174 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-1867518     |    687,814 |    744,665 |    702,591 |    702,014 |    712,984 |
| 8_0_x             |    763,582 |    815,454 |    775,889 |    775,099 |    785,034 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-1867518     | 28,434,728 | 28,740,888 | 28,439,732 | 28,435,448 | 28,435,448 |
| 8_0_x             | 27,275,496 | 27,660,272 | 27,280,329 | 27,275,888 | 27,275,888 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-1867518     | 28,963,840 | 29,270,640 | 28,968,871 | 28,964,488 | 28,964,488 |
| 8_0_x             | 27,819,448 | 28,207,856 | 27,824,559 | 27,819,448 | 27,820,824 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

Reviewed again, performance is good (10% faster compared to the 50% slower regression we introduced earlier)!

Test coverage seems to cover all tricky cases.

RTBC

Is the 4% increase in memory usage expected? Why?

Let's get some understanding around #82

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -329,6 +328,14 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +  public function prepareView($entities, $displays, $view_mode) {
   +    // Invoke hook_entity_prepare_view().
   +    $this->moduleHandler()->invokeAll('entity_prepare_view', array($this->entityTypeId, $entities, $displays, $view_mode));
   +  }
   

   Still not a fan of splitting this part of buildComponents() into a new public API on EVB. Two public methods, one being called by the other, confuses the external facing API of EVB.

   - Can't EntityFieldRenderer leverage EVB::buildComponents() directly ?
   - If not, can't EntityFieldRenderer simply repeat the invokeAll('entity_prepare_view') ?

2. +++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
   @@ -0,0 +1,358 @@
   +  protected function getCustomViewMode($entity_type_id) {
   +    return 'views:' . $this->view->storage->id() . ':' . $this->view->current_display . ':' . $entity_type_id;
   +  }
   

   Do we really need a dedicated view_mode name here ?

   EntityViewDisplay skips its init() when the view_mode === EntityDisplayBase::CUSTOM_MODE ('_custom'), and i think we do need to skip init() here :

   - it is not actually needed in our case here, and thus costs needless CPU cycles

   - it forces base fields that do not specify display options back to "hidden" (that's intended for "regular" view modes - but here it would mean that for example 'nid' could never be displayed even in a View ?)

OMG, yes. That is what I had the whole time on my mind and just could not grasp.

Yes, the same field formatter with different configurations is a must to support.

Just a reroll for now.

@Wim:

This was not exactly trivial due to the introduction of cache contexts. Could you check the interdiff and see whether it still makes sense? The entity field renderer now proxies the call to the entity translation renderers responsible of altering the view query. Is that enough?

Not sure how, but it happens. Maybe it's called before populatiing the array...

This should be green and address #85.2. Tomorrow I will post some details, now too late, I almost fell asleep while running tests ;)

#89: Well, to be fair, it's a rather complex combination of 2 levels of base classes and traits, plus layers of indirection. That's the true source of complexity here. So, HEAD is actually wrong: RendererBase::getCacheContexts() should return the empty array. DefaultLanguageRenderer and ConfigurableLanguageRenderer then also return the empty array, which is correct, since they both use non-request-dependent entity translations. Only TranslationLanguageRenderer depends on the request context.
AFAICT that's exactly what you changed it into, so, yes, the #89 interdiff is correct :)

I guess you lost your color vision.

LOL :)

@plach: I split that cache context stuff off into a separate issue, since it really is a problem of its own in HEAD. Also helps simplify this patch a bit: #2462851: Improve Views entity row renderer plugins' cache contexts.

Just a clarification: calling FieldItemsList::view() still involves building display objects, so we would need to more or less inline EVD::buildMultiple() instead.

I think that would also make it more difficult to enable render caching in the future, because then you're definitely going to avoid using EntityViewBuilder forever.
Of course, unless I'm mistaken, #2450897: Cache Field views row output is going to have its own render caching anyway… so I don't think it really matters.

I think the root cause of the trickiness here is that EntityViewDisplay is a very strange beast. If you could create an EntityViewDisplay object and then give it the entities that need to be rendered and not have to talk to the view builder also, that'd help a lot, I think.
EntityFormDisplay does work that way, IIRC.
I had a discussion about this with @swentel and others about all this a year ago, at DDD Szeged. We opened an issue, but I'm unable to find it right now.

Assigning to yched for feedback.

Especially EntityViewDisplay just doesn't work together with the model or rendering arbitrary fields, across bundles and multiple instances, with relationships and everything.

Completely true. Views has more advanced needs than EVD is designed for.

+1 to requiring @yched's feedback.

"Before" == #93/earlier iterations of this patch, "after" == latest patch, right?

+++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
@@ -121,113 +132,159 @@ public function preRender(array $values) {
+//    $entities = [];
+//    $translations = [];
+//    $bundle_field_names = [];

etc.

There's a lot commented out there :)

If you could create an EntityViewDisplay object and then give it the entities that need to be rendered and not have to talk to the view builder also, that'd help a lot, I think.

Well, "create an EntityViewDisplay object and then give it the entities that need to be rendered" is basically what the current patch does, no EVB is involved in the process ? (well, except for calling hook_entity_prepare_view(), which, as noted in #85, is not something I think we need an EVB for)

But yeah, an EVD is about a bundle, i.e rendering the same collection of fields with the same settings on a series of entities (of the same bundle). Using multiple ones for rendering a list of entities spanning several bundles belongs to the layer above (e.g EntityViewBuilder does that). Relationships are very much a Views thing, so EVD cannot handle that.

Aside from reducing the callstack, the big promise of using EVD::buildMultiple($entities) was to use FormatterInterface::prepareView($entities) once per field for all rendered rows, instead of running it row by row so far. At this point, prepareView() is mostly for multiple-loading of referenced entities for ER / File formatters (it makes a big difference there, though).

Then, having EVD being the one unified way to run formatters of fields in an entity also meant better maintainability by avoiding duplicated logic, and ansuring the code flow is the same (same hooks invoked, etc... IIRC, ensuring that the same hooks run was the main reason why D7 Views used field_attach_view() in single-field mode, which is basically what D8 HEAD still does, in a post-field_attach_view() codebase).

But well, if not using EVDs and writing separate logic means 10% more efficient code, I guess that's hard to argue against :-)
Slightly worried about having to maintain two separate code flows, though. The idea in the long run was to try to unify Views rendering and non-Views entity rendering, not split them apart :-/

I agree that with EVD as currently in HEAD (coupled to bundles, and each field only appearing once), it's not a good fit for the Views use case, where you typically have the same field display settings for a given Views display even though the entities being rendered are in different bundles. What's critical about this issue is not to use EVD, but to speed up the way Views renders multiple fields for multiple entities. So, I suggest retitling and resummarizing this issue for that.

I think one or several followups to either refactor EVD to have fewer of those limitations, or to split out the EVD::buildMultiple() implementation into something that can be used by both EVD and Views, in order to centralize the logic referenced in #107 and #100, would be good though.

#2462851: Improve Views entity row renderer plugins' cache contexts landed! That should make this issue a tiny bit smaller.

Patch rerolled.

Well the solution would be the following: Get rid of the idea to couple EVD with bundles, but instead just allow to pass in a couple of components, which have the used field names as part of it, but don't assume to not be able to render the same field twice etc.

1) Allowing EVDs to render the same field twice :

It is a long-standing wish of mine, but there are non minor challenges:
- Impacts how you work with entity templates. You can do {{ content.field_foo }} because you know that by convention the $content contains one entry per field, named after the field. If the field appears more than once, then who decides how each is named, and how do I know it as a tpl author ?
- Related, not sure how that would work with in-place editing
- Would need a complete rethink of the "Manage display" UI screen, to move from the current "fixed list of possible pieces of content (one per field), you can hide some" to something more like Views' "add a new piece of content"
- Rendering the same field twice is only really interesting if you can do #1234624: Limit the number of fields to display, which was deemed "too late" a while ago

My gut feeling says "doubtful at this point".

2) Allowing EVDs to render entities across bundles :

Yeah, EVDs are currently modeled as "the config entities for Field UI's "Manage Fields" screen *on a specific bundle*". The config entity ID is keyed by entity type and bundle.

Maybe we could untie that strong "link to a specific bundle" for runtime, non-config EVDs - meaning, if you don't have a $this->bundle, you can be used for runtime rendering, but you can't be saved in config.

Then we'd need to change the way EVDs collect the (bundle specific) field definitions they pass to each formatter.
Currently that's done "statically" in EntityDisplayBase::getFieldDefinitions() based on $this->targetEntityType / $this->bundle.
It would need to happen dynamically based on each specific $entity being rendered.

That one seems doable, I could try to work on that in Dev Days next week.

Question is : is working on 2) worhtwhile for Views if 1) stays out of reach ? I.e, would 2) alone alleviate enough of the complexity / CPU overhead that Views could switch back to using EVDs ?

Meanwhile, agreed that it's safer for this critical here to pursue the "separate code" approach from the latest patches. We'll need to closely track the code flow and hook invocations though.

@dawehner: $entity->field->view() uses EVD for the same reason field_view_field() uses field_attach_view() in D7: to ensure the same logic and hooks apply

And the big drawback of single-field view() is that you lose the single prepareView() across multiple entities.

But this makes me think : if we find we have to duplicate field rendering code, maybe it would be best if it was duplicated between EVD and $entity->field->view() rather than between EVD and Views ? Keeps the duplicate code a bit closer, and optimises $entity->field->view() for everyone instead of just Views ?

Then, Views would use
- EVDs (across bundles if we can do #112.2) on entities containing the first instance of each field, and for relationships (rendering a field with a relationship is really rendering a different entity, you can't reuse the same EVD). There you benefit of multiple processing
- one-off $entity->field->view() for fields that appear for a 2nd/3rd/Nth time

Thoughts ?

With this post I am trying to take a step back again and get back to the "drawing board" first. (Note: This includes views row #cache as a concept just to ensure all edge cases are properly supported).

Current State (As I understand it)

I have a number of rows and each row has one or more fields associated with it, all rows are configured to render in a certain way, however single rows can be rendered differently.

How I would implement it (naively)

So lets say I am trying to re-implement a 'views light', but I skip row style and field style plugins for now, however I still have different entities in my input data.

So my exemplary input could look like this:

$this->rows = [
  [ 'entity_article' => ['article', 10], 'entity_user' => ['user', 42] ],
  [ 'entity_article' => ['article', 20], 'entity_user' => ['user', 23] ],
];

While my configuration per row could look like this:

$this->fields = [
  0 => [ 'entity' => 'entity_article', 'field name' => 'title', 'field config' => [...]],
  1 => [ 'entity' => 'entity_user', 'field name' => 'name', 'field config' => [...]],
  2 => [ 'entity' => 'entity_article', 'field name' => 'body', 'field config' => [...]],
  3 => [ 'entity' => 'entity_article', 'field name' => 'title', 'field config' => [..., 'formatter' => 'summary_trimmed']],
];

So pseudo-code I would write to render that could be like:

function render() {
  $rendered_rows = [];

  foreach ($this->rows as $index => $row) {
    $rendered_rows[$index] = $this->renderRow($row);
  }
}

And in renderRow:

function renderRow($row) {
  $rendered_fields = [];
  foreach ($this->fields as $index => $field) {
    $rendered_fields[$index] = $this->renderField($field, $row);
  }

  return implode('|', $renderer_fields); // This is the row style formatter, omitted for brevity.
}

And in renderField:

function renderField($field, $row) {
  $entity_name = $field['entity'];

  $entity = entity_load($row[$entity_name][0], $row[$entity_name][1]);
  $field_name = $field['field name'];
  $field_config = $field['field configuration'];

  return '<div class="field">' . $entity->get($field_name)->view($field_config) . '</div>';
}

Adding row caching

Adding row caching is for this simplified example pretty trivial:

function getCachedRows($rows) {
  $cid_map = [];
  foreach ($rows as $index => $row) {
     $cid_map[$index] = $this->createRowCacheID($row);
  }

  if (empty($cid_map)) {
    return [];
  }

  $cids = array_filter(array_values($cid_map)); // Filter out non-cacheable rows, which have $cid == FALSE / NULL

  $cached_data = $this->cache->getMultiple($cids);

  $return = [];
  foreach ($cid_map as $index => $cid) {
    if (isset($cached_data[$cid])) {
      $return[$index] = $cached_data[$cid]->data;
    }
  }

  return $return;
}

function render() {
  $rendered_rows = $this->getCachedRows($this->rows);

  foreach ($this->rows as $index => $row) {
    if (!isset($rendered_rows[$index])) {
      $rendered_rows[$index] = $this->renderRow($row);
    }
  }

  $this->setCachedRows($this->rows, $rendered_rows); // Optimization to not cache already cached data omitted for brevity reasons.
}

So row caching happens before rendering the rows at all, that means all other code is not affected by this, which is good.

However this also shows that the renderer cache API should actually have a public API as this clearly shows that #pre_render / #cache is not enough, as else we cannot multiple prepare the rows and my little cache get code would not take care of bubbling cache contexts properly (using e.g. #cache_redirect internally).

I will open an issue for that.

Optimizing the code

So now after the cache get we have the following to get all rows that still need to be rendered and we add in a prepare function to optimize:

function render() {
  $rendered_rows = $this->getCachedRows($this->rows);

  $still_to_render = array_diff_key($this->rows, $rendered_rows);

  $this->preparedRows = $this->prepareRows($still_to_render);

  foreach ($still_to_render as $index => $row) {
    $row['index'] = $index;
    $rendered_rows[$index] = $this->renderRow($row);
  }

  $this->setCachedRows($this->rows, $rendered_rows); // Optimization to not cache already cached data omitted for brevity reasons.
}

so that renderRow looks like this:

<?php
function renderRow($row) {
  $rendered_fields = [];
  foreach ($this->fields as $index => $field) {
    $field['index'] = $index;
    $rendered_fields[$index] = $this->renderField($field, $row);
  }

  return implode('|', $renderer_fields); // This is the row style formatter, omitted for brevity.
}

and that renderField now looks like this:

function renderField($field, $row) {
  return '<div class="field">' . $this->preparedRows[$row['index']][$field['index']] . '</div>';
}

Now we only need the $this->prepareRows() function and this is what this ticket is about.

Prepare Rows

Because this is an exercise in understanding lets try to see how an ideal API for that use case would work.

function prepareRows($rows) {
  $prepared_rows = [];
  $entities_by_type = [];
  $entity_data_by_name = [];
  $row_map = [];

  // Find all entities and load them by type
  foreach ($rows as $row_index => $row) {
    foreach ($this->fields as $field_index => $field) {
      $entity_name = $field['entity'];
      $entity_type = $row[$entity_name][0];
      $entity_id = $row[$entity_name][1];

      $entities_by_type[$entity_type][] = $entity_id;
      $entity_data_by_name[$entity_name][$entity_type][] = $entity_id;
      $row_map[$entity_name][$entity_id] = $row_index; // @todo this might not be unique
    }
  }

  foreach ($entities_by_type as $entity_type => $entity_ids) {
    $loaded_entities_by_type = entity_load($entity_type, $entity_ids);
  }

  $builders_by_type = [];
  $render_entities_by_type = [];

  // Now lets prepare a display object for all the entities.
  foreach ($this->fields as $field_index => $field) {
     $entity_name = $field['entity'];
     $entity_type = reset(array_keys($entity_data_by_name[$entity_name]));

      if (!isset($builders_by_type)) {
        $builders_by_type[$entity_name] = new SimpleEntityViewDisplay($entity_type);
      }
     $field_name = $field['field name'];
     $field_config = $field['field configuration'];

     $builders_by_type[$entity_name]->addField($field_index, $field_name, $field_config); // This API does not exist, but this would make sense.
  }

  foreach ($builders_by_type as $entity_name => $evb) {
     $render_entities = [];

     // Now find the entities
     foreach ($entity_data_by_name[$entity_name] as $entity_type => $entity_ids) {
        foreach ($entity_ids as $entity_id) {
          $render_entities[] = clone $loaded_entities_by_type[$entity_type][$entity_id];
        }
     }

     $rendered = $evb->render($render_entities); // This API may not exist like this
     foreach ($rendered as $entity_id => fields) {
        $row_index = $row_map[$entity_name][$entity_id];
        foreach ($fields as field_index => $data) {
          $prepared_rows[$row_index][$field_index] = $data;
        }
     }
   }

  return $prepared_rows;
}

Even with an ideal Entity View Builder the code remains challenging.

However as the configuration for the different bundles is merely cloned we could use an Adapter pattern until EVB upstream supports something that is not configured per bundle.

Simplify

Okay, lets take another step back and do the simplest solution possible: Render all fields horizontally, that code looks like this:

( For brevity reasons the entity_load_multiple is replaced with a single loading here, as that optimization is trivial)

function prepareRows($rows) {

  $prepared_rows = [];

  foreach ($this->fields as $field_index => $field) {
     $entity_name = $field['entity'];
     $field_name = $field['field name'];
     $field_config = $field['field configuration'];

     $entities = [];
     $entity_types = [];
     $row_map = [];
     $rendered = [];

     foreach ($rows as $row_index => $row) {
        list($entity_type, $entity_id) = $row[$entity_name];
        $entity = clone entity_load_single($entity_type, $entity_id);
        $entities[$entity_type][$entity_id] = $entity;
        $entity_types[$entity_type] = $entity_type;

        // Store the address where we will find the data.
        $row_map[$row_index] = [$entity_type, $entity_id, $field_name];
     }

     foreach ($entity_types as $entity_type) {
        $evb = new SimpleEntityViewDisplay($entity_type);
        $evb->addField($field_name, $field_config);
        $rendered[$entity_type] = $evb->buildMultiple($entities[$entity_type]);
     }

    // Translate from entity_id / field_name back to row_index / field_index.
    foreach ($row_map as $row_index => $data) {
        list($entity_type, $entity_id, $field_name) = $data;
        $prepared_rows[$row_index][$field_index] = $rendered[$entity_type][$entity_id][$field_name];
    }
  }

  return $prepared_rows;
}

This looks better. Now all we need to do is to unroll the inner loop into preparation and now that we did this pre-work this becomes surprisingly simple.

Support multiple fields with the same name

To support multiple fields we need to add a $builder_index, which changes when the count for the seen field_name per entity_type changes.

function prepareRows($rows) {

  $builder_seen = [];
  $prepared_rows = [];
  $entities = [];
  $builders = [];
  $rendered = [];
  $row_map = [];

  foreach ($this->fields as $field_index => $field) {
     $entity_name = $field['entity'];
     $field_name = $field['field name'];
     $field_config = $field['field configuration'];

     $entity_types = [];

     // Get all entity types.
     foreach ($rows as $row_index => $row) {
        list($entity_type, $entity_id) = $row[$entity_name];
        $entity_types[$entity_type] = $entity_type;
     }

      $builder_index_map = [];

     // Prepare the builder index per entity type.
     foreach ($entity_types as $entity_type) {
        if (!isset($builder_seen[$entity_type][$field_name])) {
          $builder_seen[$entity_type][$field_name] = 0;
        }
        $builder_index_map[$entity_type] = $builder_seen[$entity_type][$field_name];
        $builder_seen[$entity_type][$field_name]++;
     }

     foreach ($rows as $row_index => $row) {
        list($entity_type, $entity_id) = $row[$entity_name];
        $builder_index = $builder_index_map[$entity_type];

        $entity = clone entity_load_single($entity_type, $entity_id);
        $entities[$builder_index][$entity_type][$entity_id] = $entity;

        // Store the address where we will find the data.
        $row_map[$row_index][$field_index] = [$builder_index, $entity_type, $entity_id, $field_name];
     }

     // Prepare the builders.
     foreach ($entity_types as $entity_type) {
        if (!isset($builders[$builder_index][$entity_type])) {
          $builders[$builder_index][$entity_type] = new SimpleEntityViewDisplay($entity_type);
        }
        $builders[$builder_index][$entity_type]->addField($field_name, $field_config); // The field is now unique thanks to the builder index.
     }
  }

  // Render the data per builder
  foreach ($builders as $builder_index => $entity_types) {
     foreach ($entity_types as $entity_type) {
        $rendered[$builder_index][$entity_type] = $builders[$builder_index][$entity_type]->buildMultiple($entities[$builder_index][$entity_type]);
     }
  }

  // Translate from entity_id / field_name back to row_index / field_index.
  foreach ($row_map as $row_index => $field_map) {
    foreach ($field_map as $field_index => $data) {
      list($builder_index, $entity_type, $entity_id, $field_name) = $data;
      $prepared_rows[$row_index][$field_index] = $rendered[$builder_index][$entity_type][$entity_id][$field_name];
    }
  }

  return $prepared_rows;
}

So regardless of the implementation, in the end it is a simple mapping from entity_id / field_name back to row_index / field_index.

Note that an implementation that uses field aliases for fields to add them to the Entity View Builder would still need a kind of 'seen' functionality per field per entity, but views already has unique identifiers, so that is not an issue.

All that changes then in the translation function is the omitting of the $builder_index and usage of a $field_alias instead:

  // Translate from entity_id / field_name back to row_index / field_index.
  foreach ($row_map as $row_index => $field_map) {
    foreach ($field_map as $field_index => $data) {
      list($entity_type, $entity_id, $field_alias) = $data;
      $prepared_rows[$row_index][$field_index] = $rendered[$entity_type][$entity_id][$field_alias];
    }
  }

That all explored, the answer to ycheds question are:

- Yes, this is generically useful to be able to have the bundle be optional, however an adapter could be written for now.
- And yes, field aliases would be nice to specify when adding a field so the $builder_index can be omitted and performance even more improved, however a build_index can be calculated, too.

That said I will see if my theory code can actually work ;)

TL;DR:

I explored various scenarios going from naive a implementation O(n * m) to using field based (column) multiple rendering, to the most complex creating multiple view builders per entity_type if fields are duplicated with O(k) performance.

I found out that we miss APIs for proper views row caching / render caching outside of #pre_render, #cache pattern.

I will check what would be involved to support the SimpleEntityViewDisplay API that would feel "naturally" to me.

#116 was a case study and mainly sharing my thoughts, however the code could be used to develop a good and abstract EntityViewDisplay that is dynamic and solves views use cases.

#117:

Lets discuss views row caching in the row caching ticket, but in short the problem we are facing is:

- You have a list of rows
- You need to prepare something for all rows
- You want to cache rows

If you use cache->get / cache->set directly, then you are not doing it right as any post-bubbled context would not be taken into account in your $cid.

So you must use #pre_render / #cache pattern, but that is impossible to combine with a cache multi get to never prepare anything for the rows that are cached before.

Hence opened #2466585: Decouple cache implementation from the renderer and expose as renderCache service.

--

a) That is not true, multiple builders for similar fields are handled in the last example.
b) Yes and no, this is something anyone building lists of multiple entities with a certain configuration of fields will need to deal with.

PS: Updated to the fantasy SimpleEntityViewDisplay class.

Tagging for D8MI.

This tries to the latest incorporate feedback from @yched, at least what makes sense for the scope of this issue. Most of the work here was funded by the Drupal Association as a D8 Accelerate grant, and this is critical because of the performance gain. For this reason I tried to address @yched's concerns without introducing too many changes to the current architecture and leave the door open for additional refactorings as #2466913: Allow EVD to render fields across bundles. For the same reason I didn't try to implement Fabian's suggestions: the performance gain we have now is real, we should get it and build on top of it in non-critical issues.

I removed the hook_entity_prepare_view() invocation as the new approach does not support it, at least until we do #2466913: Allow EVD to render fields across bundles, however this is not a regression as in HEAD we don't do that either.

From my profiling this is still ~10% faster and uses ~4% more memory. I still have to investigate that.

+++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
@@ -0,0 +1,237 @@
+      // Build field render arrays for the current entity types.
+      $displays[$entity_type_id]->buildFieldMultiple($this->build, $entities[$entity_type_id], $field->getFieldDefinition(), $field->options['type'], $field->options['settings'], $field_id);

Yes, this makes a lot of sense.

This is not too different from what I had FWIW and great that seemingly it does not matter where the loop is - as long as we render and prepare all rows at the same time for each field.

The field_id or field_alias indeed would need to be supported by EntityViewDisplay as well if we do the bundle thing.

However I feel that the code is re-used enough to not warrant that anymore now that we see how simple that is.

I still don't understand some things for re-use of the entityRenderer, but will check that code part again later.

Looks great!

The 4 MB more memory are likely the temporary cache for the build of the entity renderer. Destroying that after the rendering is done could help perhaps.

Fixed test failures, hopefully.

This should address #126. The main reason I picked that weird DX for ::buildFieldMultiple() was to avoid having to merge results in EntityFieldRenderer::buildFields(). However after additional profiling the difference was below the average variance. I compared the two fastest runs and exclusive diff for EntityFieldRenderer::buildFields() was ~1ms with 50 result items. I'm attaching also my current test site DB dump and my test view so people can profile this themselves.

This is the interdiff to compare only #126.2.

Thanks to Fabian's suggestion, this patch reduces the memory consumption increase to ~0.5%-1%.

@Fabian:

A final round of profiling to confirm my results would be great :)

Sure, I can do another round of profiling.

Can we make this green again before that?

Also I think its better to do the removal in a postRender / unPrepare stage? (and not per row)

Also (but out of scope for this issue): If that works, does that mean we hold references to the view object _after_ its lifetime so we never free the memory (of all the views handling)?

Is that because those are plugins that are generated in the container?

Is there other internal data like $this->view that we don't destroy?

I suspected similar in Drupal 7 for views, but was not able to pin-point the cause ...

So a follow-up issue for that would be great, I think!

Not much time to look at this right now, but this and #2466913: Allow EVD to render fields across bundles are on top of my hitlist for Dev Days next week.

Minor remark on the latest interdiff:

+++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
@@ -119,6 +119,10 @@ public function query(QueryPluginBase $query) {
   public function preRender(array $values) {
+    if (isset($this->build) || empty($values)) {
+      return;
+    }
+
     $this->buildFields($values);
   }

Then I guess we can switch to just if ($values && !isset($this->build)) {$this->buildFields($values); } ?

This should be green again.

Also I think its better to do the removal in a postRender / unPrepare stage? (and not per row)

There is no such method to override for the Field handler, nor for the Display and Style plugins, hence we'd need to introduce it, and propagate it down the line to Field handlers. Moreover we'd be calling the ::postRender() method once for each field, but we'd actually need it just once, so I think this wouldn't help with performance, aside from being a bit out of scope.

If this is green and profiling numbers are still good, I think we should be ready to get back to RTBC now.

Updated issue summary

After some work on #2450897: Cache Field views row output, I realized lazy-building fields is better than pre-building them.

@yched:

This also somehow addresses #133, it would be great if you could perform a full review :)

Yeah! That is a nice idea!

Fixed relationship handling, due to the latest commits. This might be green again.

Fixed the last failures. Reviews welcome...

I have been testing this patch yesterday. The overall performance improvement is significant (50 - 55%), but digging into the XHprof runs I'm seeing some interesting changes which I cannot completely understand.

test setup

• XHprof, latest version, compiled with git, enabled with xhprof_enable(XHPROF_FLAGS_NO_BUILTINS); in index.php (I had to use the no-builtins flag to prevent segfaults on my php 5.6.7 machine)
• Standard Drupal 8 install
• 100 article nodes, generated with devel_generate
• A view showing 100 nodes, 5 fields per row (see the attachment fields-test.txt for the export)
• I did 10 test runs before applying the patch, then applied the patch, did a drush cr, reloaded the page once and 10 more test runs

results

Building the page consistently took between 4.0 and 4.4 seconds before the patch, and between 1.8 and 2.1 seconds after the patch. That's a huge win. Comparison of two typical runs, made with the XHprof UI:

Now what's really interesting, is the time spent in Drupal\Core\Database\Statement::execute. While practically everything is faster, that function went from 176,461 to 734,029 microseconds (Excl. Wall Time).

I'm not sure what this means. Maybe this is completely logical, and related to how the performance improvement was achieved. It could also mean that some index is missing, or certain queries are not really efficient. In that case an even bigger performance increase might be possible.

Assigning to yched for final sign-off / review - though we definitely should see which $view->render() calls we can remove now again.

I will also do another profile of this ...

Added proposed commit message: Profiling takes a lot of time, too.

Skimming through the patch, it looks awesome.

Also, thank you, @marcvangend! Excellent, excellent profiling!
Regarding the increase in time spent in the DB: that's to be expected to some extent, because it causes more render cache gets. But it seems a huge increase. I'd compare the time spent in the DB cache backend's ::get() function to the total time spent in the DB, if not 90% or more of the time in the DB is the time for the cache get, then something else is causing queries.

### FINAL REPORT

=== 8.0.x..8.0.x compared (552f8ad534e45..552f8b8d9d137):

ct  : 498,833|498,833|0|0.0%
wt  : 634,438|636,206|1,768|0.3%
mu  : 25,622,432|25,622,432|0|0.0%
pmu : 26,070,536|26,070,536|0|0.0%

=== 8.0.x..issue-1867518--entity-display compared (552f8ad534e45..552f8bebdbabd):

ct  : 498,833|462,348|-36,485|-7.3%
wt  : 634,438|575,718|-58,720|-9.3%
mu  : 25,622,432|26,104,568|482,136|1.9%
pmu : 26,070,536|26,567,584|497,048|1.9%

=== SUM: 8_0_x-summary..issue-1867518--entity-display-summary compared (552f8c1431dcb..552f8c1a392ee):

ct  : 49,886,490|46,237,990|-3,648,500|-7.3%
wt  : 64,524,153|58,505,873|-6,018,280|-9.3%
mu  : 2,562,803,104|2,611,018,216|48,215,112|1.9%
pmu : 2,607,602,280|2,657,353,144|49,750,864|1.9%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------------------+------------+------------+------------+------------+------------+
| namespace                     |        min |        max |       mean |     median |       95th |
+-------------------------------+------------+------------+------------+------------+------------+
| Calls                         |            |            |            |            |            |
|                               |            |            |            |            |            |
| issue-1867518--entity-display |    462,348 |    465,538 |    462,380 |    462,348 |    462,348 |
| 8_0_x                         |    498,833 |    502,023 |    498,865 |    498,833 |    498,833 |
|                               |            |            |            |            |            |
| Wall time                     |            |            |            |            |            |
|                               |            |            |            |            |            |
| issue-1867518--entity-display |    575,718 |    636,996 |    585,059 |    584,136 |    592,481 |
| 8_0_x                         |    636,206 |    689,209 |    645,242 |    645,626 |    650,797 |
|                               |            |            |            |            |            |
| Memory usage                  |            |            |            |            |            |
|                               |            |            |            |            |            |
| issue-1867518--entity-display | 26,104,568 | 26,480,520 | 26,110,182 | 26,104,568 | 26,104,776 |
| 8_0_x                         | 25,621,992 | 26,184,456 | 25,628,031 | 25,622,432 | 25,622,432 |
|                               |            |            |            |            |            |
| Peak memory usage             |            |            |            |            |            |
|                               |            |            |            |            |            |
| issue-1867518--entity-display | 26,567,584 | 26,941,040 | 26,573,531 | 26,567,584 | 26,569,056 |
| 8_0_x                         | 26,069,264 | 26,630,960 | 26,076,023 | 26,070,536 | 26,070,536 |
|                               |            |            |            |            |            |
+-------------------------------+------------+------------+------------+------------+------------+

This are my numbers with plach' view.

We still save 9-10%, which is fine, the remainder we will get back with row caching.

--

So yched review, remove no longer needed ->render() calls, RTBC and commit! :)

I've been digging deeper into the profiler comparison and found that the increase in database time is caused by inserts. From the XHprof UI:

Regression/Improvement summary for Drupal\Core\Database\Connection::query

Drupal\Core\Database\Connection::query	Run #552e86e18369e	Run #552e87948c455	Diff	Diff%
Number of Function Calls	775	380	-395	-51.0%
Incl. Wall Time (microsec)	202,249	749,172	546,923	270.4%
Incl. Wall Time (microsec) per call	261	1,972	1,711	655.5%
Excl. Wall Time (microsec)	8,389	7,314	-1,075	-12.8%

Parent/Child Regression/Improvement report for Drupal\Core\Database\Connection::query [View Callgraph Diff]

Function Name	Calls Diff	Calls Diff%	Incl. Wall Diff (microsec)	IWall Diff%
Current Function
Drupal\Core\Database\Connection::query	-395	-20.0%	546,923	24.3%
Exclusive Metrics Diff for Current Function			-1,075	-0.2%
Parent functions
Drupal\Core\Database\Driver\mysql\Insert::execute	168	42.5%	694,908	127.1%
Drupal\Core\Cache\DatabaseBackend::getMultiple	-495	-125.3%	-126,221	-23.1%
Drupal\Core\Database\Query\Select::execute	-66	-16.7%	-21,316	-3.9%
Drupal\Core\Cache\DatabaseCacheTagsChecksum::calculateChecksum	-1	-0.3%	-804	-0.1%
Drupal\Core\Routing\RouteProvider::getRoutesByNames	0	0.0%	-681	-0.1%
Drupal\Core\Routing\RouteProvider::getRoutesByPath	0	0.0%	351	0.1%
Drupal\Core\Session\SessionHandler::read	0	0.0%	264	0.0%
Drupal\Core\Path\AliasStorage::lookupPathSource	0	0.0%	261	0.0%
Drupal\Core\KeyValueStore\DatabaseStorage::getMultiple	-1	-0.3%	161	0.0%
Child functions
Drupal\Core\Database\Statement::execute	-395	-25.0%	557,539	101.9%
Drupal\Core\Database\Connection::prepareQuery	-395	-25.0%	-5,115	-0.9%
Drupal\Core\Database\Connection::expandArguments	-395	-25.0%	-4,055	-0.7%
Drupal\Core\Database\Connection::defaultOptions	-395	-25.0%	-371	-0.1%

As it turns out, every time I load the page, no less than 168 messages are logged through Drupal\dblog\Logger\DbLog::log. That explains the increased time. There seem to be just 3 different messages being logged:

Notice: Undefined index: per_page in Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter->viewElements() (line 168 of /home/marc/http/public/drupalcore/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php).

Notice: Undefined index: default_mode in Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter->viewElements() (line 167 of /home/marc/http/public/drupalcore/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php).

Notice: Undefined index: form_location in Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter->viewElements() (line 183 of /home/marc/http/public/drupalcore/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php).

Looks like fixing these notices will improve performance even more.