Use #pre_render pattern for entity render caching

If we do this, I'd say that we move only stuff thats slow. If you move °everything*, then modules have nothing to alter after building node an entity display for example. Something to consider anyway.

Per #2151459-2: Enable node render caching.

IIRC, #pre_render is also currently used for "pre-theming reorganizations or the render array":
- reordering according to weights configured in Field UI '"Manage display" screen
- setting #access = FALSE on "extra fields" that are configured to be "hidden"
- nesting by field_groups.module...

If we push the generation of the formatters itself to #pre_render too, we'll need to be cautious about the order of execution.

I've put together a first pass at moving field renderable building of an entity into #pre_render so that the field renderables are not built on a cache hit.

Cold-cache starts are a little worse. Warm-cache starts are a little better. I've performed XHProf runs of cold-cache vs. cold-cache and warm-cached vs. warm-cache.

cold-cache vs. cold-cache

and just drupal_render

warm-cached vs. warm-cache

and just drupal_render

Thanks for starting this!

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -136,6 +137,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
       '#langcode' => $langcode,
+      '#pre_render' => array(array($this, 'entityViewBuilderPreRender')),
     );

Hm, in #2023571: Support preprocessing in EntityViewBuilder, we discussed something like this as being problematic because we are then carrying around the view builder object in the render array, as if that wouldn't already contain enough stuff :)

Not sure how much of a problem it is.

I should explain how I did the performance testing in #4. I set up a minimal installation and the created a content type with every field that core ships with. I created a single node page with content in each field. From that page, I removed the extraneous content from the page.html.twig and html.html.twig files so that only the page_top, page_bottom and content variables were printed. I attempted, as much as possible, to limit the processed content down to the single node.

Hm, in #2023571: Support preprocessing in EntityViewBuilder, we discussed something like this as being problematic because we are then carrying around the view builder object in the render array, as if that wouldn't already contain enough stuff :)

Right, this pattern is not ideal. We could also make entityViewBuilderPreRender a static method, but the method refers to $this, so that isn't an option.

We might be able to build a Closure and pass that with NULL as the invocation object. I know how I'd do this in JavaScript; it's quite simple. I'm not sure how sophisticated Closure support in PHP 5.4 is. I can give it a shot.

Hm. No idea how that closure stuff would work :)

It's a bit ugly, but you could have a a static method that does nothing else but call \Drupal::entityManager()->getViewBuilder($entity_type)->actualMethod().

IMHO doesn't matter that it can't be injected/unit tested, as it's going to be routed through code that will not be unit testable anyway in 8.x and the actual logic still could have unit tests, assuming that there are no function calls left in it.

Berdir, I'm finalizing the patch now, but this is what the Callback stuff looks like. It's working for me locally without issue:

protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
  $that = $this;
  // Create a closure that uses the current execution context object as its
  // invocation object.
  $callback = function ($elements) use ($that) {
    return $that->entityViewBuilderPreRender($elements);
  };

  $return = array(
    '#theme' => $this->entityType,
    "#{$this->entityType}" => $entity,
    '#view_mode' => $view_mode,
    '#langcode' => $langcode,
    '#pre_render' => array($callback),
  );

  // Cache the rendered output if permitted by the view mode and global entity
  // type configuration.
  if ($this->isViewModeCacheable($view_mode) && !$entity->isNew() && $entity->isDefaultRevision() && $this->entityInfo->isRenderCacheable()) {
    $return['#cache'] = array(
      'keys' => array('entity_view', $this->entityType, $entity->id(), $view_mode),
      'granularity' => DRUPAL_CACHE_PER_ROLE,
      'bin' => $this->cacheBin,
      'tags' => array(
        $this->entityType . '_view' => TRUE,
        $this->entityType => array($entity->id()),
      ),
    );
  }

  return $return;
}

Ok, here's the updated patch without the #pre_render callback that requires a reference to the class instance. We use an anonymous function that's bound to an object (the class instance) as a callback.

lol @ $that = $this;
JS you say ? ;-)

[edit: I thought closures couldn't be serialized though ?]

timplunkett mentioned serialization of these callbacks as well and now I fear that I'm missing some bigger issue. Why would we need to serialize a #pre_render function? This information isn't cached and I don't think we need to transfer it in any way.

Well, we do like to serialize/cache all the things, maybe it could happen in a preview form where the entity is then stored in $form_state (I guess that's cloned first).

We already had all sorts of crazy issues with serialization, I wouldn't be surprised if we're hit here as well.

And yes, not sure how this is better than a direct reference, that still has it, just indirectly, no?

@jessebeach: Yeah, sorry, I still easily get confused by the render cache flow. In itself it doesn't serialize render arrays.

'#callback' => array($this, 'methodName') is something that we "try" to avoid on the form side, because $form arrays do get systematically serialized.

Less a problem for non-form render arrays by default, even though :
- as Berdir points, wo do tend to happily serialize lots of things
- it's then a question of setting & encouraging good practices. "Fine in render arrays but problematic in forms" is a bit leaky.

At any rate, agreed with Berdir, the closure approach doesn't seem to bring much over array($this, 'methodName') ?

Posting an intermediate patch so that @benjifisher and I can work on it during the #drupal Global Sprint day.

More refactorings to EntityViewBuilder::viewMultiple().

The latest for benjifisher and me to review before posting a patch for testbot.

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -111,8 +112,9 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
          // Remove previously built content, if exists.
   -      $entity->content = array(
   +      $entity->content += array(
   

   This comment no longer makes sense. We should explain why this is being merged and not assigned.

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -132,18 +134,26 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
   +    $that = $this;
   +    // Create a closure that uses the current execution context object as its
   +    // invocation object.
   +    $callback = function ($elements) use ($that) {
   +      return $that->entityViewBuilderPreRender($elements);
   +    };
   +
   

   This approach is still being debated, but I'm leaving it like this for now.

3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -154,7 +164,52 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +
   +    $this->buildContent(array($entity), array($bundle => $display), $view_mode, $langcode);
   

   Add a comment that this is where we build the entities field renderables.

4. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -154,7 +164,52 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +    $build += $entity->content;
   

   Add a comment about how the view hooks should not be allowed to override the default build information here like #theme. This behavior maintains current 8.x behavior for view hooks.

2. Yeah, I don't think we should do that :) It doesn't actually change anything, we still have the reference to the view builder in there, just one additional layer of indirection. And while serialization a view builder might be bad, this will fatal error, so that's not better :) (Unless we want it to)

The attached patch adds a few comments as suggested in #18. It also removes the default value from the middle parameter of getBuildDefaults().

Testbot, what do you think?

Experimenting with the field-level errors from the test run in #20. The view method is being invoked on different fields compared to 8.x in field_invoke_method. I'm trying to determine what is causing this -- most likely a difference in display and view mode that this patch has introduced.

The test-case I'm working on is a comment preview:

/comment/reply/node/1/comment

The CommentDefaultFormatter is checking the status of the comment, but instead of returning an int, the status is returning Drupal\Core\Field\ConfigFieldItemList as the value of the status property of the field. In fact, all the properties of the field have the Drupal\Core\Field\ConfigFieldItemList as their value.

I can't yet determine where the values of the field are assigned to understand why the status field is getting mis-assigned its value.

Not able to point to a specific place in the patch that would cause this, but it might be a good idea to wait on #2167267: Remove deprecated field_attach_*_view(), that is going to touche those exact same places.

The comment stuff is related to the change in #2151459: Enable node render caching as we've found. As suggested there, instead of the back/forth in the comment preview logic, I would simply clone the commented entity and then change that.

Question: Do we need a new method/something that gives field formatters and other code that would now run too late a chance to add cache keys/tags?

Quickly discussed in IRC, one example of what I'm talking about is comment_entity_view_alter(), which alters the cache key for the pager.

This patch moves that hook into pre_render(), which means that it won't be executed (early enough) anymore. This will need something to handle this, @catch mentioned that it's probably the same hook as we'll need for #2099137: Entity/field access and node grants not taken into account with core cache contexts. I expect that some test failures are related to that.

Re-roll (not sure if I did everything correctly, but it seems be working). Fixed the comment preview stuff.

Now we have a very interesting problem however. And that's a nested #pre_render call that is adding a #post_render element (comment form). Post render thingies are collected after pre render caches are executed, but only on the top level element. Not sure how to fix that...

We might also be seeing more field instance/formatter related failures now that this is actually affecting the render cache. For example the comment form settings.

Looking at #31 and I can see a tantalising thread of something .... that I want to unpick

Looking at \Drupal\book\Tests\BookTest first warning message

Undefined variable: url_targetDrupal\simpletest\WebTestBase->clickLink('Printer-friendly version') Drupal\book\Tests\BookTest->testBookExport() Drupal\simpletest\TestBase->run() simpletest_script_run_one_test('46', 'Drupal\book\Tests\BookTest')

undefined variable is code for an unpatched bug report

notice in clickLink if link does not exist() --- https://drupal.org/node/1452896

So the WebTestBase clickLinks() in Drupal\book\Tests\BookTest are suddenly invalid... hmm

Unfortunately book is undergoing rapid changes recently and the warning message no longer reflect the source code so I am asking for a retest before proceeding further,,,

31: pre-render-2099131-31.patch queued for re-testing.

martin107, it's great to have you looking at this issue with us.

It seems that the links aren't printing on nodes. The missing link passed clickLink is a link added in post_render processing.

<ul class="links inline">
  <li class="book-printer"><a href="/book/export/html/1" title="Show a printer-friendly version of this book page and its sub-pages.">Printer-friendly version</a>
  </li>
</ul>

Which is probably related to the conflict in processing order that Berdir points out in #31:

And that's a nested #pre_render call that is adding a #post_render element (comment form). Post render thingies are collected after pre render caches are executed, but only on the top level element. Not sure how to fix that...

Wim and I discussed this conflict a couple months back and we knew this day of reckoning would come after we enabled entity render caching. So, here we are; it's time to untangle the mess!

To reproduce the links issue, follow these steps:

1. Enable the Book module
2. Create an article; create a new book with the article as the top page
3. Clear the caches
4. Open the article page in an anonymous session (like in incognito mode)
5. Refresh the article in your logged-in session.
6. The links will not be present. They've been cached-out.

Actually, if you refresh the article a second time in your logged-in session, the links appear. bizarre.

We're not processing the <drupal:render-cache-placeholder> on the first pass after a cache clear.

I think I know where to look now.

This patch separates the one location in drupal_render where post_render_cache functions are rolled into two locations: Once at every component in the renderable that will be cached (e.g. isset($elements['#cache'])) and again at those components that are the top of recursive chains e.g. the "nodes" key that contains a list of nodes to render.

The first call to drupal_render_collect_post_render_cache at the discovery of a cacheable component is necessary to retrieve the post render cache callbacks before they become hidden behind cache. The second call to drupal_render_collect_post_render_cache, at the end of drupal_render, is necessary to scoop up any post render cache callbacks that may have been introduced by pre_render functions that build the renderables for child elements.

Let's see what the bot says about it.

I expect test fails, but hopefully the bulk of fails that resulted from post-render elements not being printed to the page will be resolved.

Moved the second call to drupal_render_collect_post_render_cache to before drupal_render_cache_set to get the Comment module tests to pass. I need to look into why this causes to the tests to pass.

Drupal\comment\Tests\CommentPagerTest is exactly the test fail that I expected to happen based on comments #28-#30.

CommentPagerTest should be fixed in some issue, please try this hunk

EDIT filed #2210177: Use getSetting() in comment formatter

Committed the patch referenced in #45.

41: pre-render-2099131-41.patch queued for re-testing.

Posting to see what the bot says. This patch resolves the comment pager test fails and perhaps others.

It introduces hook_entity_defaults_alter. It's still rough. If it resolves more test failures, I'll post another comment describing the changes in great detail and my concerns with them.

Still dirty, but I'm moving along resolving test failures. Once I get everything green, we'll do some performance analysis and decide if the gains are enough to warrant the refactoring that will be necessary to get these changes in for realz

"639 fails", hmmm, 4 steps forward, 635 steps back. I think I know what I did to roll out all these fails. I'll fix it in the morning.

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -272,6 +282,7 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    
        foreach ($entities as $key => $entity) {
   +      $entityType = "{$this->entityTypeId}";
          // Ensure that from now on we are dealing with the proper translation
          // object.
          $entity = $this->entityManager->getTranslationFromContext($entity, $langcode);
   @@ -284,8 +295,7 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   
   @@ -284,8 +295,7 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    
          // Set defaults for #pre_render.
          $build[$key] += $this->getBuildDefaults($entity, $view_mode, $langcode);
   -
   -      \Drupal::moduleHandler()->alter(array('entity_defaults'), $build[$key], $entity, $view_mode, $langcode);
   +      \Drupal::moduleHandler()->alter(array($entityType . '_defaults', 'entity_defaults'), $build[$key], $entity, $view_mode, $langcode);
    
          $build[$key]['#weight'] = $weight++;
   

   $entityType seems unnecessary and strangely initialized, could just as well use $this->entityTypeId directly below?

2. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
   @@ -65,6 +65,22 @@ public function __construct($plugin_id, array $plugin_definition, FieldDefinitio
    
   +  public function getDefaults(FieldItemListInterface $items) {
   +    $info = array();
   +    // Gather cache tags from reference fields.
   +    foreach ($items as $item) {
   +      if (isset($item->format)) {
   +        $info['#cache']['tags']['filter_format'] = $item->format;
   +      }
   +
   +      if (isset($item->entity)) {
   +        $info['#cache']['tags'][$item->entity->getEntityTypeId()][] = $item->entity->id();
   +        $info['#cache']['tags'][$item->entity->getEntityTypeId() . '_view'] = TRUE;
   +      }
   +    }
   +    return $info;
   +  }
   

   Hm, you're just moving this, but ->format should really live in the text formatters and ->entity in EntityReferenceFormatterBase.

   Then you can also make it unconditional.

   The latter is interesting, because you can see that the function there filters out non-accessible references for the user, so wondering if we should only add tags for that, but that would be pretty expensive, as we need to load them there then.

   Also, looks like you're not moving the array initialization above in view() won't that unset the cache tags again there?

Also, looks like you're not moving the array initialization above in view() won't that unset the cache tags again there?

Yes, something like that is happening. I think that's what caused the flood of fails. I'm debugging now.

Noted about format and entity.

Again, very dirty patch. I fully plan to take all these changes into consideration in totality once I've at least gotten the tests to pass. Plus I'm learning these sub-systems as I go, so the changes lack a certain finesse.

Hopefully this patch gets the errors back into the "couple dozen" range.

The interdiff to #58.

sorry for all the noise :/

Another go at a full round of testing.

Reroll

triggering testbot

hmm

#58 - l 138fails, 234 exceptions

#64 969 fail(s), and 62 exception(s).

not sure how much of that is jessebeach's change in #62 or my reroll... I will start looking into my changes and try to find any obvious gotchas.

looking at the diff between the #62 and #64

2 changes A and B

A)

< -      // Gather cache tags from reference fields.
< -      foreach ($items as $item) {
< -        if (isset($item->format)) {
< -          $info['#cache']['tags']['filter_format'] = $item->format;
< -        }
< -
< -        if (isset($item->entity)) {
< -          $info['#cache']['tags'][$item->entity->getEntityTypeId()][] = $item->entity->id();
< -          $info['#cache']['tags'][$item->entity->getEntityTypeId() . '_view'] = TRUE;
< -        }
< -      }
< -
<        $addition[$field_name] = array_merge($info, $elements);
<      }

B)

< -          $build[$key] += $formatter->view($items);
---
> -          $build[$key][$field_name] = $formatter->view($items);

B - seems like the right thing to do .. not sure about A

What about A is setting off alarms for you?

I'm digging into these fails, too. This is such a whack-a-mole game. I fix one test and break 100 more.

Ok A and B are the only changes in reroll.... I did not want to give you a tricky to fix problem when you are analysing a hard problem.
I'm at the end of my day here... I will look at test fails from a different perspective tomorrow..

book is failing again ... your insight of creating a manual test was good ... I will start there.

The spike in errors has to do with the <drupal:render-cache-placeholder /> element not being replaced on the first pass. I introduced this regression in the patch to fix the fails in RenderTest. So, I just need to make RenderTest and BookTest::testBookExport() pass and we should then be making progress again.

Getting RenderTest and BookTest::testBookExport() to pass just about break my brain. I think I tried about 40 different approaches and walked through a debugger for several hours before landing on the following solution:

1. Collect all post_render_cache entries before the theming starts for anything that has a #cache property or the root of a recursive chain call to drupal_render.
2. After theming, make one more effort to collect post_render_cache tags on the root of a recursive chain if the first attempt turned up empty (because its a list of nodes that have pre_render functions that hadn't run yet).

So, let's see if there's a third case now that fails because these two are resolved =D

After a few more hours banging my head on the remaining test failures, I've come to the conclusion that we cannot use #pre_render functions to delay the building of renderables until after a cache-check. The current behavior of drupal post render cache requires that the complete renderable be available to traverse either when a renderable element has a #cache property or when the renderable is the root of a renderable tree. By delaying tree node building until subsequent recursive calls to drupal_render, we subvert the drupal post render cache's collecting of post render renderables.

I have found no way to re-process an element to collection post render cache renderables without duplicating post render information or failing to render a post render cache renderable, thus output the drupal> render element to the DOM.

The only way I can think to solve this is to process the complete build of the view on the first call to drupal_render, producing the same behavior we had before, with the complete build step just slightly delayed until after a cache check.

I've thrown together a rough mockup of the concept and profiled it. Everything but number of function calls improves.

I profiled a normal Article node with tags, an image and a couple comments (one awaiting moderation approval). The performance gains are good enough on first glance to warrant consideration. But I don't want to pursue this strategy without gathering opinions about it.

It's still going to need more work if we decide to move forward. As of now, it pass RenderTest, CommentAdminTest and BookTest: the three tests that I've only ever been able to get 2 of which to pass at any one point in developing on this issue.

The test failures in #73 are the result of too many function calls. We cannot attempt to build out the complete renderable tree in drupal_render because it leeds to too many recursive function calls. Controllers know how to build a renderable; drupal_render is too generic a function to know (at least at the moment) how to build a specific renderable (e.g. block, page, entity, View).

I am completely out of ideas for how to proceed and frankly I'm not convinced that this issue is really critical. The performance gains are theoretical since we can't get a patch to green. And there are other layers of caching in Drupal that alleviate the cost of building a renderable.

I'd like to propose that we keep this issue Critical for now, but remove the beta blocker designation. I'll bring up this issue in Szeged when there will be more brain power available to puzzle it out.

Removing the beta blocker tag.

I am not convinced that this proposed change will give us performance increase that will justify the complexity we'll have to introduce.

Both the original issue summary and the first comment by Moshe concede that the proposed change would be tricky. After spending several weeks on this issue, I'm inclined to agree. I don't think this issue should block a beta release.

A less memory intensive version of #73, inspired by #2215719: Cache tags set by #pre_render callbacks are lost in page cache.

77: pre-render-2099131-77.patch queued for re-testing.

77: pre-render-2099131-77.patch queued for re-testing.

Pulling 8.x and re-rolling, just in case that has anything to do with the login failures.

83: pre-render-2099131-84.patch queued for re-testing.

Catch, can you post an interdiff for #90?

1. +++ b/core/modules/node/lib/Drupal/node/NodeViewBuilder.php
   @@ -34,16 +34,25 @@
   +      $callback = '\Drupal\node\NodeViewBuilder::renderLinks';
   +      $token = \Drupal\Component\Utility\Crypt::randomBytesBase64(55);
   +      $context = array(
   +        'node_entity_id' => $entity->id(),
   +        'view_mode' => $view_mode,
   +        'langcode' => $langcode,
   +        'in_preview' => !empty($entity->in_preview),
   +        'token' => $token,
   +      );
   +
          $entity->content['links'] = array(
   -        '#type' => 'render_cache_placeholder',
   -        '#callback' => '\Drupal\node\NodeViewBuilder::renderLinks',
   -        '#context' => array(
   -          'node_entity_id' => $entity->id(),
   -          'view_mode' => $view_mode,
   -          'langcode' => $langcode,
   -          'in_preview' => !empty($entity->in_preview),
   +        '#post_render_cache' => array(
   +          $callback => array(
   +            $context,
   +          ),
            ),
   -      );
   +        '#markup' => drupal_render_cache_generate_placeholder($callback, $context, $context['token']),
   +      ); ¶
   

   So an element that will be replaced in post render cache is responsible for generating a placeholder as its #markup.

2. +++ b/core/modules/node/lib/Drupal/node/NodeViewBuilder.php
   @@ -92,7 +101,10 @@
   +  public static function renderLinks(array $element, array $context) {
   +    $callback = '\Drupal\node\NodeViewBuilder::renderLinks';
   +    $placeholder = drupal_render_cache_generate_placeholder($callback, $context, $context['token']);
   +
        $links = array(
          '#theme' => 'links__node',
          '#pre_render' => array('drupal_pre_render_links'),
   @@ -110,8 +122,10 @@
   
   @@ -110,8 +122,10 @@
          );
          \Drupal::moduleHandler()->alter('node_links', $links, $entity, $hook_context);
        }
   +    $replace = drupal_render($links);
   +    $element['#markup'] = str_replace($placeholder, $replace, $element['#markup']);
    
   -    return $links;
   +    return $element;
      }
   

   And then responsible for replacing the placeholder after caches have been set.

Having tried 101 ways to get pre render to work within the recursive drupal render pipeline, I'm encouraged by this approach. It does place the onus of setup and processing on the developer...BUT, post render cache items will be rare and even the established system is sufficiently complex to grok. Good docs can support this more manual approach.

Added the related issue #2227383: EntityViewBuilderInterface::view() should accept an EntityViewDisplay, not only a view mode ID to deal with wonky comment preview code.

So, big picture, patches #87 through #103 do the following:

1. Remove the render_cache_placeholder type.
2. Move placeholder rendering into build method of an element (entity/field/etc).
3. Move post render element building/rendering and placeholder replacement into the #post_render_cache callback registered on the initial build array of an element.
4. Alters and deletes some tests. I'm going back into see if these should be restored or reinstated, especially testDrupalRenderChildrenPostRenderCache.

Overall, I like the approach.

WimLeers, we definitely need your input here.

AWESOME work, Jesse & catch! I can't quite believe you've actually gotten it green! :)

Now let's get all tests undeleted and green again :)

I focused specifically on the #post_render_cache stuff, but I also reviewed the other changes at the same time.

#90/#92:

+++ b/core/includes/common.inc
@@ -3954,7 +3932,7 @@
-  if (!$is_recursive_call) {
+  if (!$is_recursive_call || isset($elements['#cache'])) {

Why?

This will "infect" a parent element that has #cache set.

(Also see later, in the review of #103.)

#98:

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -211,7 +211,7 @@ public static function renderForm(array $element, array $context) {
-    $replace = drupal_render($form);
+    $replace = drupal_render($form, TRUE);

Great find!

#103:

1. +++ b/core/includes/common.inc
   @@ -3803,16 +3803,6 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   -  // Collect all #post_render_cache callbacks associated with this element when:
   -  // - about to store this element in the render cache, or when;
   -  // - about to apply #post_render_cache callbacks.
   

   Why not keep this comment? It's still accurate?

2. +++ b/core/includes/common.inc
   @@ -3916,6 +3906,12 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
   +  if (!$is_recursive_call || isset($elements['#cache'])) {
   

   This is where that comment belongs.

   More importantly, this move introduces a subtle change: #post_render_cache callbacks are collected after #post_render</code callbacks run. However, that shouldn't matter: <code>#post_render callbacks work on the final output, and cannot introduce additional #post_render_cache callbacks. So it's fine.

   I just want us to be aware that this is the case.

3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -91,8 +93,10 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   -      // Remove previously built content, if exists.
   -      $entity->content = array(
   +      if (empty($entity->content)) {
   +        $entity->content = array();
   +      }
   +      $entity->content += array(
            '#view_mode' => $view_mode,
          );
   

   Why is this change necessary?

4. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -119,7 +123,7 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
   +        $entity->content = NestedArray::mergeDeepArray(array($entity->content, array('#view_mode' => $view_mode,), $build[$id],), TRUE);
   

   There's a few trailing commas here that shouldn't be there :)

5. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -138,8 +142,12 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
   +    $this->moduleHandler->alter('entity_view_mode', $view_mode, $entity, $context);
   

   $context is uninitialized, but should contain the langcode.

   Since there are no test fails, we have no test coverage for this hook…

   I'm also not sure there's much value in this. And it's in the critical path for rendering entities. So maybe we should remove it?

6. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -175,6 +192,61 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +    $context = array('langcode' => $langcode);
   

   This is no longer necessary here, can/should be moved up.

7. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -175,6 +192,61 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +    // Build field renderables.
   +    $entity->content = $elements;
   +    $this->buildContent(array($entity->id() => $entity), array($bundle => $display), $view_mode, $langcode);
   ...
   +    $build = $entity->content;
   ...
   +    unset($entity->content);
   ...
   +    $this->alterBuild($build, $entity, $display, $view_mode, $langcode);
   

   This whole $entity->content thing seems really bizarre. First we fill $entity->content, then we call buildContent() on it which also has to do strange things with it. Then we futz it back to $build, ***unset*** it, and invoke alterBuild() with $build.

   AFAICT we can just always use $build? :)

8. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -209,58 +281,26 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   +      //$build[$key] = $entity->content;
   

   Dead code.

9. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
   @@ -66,6 +66,27 @@ public function __construct($plugin_id, array $plugin_definition, FieldDefinitio
   +  public function getDefaults(FieldItemListInterface $items) {
   

   It feels like this should be getCacheTags(). There's no reason for this to be in the form of a render array: it's just unnecessary wrapping; it makes it impossible to use for other types of caching than render caching.

   FormatterInterface should probably extend CacheableInterface.

10. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedViewBuilder.php
    @@ -18,7 +18,7 @@ class FeedViewBuilder extends EntityViewBuilder {
    -  protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
    +  protected function getBuildDefaults(EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
    
    +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemViewBuilder.php
    @@ -18,7 +18,7 @@ class ItemViewBuilder extends EntityViewBuilder {
    -  protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
    +  protected function getBuildDefaults(EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
    

    Why set these defaults? It used to match EntityViewBuilderInterface::getBuildDefaults().

11. +++ b/core/modules/comment/lib/Drupal/comment/CommentViewBuilder.php
    @@ -123,17 +123,24 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    +      $token = \Drupal\Component\Utility\Crypt::randomBytesBase64(55);
    

    Let's put this in a helper function? :)

12. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewDisplay.php
    @@ -230,7 +230,13 @@ public function buildMultiple(array $entities) {
    +          if (!isset($build[$key][$field_name])) {
    ...
    +          $build[$key][$field_name] += (!empty($built_field[$field_name])) ? $built_field[$field_name] : array();
    

    I'm confused by this: I don't understand why we're setting $entity->content. But apparently it may cause some fields to be rendered already, and others not just yet. Especially because we're still generating each formatted field and the third quoted line is just adding those keys that aren't already rendered… which I don't understand at all.

13. +++ b/core/modules/node/tests/modules/node_test/node_test.module
    @@ -21,7 +21,6 @@ function node_test_node_view(NodeInterface $node, EntityViewDisplayInterface $di
    -    $node->rss_namespaces['xmlns:drupaltest'] = 'http://example.com/test-namespace';
    
    @@ -39,6 +38,15 @@ function node_test_node_view(NodeInterface $node, EntityViewDisplayInterface $di
    +function node_test_node_defaults_alter(array &$build, NodeInterface &$node, $view_mode = 'full', $langcode = NULL) {
    +  if ($view_mode == 'rss') {
    +    $node->rss_namespaces['xmlns:drupaltest'] = 'http://example.com/test-namespace';
    

    This feels unrelated?

14. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderTest.php
    @@ -471,7 +471,6 @@ function testDrupalRenderPostRenderCache() {
    -    $this->assertTrue(!isset($element['#context_test']), '#context_test is not set: impossible to modify $element itself, only possible to modify its #markup and #attached properties.');
    

    Why remove this? It implies that it is now possible to do this…?

15. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderTest.php
    @@ -774,235 +764,6 @@ function testDrupalRenderChildrenPostRenderCache() {
    -  function testDrupalRenderRenderCachePlaceholder() {
    

    We must keep this one but rename it to testDrupalPostRenderCacheWithPlaceholder. This test will no longer use #type = render_cache_placeholder, but instead it will have to generate its own placeholder (using the helper method).

16. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderTest.php
    @@ -774,235 +764,6 @@ function testDrupalRenderChildrenPostRenderCache() {
    -  function testDrupalRenderChildElementRenderCachePlaceholder() {
    

    We most definitely cannot remove this. It was added to cover an important case that was breaking node render caching.

    See #2151459-37: Enable node render caching & #2151459-38: Enable node render caching.

17. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
    @@ -491,19 +491,22 @@ function testLanguageFallback() {
    +    // @todo, this assertion requires rendering.
    +    //$this->assertIdentical($entity2, $translation, 'When the entity has no translation no fallback is applied.');
    ...
    +    // @todo, this assertion requires rendering.
    +    //$this->assertEqual($build['label']['#markup'], $values[$current_langcode]['name'], 'By default the entity is rendered in the current language.');
    ...
    +      // @todo, this assertion requires rendering.
    +      //$this->assertEqual($build['label']['#markup'], $values[$expected]['name'], 'The entity is rendered in the expected language.');
    

    We need to get these back now that the patch is green :)

I've put the following tests back:

testDrupalRenderRenderCachePlaceholder
testDrupalRenderChildElementRenderCachePlaceholder

#108:

2. Heh :)
7. Ok. I just fear that by not cleaning it up now, we break things in subtle ways.
9. Yeah, good point. Cache tag bubbling should already take care of this. I think this is a remnant of Jesse's earlier attempts to work around the problems she was having.
10. Confirmed — it's fine — sorry for the noise!
12. Sure, we can do clean-up in another issue, but this code did not exist before…
14. Thanks to the move you did (point 2), that's indeed true now, but before you could set #theme for example, or unset #cache or whatnot.

+++ b/core/lib/Drupal/Core/Field/FormatterBase.php
@@ -91,22 +91,9 @@ public function view(FieldItemListInterface $items) {
-        '#cache' => array('tags' => array())
       );
 
-      // Gather cache tags from reference fields.
-      foreach ($items as $item) {
-        if (isset($item->format)) {
-          $info['#cache']['tags']['filter_format'] = $item->format;
-        }
-
-        if (isset($item->entity)) {
-          $info['#cache']['tags'][$item->entity->getEntityTypeId()][] = $item->entity->id();
-          $info['#cache']['tags'][$item->entity->getEntityTypeId() . '_view'] = TRUE;
-        }
-      }
-
-      $addition = array_merge($info, $elements);
+      $addition[$field_name] = array_merge($info, $elements);

Woah, dunno when that code got added, but happy to see it go away :-)

$addition[$field_name] looks wrong though, keying the render array by field name is supposed to be the job of the formatter's caller now.

For the record, I've been wondering about why TF we bother with $entity->content as well, and would love to see it die in flames too - here or in a separate issue :-)

#117: It's from this patch, it's not in core :)

Maybe I'll try to get the $entity->content -> $build thing done. I want to see it die miserably.

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -175,6 +184,60 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +   * This function is assigned as a #build callback in
   +   * \Drupal\Core\Entity\EntityViewBuilder::getBuildDefaults().
   +  public function entityViewBuilderBuildView(array $elements) {
   

   Comment is wrong. It's assigned as a #pre_render. Given that, why not name the function preRender()? I don't think prefixing the method name with class name as buying us anything.

2. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
   @@ -91,22 +91,9 @@ public function view(FieldItemListInterface $items) {
   -      $addition = array_merge($info, $elements);
   +      $addition[$field_name] = array_merge($info, $elements);
   

   Per #117, this should not be necessary.

Maybe I'll try to get the $entity->content -> $build thing done. I want to see it die miserably.

Per #1977266-43: Fix ContentEntityBase::__get() to not return by reference, I'd also love to see us remove $entity->content entirely. I think that will require changing the API of hook_entity_view() and hook_ENTITY_TYPE_view() though. I don't see that as in scope for this issue, but if others think it is, I'll be ok with that. Either way, a huge +1 to doing it, whether in this issue or not.

I've been working in #2228043, continuing where catch left off, getting rid of $entity->content. It's now green :)

This identical to #2228043-36: Helper for #2099131, and should therefore also be green. I'll be working on a follow-up reroll that also cleans up some loose ends.

Note: the attached interdiff is against #2228043-23: Helper for #2099131, i.e. catch's work since #123. I've also created an interdiff for his work, so that only reading the interdiffs allows you to see all changes.

• AFAICT all the <entity type>BuildContentTest tests are now obsolete, so deleting those.
• s/$replace/$markup/
• Created https://drupal.org/node/2238835 for the @todo that catch added.
• The update to CommentCacheTagsTest caused it to verify that a nonsensical filter_format: cache tag was present. The cause: apparently one can create a comment with just 'comment_body' => 'The comment text.' or even 'comment_body' => array('value' => 'The comment text.'), rather than the proper 'comment_body' => array('value' => 'The comment text.' , 'format' => 'plain_text'). In the first two examples, $item->format === NULL, which causes filter_format: to be generated as a cache tag. i.e. either this is a bug in the Comment entity, or it's a explicitly allowed yet bizarre feature of it. In any case, I fixed that.
• Replaced the last few direct calls to Crypt::randomBytesBase64() with calls to drupal_render_cache_generate_token().
• Some docs fixes.

One more reroll coming up that makes EntityViewBuilder quite a bit less crufty, but might break things. Then I'll unassign.

As said, making EntityViewBuilder less crufty.

Also moving back RenderTest::testDrupalRenderRenderCachePlaceholder() and RenderTest::testDrupalRenderChildElementRenderCachePlaceholder() to their original locations, their unnecessary relocation within the same file makes the patch more difficult to review. (And in doing so, I found two lingering debug() calls :)) That reduced the patch size by a nice 18 KB :)

Review

1. WebTestBase::drupalBuildView() is too ambiguous. What about ::drupalViewEntity()?
2. This bit of code in CommentViewBuilder:

     public function buildContent(array &$build, array $entities, array $displays, $view_mode, $langcode = NULL) {
       // Pre-load associated users into cache to leverage multiple loading.
       $uids = array();
       foreach ($entities as $entity) {
         $uids[] = $entity->getOwnerId();
       }
       $this->entityManager->getStorage('user')->loadMultiple(array_unique($uids));
   

   has become obsolete now. Because CommentViewBuilder uses EntityViewBuilder::viewMultiple(), the default render strategy kicks in, which is to use #pre_render. If we use #pre_render, then buildContent() is always called on a per-entity basis. Now, because this code will run less often (right now it still runs on every render cache hit!), it may not be necessary anymore.
   If we are certain that we want to keep this, because comments are usually rendered many-at-a-time, then we could optimize CommentViewBuilder::viewMultiple() for that: instead of a #pre_render callback per entity (i.e. per comment), we could make a single #pre_render callback for all comments, which then could again call buildContent() with all comments at once. This would make a cache miss faster.

3. All the changes in the area of "while commenting or previewing a comment, show the parent entity", do we really need them? IIRC Berdir showed how we can make it work with today's API, even without #2227383: EntityViewBuilderInterface::view() should accept an EntityViewDisplay, not only a view mode ID, even though that would make it much cleaner.

Taking a look at this as part of the NYCCamp sprint.

Took some time to look this over. It makes sense. I have to say that the use case where you need to add an uncacheable element to an entity is very confusing, but I don't think improving DX for that should block this issue. One nitpick:

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -208,59 +262,19 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+      $entityType = "{$this->entityTypeId}";

Should probably be $this->entityTypeId.

WebTestBase::drupalBuildView() is too ambiguous. What about ::drupalViewEntity()?

I'd like leave the word "build" in the method name, since it is building. So I've renamed it to drupalBuildEntityView. This method was also missing docs. I added them.

I'm still mentally processing Review comments #2 and #3 from comment #130.

If we are certain that we want to keep this, because comments are usually rendered many-at-a-time, then we could optimize CommentViewBuilder::viewMultiple() for that: instead of a #pre_render callback per entity (i.e. per comment), we could make a single #pre_render callback for all comments, which then could again call buildContent() with all comments at once. This would make a cache miss faster.

Here are some numbers to put behind this issue.

TESTING CONTEXT: standard article node with an image and tags. The node has 30 comments. xdebug is turned off and xhprof is running with the single flag XHPROF_FLAGS_MEMORY.

I ran a general performance analysis for this patch, warm-vs-warm cache; I took five runs of each condition and then selected the fastest of those five for each. I ended up with a 77ms overall improvement:

I then compared cold-vs-cold cache, 8.x vs. 8.x-patched using the same 5 runs methods. Focusing in on the Drupal\comment\CommentViewBuilder::buildContent method, we find a 19ms overall deterioration for the request.

The cold cache decrease in performance of 19ms is only a 0.5% increase in total request time because the request itself, on a cold cache, takes 6.3 times longer to process than a warm cache request. So 19ms is far less of a increase in total processing time on the slow request than the 77ms ends up being in a relatively quick request time. So, we can compare the percentages here. We need to compare the milliseconds.

Personally, I'm willing to hive off the refactor of CommentViewBuilder's implementation of #pre_render to a major and take the 19ms hit on a cold cache so that we get the bigger refactor in now, establish the pattern in core and get a bigger "fast by default" win.

re #130:3, The solution to the hack in comment module to render a the entity that a comment preview is associated with is to simply not render the entity. The code that did this is removed in the patch:

-  else {
-    // The comment field output includes rendering the parent entity of the
-    // thread to which the comment is a reply. The rendered entity output
-    // includes the comment reply form, which contains the comment preview and
-    // therefore the rendered parent entity. This results in an infinite loop of
-    // parent entity output rendering the comment form and the comment form
-    // rendering the parent entity. To prevent this infinite loop we temporarily
-    // set the value of the comment field on the rendered entity to hidden
-    // before calling entity_view(). That way when the output of the commented
-    // entity is rendered, it excludes the comment field output. As objects are
-    // always addressed by reference we ensure changes are not lost by setting
-    // the value back to its original state after the call to entity_view().
-    $field_name = $comment->getFieldName();
-    $original_status = $entity->get($field_name)->status;
-    $entity->get($field_name)->status = CommentItemInterface::HIDDEN;
-    $build = entity_view($entity, 'full');
-    $entity->get($field_name)->status = $original_status;
-  }

I am fine with this change. This is a minimally viable solution and we have an issue to improve the UI: #2227383: EntityViewBuilderInterface::view() should accept an EntityViewDisplay, not only a view mode ID. Any improvement to this view will be introduced as an optional view mode, so existing sites will have the chance to enhance the comment preview page by selecting a new view mode for it.

rbayliss and I did a thorough review of the code in NYCCamp.

So in my opinion, the only blocking issue here is the performance regression for cold cache requests on pages that have numerous comments. In the service of getting a beta blocker resolved, I suggest we note the regression in an issue, take the overall improvement now, and get the architectural improvement in place.

I'll start writing up the change notice!

It looks like the latest patch might have been based off #122 instead of #123? Either way I got the comment preview issue 'fixed' in #123 without a UI regression via berdir's suggested change from #30 (which we'd overlooked until he pointed it out again in Szeged). See the interdiff https://drupal.org/files/issues/interdiff_3373.txt- hopefully just applying that interdiff will be enough.

I'll sort this out and post the patch soon.

What it came down to was 1. just take the hit on misses 2. Do some multiple logic on all items, even if they're all cache hits - which means a performance regression on hits 3. Nasty hack to enable running the multiple logic on cache miss items only. It's definitely a problem though so good if we can resolve it.

This is what I ran into last night...just when I thought I had it! I'm experimenting with some ideas to do multiple item building in the pre_render/cache-miss cycle.

This is the work since #133 with the interdiff from #123 applied.

I'm continuing to work on the multiple/single building issue.

There's also a refactor of an anonymous function into a method.

Setting to Needs Work since that more accurately represents the state of the patch.

I think I have multiple build working along side single build. I ran RenderTest locally, which is, historically, a good canary.

Please review the approach. I've got some code duplication and docs to write if this comes back green or close to it.

Weird, I merged in changes from 8.x locally and didn't run into any conflicts. No interdiff; chasing HEAD.

This should sort the failing tests.

The Drop is moving fast today. Chasing HEAD, no interdiff.

Code cleanup and consolidation.

Some performance numbers.

First case: 8.x to 8.x-patched, warm cache to warm cache, article node with 10 multi-value fields added (various default fields). The article has 50 comments on it.

We get a 50ms gain in this case.

Second case: 8.x to 8.x-patched, warm cache to warm cache, article node with 10 multi-value fields added (various default fields).

The overall gain is 44ms, which is consistent with the first case, given that this node does not have comments to render, so it should built a little faster.

Third case: 8.x to 8.x-patched, cold cache to cold cache, article node with 10 multi-value fields added (various default fields). The article has 50 comments on it.

In this case, we're specifically looking at CommentViewBuilder::buildContent(), which had been building comment entities one at a time (rather than in multiples) in #138 and before. Now entities are build in multiples, when possible, in the new pre render functions.

We get a speed up in processing time for the cost of a little more memory usage. There is now no regression in time vs. 8.x HEAD.

#147 came back green. All of Wim's concerns from #130 have been addressed.

The latest patch needs a review from someone who isn't me, but as far as I'm concerned, this patch is ready to go. It gives us a huge performance win.

Assigning to catch.

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -139,20 +137,50 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
      protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
   -    $return = array(
   +    // Allow modules to change the view mode.
   +    $context = array('langcode' => $langcode);
   +    $this->moduleHandler->alter('entity_view_mode', $view_mode, $entity, $context);
   +
   (...)
   +
   +    // Collect cache defaults for this entity.
   +    $build = $this->getBuildCacheDefaults($build, $entity, $view_mode, $langcode);
   +
   

   So getBuildDefaults() is called with a $view_mode, alters it first thing, and calls getBuildCacheDefaults() with the altered $view_mode. Feels potentially confusing, plus altering the view mode doesn't sound like something you'd expect from a method named getBuildDefaults().

   I'd think the alter would be better off done by the caller of getBuildDefaults() instead - i.e. the foreach($entities) loop in viewMultiple().

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -165,11 +193,155 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +  public function entityViewBuilderBuildSingle(array $build) {
   ...
   +  public function entityViewBuilderBuildMultiple(array $build) {
   

   Sorry if this has been discussed earlier, but EntityViewBuilder::entityViewBuilderFoo() ?
   Why would two random methods in the class be prefixed by the class name itself ?

   Also: "single" cannot be treated as a degenerate case of "multiple" anymore ? It's a bit sad to have to maintain those two separate code paths :-/

   Also: nitpick - methods order in the class does not really reflect code execution, makes it a bit hard to get a real mental view of the process. Reordering might make the reviews harder though, so maybe a followup ?

3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -165,11 +193,155 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +    // Assign the weights configured in the display.
   +    // @todo: Once https://drupal.org/node/1875974 provides the missing API,
   +    //   only do it for 'extra fields', since other components have been taken
   +    //   care of in EntityViewDisplay::buildMultiple().
   +    foreach ($display->getComponents() as $name => $options) {
   +      if (isset($build[$name])) {
   +        $build['#weight'] = $options['weight'];
   +      }
   +    }
   +
   

   Looks out of place in a method named callViewHooks() :-)
   (if there was a way to re-unify the single / multiple code paths, we wouldn't have to find a name for the method that does the shared bits ?)

4. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -165,11 +193,155 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +    $view_hook = "{$this->entityTypeId}_view";
   

   unused, says PHPstorm.

5. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -165,11 +193,155 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +    foreach($children as $key) {
   

   missing space after foreach :-)

6. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -165,11 +193,155 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   +        // Remove the single item pre_render function from the multiple build items.
   +        $pre_render = $build[$key]['#pre_render'] ?: array();
   +        foreach ($pre_render as $index => $callable) {
   +          if (is_array($callable) && $callable[1] === 'entityViewBuilderBuildSingle') {
   +            unset($build[$key]['#pre_render'][$index]);
   

   Woah, that doesn't really help clarifying the code flow :-)

7. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -206,59 +378,23 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   +      // Set render defaults.
   +      $build[$key] = $this->getBuildDefaults($entity, $view_mode, $langcode);
   +      $entityType = $this->entityTypeId;
   +      \Drupal::moduleHandler()->alter(array($entityType . '_defaults', 'entity_defaults'), $build[$key], $entity, $view_mode, $langcode);
   

   So that's basically all viewMultiple() does now - "call getBuildDefaults(), that'it, done until #pre_render if cache miss"
   In that case, the method name is now a bit misleading. it's not about "defaults" anymore, it's the actual, "final" (before #pre_render) build.

   Similarly, buildContent(), that does the actual rendering at #pre_render time, is now misleading too.

   It would help understanding the new (somewhat complex :-p) flow if method names clearly relfected at which step they kick in.

   (hm, this being said, EntityViewDisplay::buildMultiple() is now kicked out of the 'build" phase into the "pre render" phase - a bit unfortunate - well, let's say at least method names within EVB could be internally consistent with the new split ?)

8. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
   @@ -90,22 +90,9 @@ public function view(FieldItemListInterface $items) {
   -      // Gather cache tags from reference fields.
   -      foreach ($items as $item) {
   -        if (isset($item->format)) {
   -          $info['#cache']['tags']['filter_format'] = $item->format;
   -        }
   -
   -        if (isset($item->entity)) {
   -          $info['#cache']['tags'][$item->entity->getEntityTypeId()][] = $item->entity->id();
   -          $info['#cache']['tags'][$item->entity->getEntityTypeId() . '_view'] = TRUE;
   -        }
   -      }
   -
   -      $addition = array_merge($info, $elements);
   +      $addition[$field_name] = array_merge($info, $elements);
   

   Aw THANK YOU :-)

9. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedViewBuilder.php
   @@ -18,7 +18,7 @@ class FeedViewBuilder extends EntityViewBuilder {
   -  protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
   +  protected function getBuildDefaults(EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
   
   +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemViewBuilder.php
   @@ -18,7 +18,7 @@ class ItemViewBuilder extends EntityViewBuilder {
   -  protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
   +  protected function getBuildDefaults(EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
   

   Not sure why those changes are needed ?
   getBuildDefaults() is an internal, protected method, so calling code should take care of providing defaults ?

10. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewDisplay.php
    @@ -230,7 +230,13 @@ public function buildMultiple(array $entities) {
    -          $build[$key][$field_name] = $formatter->view($items);
    +          if (!isset($build[$key][$field_name])) {
    +            $build[$key][$field_name] = array();
    +          }
    +          // @todo, maybe just return the contents of view without a wrapper and the subsequent need to reference
    +          // a $field_name index?
    +          $built_field = $formatter->view($items);
    +          $build[$key][$field_name] += (!empty($built_field[$field_name])) ? $built_field[$field_name] : array();
    

    I don't get that change.

    In current HEAD, $formatter->view() returns a build array that is *not* wrapped in a $field_name key, it's EVDisplay::buildMultiple that sets $build[$key][$field_name] = the result of the formatter.

    So, not sure why we're using $build_field[$field_name], nor why the (isset($build[$key][$field_name]) check is needed (I can't see why it would be set) ?

11. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -365,16 +365,23 @@ protected function getFieldDefinitions() {
    -      // The display only cares about fields that specify display options.
    -      // Discard base fields that are not rendered through formatters / widgets.
    -      $display_context = $this->displayContext;
    -      $this->fieldDefinitions = array_filter($definitions, function (FieldDefinitionInterface $definition) use ($display_context) {
    -        return $definition->getDisplayOptions($display_context);
    -      });
    +      $this->fieldDefinitions = array_filter($definitions, array($this, 'fieldHasDisplayOptions'));
    ...
    +  private function fieldHasDisplayOptions(FieldDefinitionInterface $definition) {
    

    Hmm - matter of taste, but I find the current shape easier to parse :-) We get inline anonymous functions in PHP now, why not use them in straightforward cases like this ?
    At any rate, this is not really related ?

12. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldFormatter/EntityReferenceIdFormatter.php
    @@ -35,7 +35,15 @@ public function viewElements(FieldItemListInterface $items) {
    -        $elements[$delta] = array('#markup' => check_plain($item->target_id));
    +        $elements[$delta] = array(
    +          '#markup' => check_plain($item->target_id),
    +          '#cache' => array(
    +            'tags' => array(
    +              $referenced_entity->getEntityTypeID() => $referenced_entity->id(),
    +              $referenced_entity->getEntityTypeID() . '_view' => TRUE,
    +            ),
    +          ),
    +        );
    

    Is that really needed ? This prints a numeric ID, it won't change even if the entity itself changes ?

    (aw - is that for the case where the referenced entity gets deleted ? Maybe worth a comment then ?)

yched, great review! I'm putting together a response to your questions/points.

catch, #148, cases one and two are both warm-cache tests. The multiple build #pre_render is only called when EVB::viewMultiple() is called outside of a call to EVB::view(). The #pre_render key is dropped here on a single entity view call.

/**
 * {@inheritdoc}
 */
public function view(EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
  $buildList = $this->viewMultiple(array($entity), $view_mode, $langcode);
  return $buildList[0];
}

I'll explain more in my forthcoming response to yched's review.

I understand it's only called for listings, but isn't called both when there's both a warm and cold cache?

I can't find a case in core where we render a list of things that isn't also a View. And Views renders lists of things on a row-level, meaning as singles. For comments, those get cached, as a field, on their entity, so we don't build them again after the first time.

Yes, we might have a situation where entities are passed, as a set, to EVB:viewMultiple() and each individually item might have a cache entry. In that case, we'd end up building each individual item (same as 8.x HEAD). The single-item pre_render is removed if it's built in the multiple-item pre_render, so we never build twice. In this case (that I can't reproduce in core), we'd end up building the renderable and then getting the cache entry...same as 8.x now. So, my patch from #138 optimizes for nodes with long lists of comments, eliminating the regression for comment building. BUT, you're right, it does open up the possibility for a performance regression if a developer renders a list of entities outside of a View. But I can't think of anywhere in core where we do this that I can use as a test case!

Maybe we could optimize at the multiple level with a cache entry that lists all of the entity IDs in the list? And if one of them gets invalidated, we can build that one again, but skip the rest?

Yes, we might have a situation where entities are passed, as a set, to EVB:viewMultiple() and each individually item might have a cache entry. In that case, we'd end up building each individual item (same as 8.x HEAD).

catch, alternatively, I can move this single/multiple pre_render strategy to CommentViewBuilder and restore the single pre_render strategy to EntityViewBuilder. That'll give us same gains pre-#138 and resolve the multiple comment building regression noted in #130. These strategies are deep enough in the builder classes that we can continue to optimize post release. And what I'd like to see in this issue is that we lock in a solid win.