Problem/Motivation

Currently entity render caching sets #cache on the render array, but this only allows drupal_render() to cache the actual process of getting the HTML to a string.

We can also use #pre_render and defer the actual building of the bulk of the render array to cache misses as well.

The build and rendering process looks like this (in pseudo UML).

The entity build and render flow. Starts at a route controller. Flows to EntityViewBuilder then drupal_render

Note how the building of a renderable occurs before drupal_render() is invoked. This means that the cost of building an entity and its fields is incurred before the cache check occurs. Regardless of whether a cached entry is returned and rendering is avoided, the cost of building the entity and its fields is not avoided.

Proposed resolution

This issue proposes to move the costly building of an entity and its fields to after the cache check. To do this, we will undertake the building in a #pre_render callback on the entity. The flow looks a bit like this (again in pseudo UML).

The proposed entity build and render flow. Starts at a route controller. Flows to EntityViewBuilder which postpones building in a pre render function then drupal_render is called, cache is checked and if no cache entry exists, the pre render function is called and the entity is built before rendering continues.

User interface changes

None.

API changes

Ideally, module developers will not notice this subtle shift in when the view and build hooks are invoked. To a developer, the build and render flow will appear to be the same.

This is not true for entities that override the EntityViewBuilder::view() and EntityViewBuilder::viewMultiple() methods. These methods, when the parent method is invoked, will return a minimal set of properties for the entity that include view mode, language and caching information. Entity classes should only override these methods in order to augment the information returned by EntityViewBuilder or to replace the pre render optimization strategy. Otherwise, the entity classes may provide an additional pre render function that will be run after the default EntityViewBuilder::buildEntity and/or EntityViewBuilder::buildEntityMultiple have run, providing the full entity (and its fields) for manipulation. Even better is to just implement the view and build hook and alter methods for specific changes in the build process.

Once the result of rendering is cached, the EntityViewBuilder::buildEntity and EntityViewBuilder::buildEntityMultiple code paths will not be run again until the cache for the entity is invalidated.

The signatures of the following hooks will change:

  • hook_ENTITY_TYPE_view
  • hook_entity_view

Before

function hook_entity_view(\Drupal\Core\Entity\Entity $entity, \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display, $view_mode, $langcode) {}

After

function hook_entity_view(array &$build, \Drupal\Core\Entity\Entity $entity, \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display, $view_mode, $langcode) {}

Fields that reference entities; caching considerations

In order to collect cache tags from an entity, that entity must be run through a View Builder or rendered. In the case of a field that references another entity, such and the entity_reference field, the formatter's viewElements method must render the referenced entity in place in order to make its caching information available to the referencing entity. This recursive building would have happened before the changes introduced in this change notice's patch (#2099131). Looking at the viewElements method of the entity_reference EntityReferenceEntityFormatter plugin, we see the entity build and render here:

$referenced_entity_build = entity_view($item->entity, $view_mode, $item->getLangcode());
drupal_render($referenced_entity_build, TRUE);
$elements[$delta] = $referenced_entity_build;

The CommentDefaultFormatter is another example of a field plugin that must render its referenced entities in order to bubble up cache tag information to the parent entity.

diff --stat test coverage changes

.../Tests/CustomBlockBuildContentTest.php          |  41 -----
.../Tests/CustomBlockCacheTagsTest.php             |  15 +-
.../Drupal/comment/Tests/CommentCacheTagsTest.php  |  17 +-
.../comment/Tests/CommentContentRebuildTest.php    |  46 -----
.../Tests/CommentDefaultFormatterCacheTagsTest.php | 124 ++++++++++++++
.../Drupal/datetime/Tests/DateTimeFieldTest.php    |   4 +-
.../Tests/EntityReferenceFormatterTest.php         | 117 +++++++++++--
.../image/Tests/ImageFieldDefaultImagesTest.php    |  16 +-
.../Drupal/image/Tests/ImageThemeFunctionTest.php  |  25 ++-
.../lib/Drupal/node/Tests/NodeBuildContentTest.php |  38 -----
.../lib/Drupal/node/Tests/NodeCacheTagsTest.php    |   2 +
.../node/Tests/NodeEntityViewModeAlterTest.php     |   2 +-
.../lib/Drupal/node/Tests/NodeRSSContentTest.php   |   4 +-
.../lib/Drupal/node/Tests/SummaryLengthTest.php    |   5 +-
.../lib/Drupal/simpletest/WebTestBase.php          |  48 ++++++
.../lib/Drupal/system/Tests/Common/RenderTest.php  |  68 ++++----
.../Tests/Entity/EntityCacheTagsTestBase.php       |  20 ++-
.../system/Tests/Entity/EntityTranslationTest.php  |   9 +-
.../Entity/EntityWithUriCacheTagsTestBase.php      |  15 --
.../system/Tests/Theme/TwigDebugMarkupTest.php     |   9 +-
.../Drupal/entity_test/EntityTestViewBuilder.php   |  12 +-
.../views/Tests/Entity/RowEntityRenderersTest.php  |   5 +-
Files: 
CommentFileSizeAuthor
#216 pre-render-2099131-216.patch145.59 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,243 pass(es).
[ View ]
#216 interdiff.txt145.59 KBjessebeach
#211 interdiff.txt7.75 KBjessebeach
#211 pre-render-2099131-211.patch145.73 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,252 pass(es).
[ View ]
#201 interdiff.txt1.79 KBWim Leers
#201 pre-render-2099131-201.patch145.17 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,956 pass(es).
[ View ]
#190 interdiff.txt19.15 KBWim Leers
#190 pre-render-2099131-188.patch140.71 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,847 pass(es), 3 fail(s), and 212 exception(s).
[ View ]
#133 pre-render-2099131-132.patch108.99 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,583 pass(es).
[ View ]
#130 interdiff.txt29.63 KBWim Leers
#130 2099131-130.patch107.57 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,699 pass(es).
[ View ]
#106 entity-pre-render-2099131-106.patch78.62 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,781 pass(es).
[ View ]
#106 interdiff.txt14.46 KBjessebeach
#103 2099131-102.patch64.47 KBcatch
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,752 pass(es).
[ View ]
#103 interdiff.txt885 bytescatch
#98 2099131-98.patch62.44 KBcatch
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,750 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#93 2099131-93.patch59.64 KBcatch
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,394 pass(es), 80 fail(s), and 36 exception(s).
[ View ]
#93 interdiff.txt21.3 KBcatch
#92 interdiff.txt14.09 KBcatch
#90 2099131-90.patch39.52 KBcatch
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,420 pass(es), 107 fail(s), and 37 exception(s).
[ View ]
#87 2099131-87.patch28.78 KBcatch
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,277 pass(es), 1,870 fail(s), and 216 exception(s).
[ View ]
#83 pre-render-2099131-84.patch28.97 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site.
[ View ]
#77 pre-render-2099131-77.patch28.97 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site.
[ View ]
#77 interdiff.txt2.17 KBjessebeach
#73 interdiff.txt9.17 KBjessebeach
#73 pre-render-2099131-73.patch30.78 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,905 pass(es), 100 fail(s), and 57 exception(s).
[ View ]
#71 interdiff.txt1.82 KBjessebeach
#71 pre-render-2099131-71.patch31.24 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,685 pass(es), 135 fail(s), and 23 exception(s).
[ View ]
#64 pre-render-2099131-64.patch30.16 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,721 pass(es), 969 fail(s), and 62 exception(s).
[ View ]
#62 pre-render-2099131-62.patch30.61 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#62 interdiff.txt13.16 KBjessebeach
#59 interdiff.txt6.38 KBjessebeach
#58 pre-render-2099131-57.patch26 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,871 pass(es), 138 fail(s), and 234 exception(s).
[ View ]
#53 interdiff.txt12.51 KBjessebeach
#53 pre-render-2099131-53.patch22.83 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,285 pass(es), 639 fail(s), and 251 exception(s).
[ View ]
#48 pre-render-2099131-48.patch12.1 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,069 pass(es), 28 fail(s), and 26 exception(s).
[ View ]
#48 interdiff.txt2.43 KBjessebeach
#45 interdiff.txt1.1 KBandypost
#41 pre-render-2099131-41.patch10.39 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,059 pass(es), 36 fail(s), and 26 exception(s).
[ View ]
#41 interdiff.txt1.41 KBjessebeach
#40 interdiff.txt1.26 KBjessebeach
#40 pre-render-2099131-40.patch10.27 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es), 39 fail(s), and 28 exception(s).
[ View ]
#39 render-placeholder-2.png40.05 KBjessebeach
#31 pre-render-2099131-31-interdiff.txt3.24 KBBerdir
#31 pre-render-2099131-31.patch9 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,654 pass(es), 156 fail(s), and 29 exception(s).
[ View ]
#23 field-with-patch-3.png193.61 KBjessebeach
#23 field-without-patch-4.png189.55 KBjessebeach
#22 pre-render-2099131-21.patch7.29 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 63,158 pass(es), 15 fail(s), and 51 exception(s).
[ View ]
#22 interdiff.txt4.08 KBjessebeach
#20 interdiff18-19.txt1.99 KBbenjifisher
#20 pre-render-2099131-19.patch7.29 KBbenjifisher
FAILED: [[SimpleTest]]: [MySQL] 62,706 pass(es), 181 fail(s), and 34 exception(s).
[ View ]
#17 pre-render-2099131-17-do-not-test.patch7.04 KBjessebeach
#17 interdiff.txt6.82 KBjessebeach
#16 interdiff.txt5.3 KBjessebeach
#16 pre-render-2099131-13-do-not-test.patch6.39 KBjessebeach
#11 pre-render-2099131-11-do-not-test.patch5.79 KBjessebeach
#11 interdiff.txt1.08 KBjessebeach
#4 pre-render-fields-2099131-4.patch5.97 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 59,713 pass(es), 159 fail(s), and 56 exception(s).
[ View ]
#138 pre-render-2099131-138.patch109.22 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,730 pass(es).
[ View ]
#138 interdiff.txt5.17 KBjessebeach
#140 interdiff.txt7.83 KBjessebeach
#140 pre-render-2099131-140.patch114.18 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-140.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#142 pre-render-2099131-142.patch114.14 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,763 pass(es), 7 fail(s), and 9 exception(s).
[ View ]
#144 interdiff.txt983 bytesjessebeach
#144 pre-render-2099131-144.patch114.15 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-144.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#146 pre-render-2099131-146.patch114.21 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,792 pass(es).
[ View ]
#147 pre-render-2099131-147.patch115.73 KBjessebeach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,874 pass(es).
[ View ]
#147 interdiff.txt9.47 KBjessebeach
#159 pre-render-2099131-159.patch112.65 KBjessebeach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,953 pass(es), 88 fail(s), and 12 exception(s).
[ View ]
#159 interdiff.txt20.56 KBjessebeach
#163 current-flow.jpg65.39 KBjessebeach
#163 new-flow.jpg95.78 KBjessebeach
#166 pre-render-2099131-166.patch115.86 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,268 pass(es), 85 fail(s), and 0 exception(s).
[ View ]
#166 interdiff.txt17.64 KBWim Leers
#177 pre-render-2099131-177.patch125.4 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-177.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#177 interdiff.txt19.85 KBWim Leers

Comments

moshe weitzman’s picture

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.

effulgentsia’s picture

Priority:Major» Critical
Issue summary:View changes
Issue tags:+beta blocker
yched’s picture

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.

jessebeach’s picture

Status:Active» Needs review
StatusFileSize
new5.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,713 pass(es), 159 fail(s), and 56 exception(s).
[ View ]

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

Run #52dedf6040aab Run #52dedf7982251 Diff Diff%
Number of Function Calls 177,076 176,995 -81 -0.0%
Incl. Wall Time (microsec) 1,495,867 1,705,220 209,353 14.0%
Incl. CPU (microsecs) 1,424,855 1,572,003 147,148 10.3%
Incl. MemUse (bytes) 12,833,536 18,852,296 6,018,760 46.9%
Incl. PeakMemUse (bytes) 12,936,944 19,234,840 6,297,896 48.7%

and just drupal_render

drupal_render Run #52dedf6040aab Run #52dedf7982251 Diff Diff%
Number of Function Calls 5 5 0 0.0%
Incl. Wall Time (microsec) 979,627 1,280,981 301,354 30.8%
Incl. Wall Time (microsec) per call 195,925 256,196 60,271 30.8%
Excl. Wall Time (microsec) 173 174 1 0.6%
Incl. CPU (microsecs) 931,135 1,177,204 246,069 26.4%
Incl. CPU (microsecs) per call 186,227 235,441 49,214 26.4%
Excl. CPU (microsec) 153 157 4 2.6%
Incl. MemUse (bytes) 8,273,144 15,334,128 7,060,984 85.3%
Incl. MemUse (bytes) per call 1,654,629 3,066,826 1,412,197 85.3%
Excl. MemUse (bytes) 25,968 24,264 -1,704 -6.6%
Incl. PeakMemUse (bytes) 8,589,344 15,668,008 7,078,664 82.4%
Incl. PeakMemUse (bytes) per call 1,717,869 3,133,602 1,415,733 82.4%
Excl. PeakMemUse (bytes) 1,632 1,392 -240 -14.7%

warm-cached vs. warm-cache

Run #52dee07b59770 Run #52dee09612f4e Diff Diff%
Number of Function Calls 36,145 27,953 -8,192 -22.7%
Incl. Wall Time (microsec) 335,142 267,652 -67,490 -20.1%
Incl. CPU (microsecs) 324,606 256,873 -67,733 -20.9%
Incl. MemUse (bytes) 7,928,568 6,711,192 -1,217,376 -15.4%
Incl. PeakMemUse (bytes) 7,948,008 6,715,832 -1,232,176 -15.5%

and just drupal_render

drupal_render Run #52dee07b59770 Run #52dee09612f4e Diff Diff%
Number of Function Calls 5 5 0 0.0%
Incl. Wall Time (microsec) 117,704 117,269 -435 -0.4%
Incl. Wall Time (microsec) per call 23,541 23,454 -87 -0.4%
Excl. Wall Time (microsec) 256 171 -85 -33.2%
Incl. CPU (microsecs) 112,020 111,741 -279 -0.2%
Incl. CPU (microsecs) per call 22,404 22,348 -56 -0.2%
Excl. CPU (microsec) 240 154 -86 -35.8%
Incl. MemUse (bytes) 3,019,848 3,144,736 124,888 4.1%
Incl. MemUse (bytes) per call 603,970 628,947 24,978 4.1%
Excl. MemUse (bytes) 25,200 23,336 -1,864 -7.4%
Incl. PeakMemUse (bytes) 3,095,592 3,219,296 123,704 4.0%
Incl. PeakMemUse (bytes) per call 619,118 643,859 24,741 4.0%
Excl. PeakMemUse (bytes) 20,840 21,352 512 2.5%
Berdir’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 4: pre-render-fields-2099131-4.patch, failed testing.

jessebeach’s picture

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.

jessebeach’s picture

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.

Berdir’s picture

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.

jessebeach’s picture

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;
}

jessebeach’s picture

Issue summary:View changes
StatusFileSize
new1.08 KB
new5.79 KB

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.

yched’s picture

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

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

jessebeach’s picture

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.

Berdir’s picture

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?

yched’s picture

@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') ?

jessebeach’s picture

StatusFileSize
new6.39 KB
new5.3 KB

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

More refactorings to EntityViewBuilder::viewMultiple().

jessebeach’s picture

StatusFileSize
new6.82 KB
new7.04 KB

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

jessebeach’s picture

  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.

Berdir’s picture

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)

benjifisher’s picture

Status:Needs work» Needs review
StatusFileSize
new7.29 KB
FAILED: [[SimpleTest]]: [MySQL] 62,706 pass(es), 181 fail(s), and 34 exception(s).
[ View ]
new1.99 KB

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?

Status:Needs review» Needs work

The last submitted patch, 20: pre-render-2099131-19.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new4.08 KB
new7.29 KB
FAILED: [[SimpleTest]]: [MySQL] 63,158 pass(es), 15 fail(s), and 51 exception(s).
[ View ]

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.

jessebeach’s picture

Issue summary:View changes
StatusFileSize
new189.55 KB
new193.61 KB

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.

jessebeach’s picture

Issue summary:View changes
jessebeach’s picture

Issue summary:View changes
yched’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 22: pre-render-2099131-21.patch, failed testing.

Berdir’s picture

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?

catch’s picture

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?

For tags, no:

Cache tags are collected only once the array has been fully rendered on a cache miss, including #pre_render callbacks. So drupal_render_collect_cache_tags() has the chance to find tags added anywhere in that process - including those in the $element returned from a pre_render callback. Similarly if nested elements are cached, we get their cache tags out of the cache entry (i.e. when the pre_render isn't called because it's a cache hit). Same goes for #attached (except for direct drupal_render() calls and the theme system which still break this - but that's not any better or worse here).

For cache keys, it's not possible for nested elements to affect those - since you need to know cache keys both on cache hits and cache misses. The only options there are #post_render_callback so you don't have to change the cache keys at all, or #2099137: [PP-1] Entity/field access and node grants not taken into account with core cache contexts to change them at the top level independently.

Berdir’s picture

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: [PP-1] Entity/field access and node grants not taken into account with core cache contexts. I expect that some test failures are related to that.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,654 pass(es), 156 fail(s), and 29 exception(s).
[ View ]
new3.24 KB

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.

Status:Needs review» Needs work

The last submitted patch, 31: pre-render-2099131-31.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review

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,,,

martin107’s picture

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

Status:Needs review» Needs work

The last submitted patch, 31: pre-render-2099131-31.patch, failed testing.

jessebeach’s picture

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!

jessebeach’s picture

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.
jessebeach’s picture

Issue summary:View changes

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

jessebeach’s picture

Issue summary:View changes
StatusFileSize
new40.05 KB

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.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new10.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es), 39 fail(s), and 28 exception(s).
[ View ]
new1.26 KB

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.

jessebeach’s picture

StatusFileSize
new1.41 KB
new10.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,059 pass(es), 36 fail(s), and 26 exception(s).
[ View ]

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.

The last submitted patch, 40: pre-render-2099131-40.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 41: pre-render-2099131-41.patch, failed testing.

Berdir’s picture

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

andypost’s picture

StatusFileSize
new1.1 KB

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

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

webchick’s picture

Committed the patch referenced in #45.

andypost’s picture

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

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
new12.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,069 pass(es), 28 fail(s), and 26 exception(s).
[ View ]

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.

The last submitted patch, 41: pre-render-2099131-41.patch, failed testing.

The last submitted patch, 41: pre-render-2099131-41.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 48: pre-render-2099131-48.patch, failed testing.

The last submitted patch, 48: pre-render-2099131-48.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new22.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,285 pass(es), 639 fail(s), and 251 exception(s).
[ View ]
new12.51 KB

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

Status:Needs review» Needs work

The last submitted patch, 53: pre-render-2099131-53.patch, failed testing.

jessebeach’s picture

"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.

Berdir’s picture

  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?

jessebeach’s picture

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.

jessebeach’s picture

StatusFileSize
new26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,871 pass(es), 138 fail(s), and 234 exception(s).
[ View ]

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.

jessebeach’s picture

StatusFileSize
new6.38 KB

The interdiff to #58.

jessebeach’s picture

Status:Needs work» Needs review

sorry for all the noise :/

Status:Needs review» Needs work

The last submitted patch, 58: pre-render-2099131-57.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new13.16 KB
new30.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another go at a full round of testing.

Status:Needs review» Needs work

The last submitted patch, 62: pre-render-2099131-62.patch, failed testing.

martin107’s picture

StatusFileSize
new30.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,721 pass(es), 969 fail(s), and 62 exception(s).
[ View ]

Reroll

martin107’s picture

Status:Needs work» Needs review

triggering testbot

Status:Needs review» Needs work

The last submitted patch, 64: pre-render-2099131-64.patch, failed testing.

martin107’s picture

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

jessebeach’s picture

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.

martin107’s picture

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.

jessebeach’s picture

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.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new31.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,685 pass(es), 135 fail(s), and 23 exception(s).
[ View ]
new1.82 KB

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

Status:Needs review» Needs work

The last submitted patch, 71: pre-render-2099131-71.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new30.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,905 pass(es), 100 fail(s), and 57 exception(s).
[ View ]
new9.17 KB

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.

Run #531e520a07af1 Run #531e52244956f Diff Diff%
Number of Function Calls 85,162 89,609 4,447 5.2%
Incl. Wall Time (microsec) 1,115,109 835,682 -279,427 -25.1%
Incl. CPU (microsecs) 1,074,384 805,639 -268,745 -25.0%
Incl. MemUse (bytes) 15,542,264 14,105,896 -1,436,368 -9.2%
Incl. PeakMemUse (bytes) 15,605,920 14,170,256 -1,435,664 -9.2%

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.

Status:Needs review» Needs work

The last submitted patch, 73: pre-render-2099131-73.patch, failed testing.

jessebeach’s picture

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.

jessebeach’s picture

Issue tags:-beta blocker

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.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
new28.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 77: pre-render-2099131-77.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 77: pre-render-2099131-77.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 77: pre-render-2099131-77.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new28.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 83: pre-render-2099131-84.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 83: pre-render-2099131-84.patch, failed testing.

catch’s picture

StatusFileSize
new28.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,277 pass(es), 1,870 fail(s), and 216 exception(s).
[ View ]

Started having a look at this. Haven't got anywhere yet but here's a re-roll.

catch’s picture

Status:Needs work» Needs review

Getting an up-to-date test bot run.

Status:Needs review» Needs work

The last submitted patch, 87: 2099131-87.patch, failed testing.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new39.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,420 pass(es), 107 fail(s), and 37 exception(s).
[ View ]

Still lots to do but here's an interim patch. I hit xdebug max function nesting level, but setting it to 500 I got 100% pass on the comment links test, and a bit of manual testing of node links was OK.

jessebeach’s picture

Catch, can you post an interdiff for #90?

catch’s picture

StatusFileSize
new14.09 KB

And the interdiff.

The main change is removing support for #type => 'render_cache_placeholder'. This makes it impossible to reliably collect #post_render_cache callbacks because you have to render the placeholder to get the callback set.

Instead I'm converting the cases that use this to just use #post_render_cache directly - that removes the chicken/egg situation and seems to be working.

catch’s picture

StatusFileSize
new21.3 KB
new59.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,394 pass(es), 80 fail(s), and 36 exception(s).
[ View ]

More or less completely removed support for #type => 'render_cache_placeholder' and the tests supporting it. That hopefully means we'll be down to real test failures / failures due to stuff that's not converted yet.

Also getting RenderTest to passing. I removed assertions checking that the element itself can't be altered. While we only care about #attached and #markup changes, I'm not convinced it matters if the element can or cannot actually be changed?

The last submitted patch, 90: 2099131-90.patch, failed testing.

jessebeach’s picture

  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.

Status:Needs review» Needs work

The last submitted patch, 93: 2099131-93.patch, failed testing.

catch’s picture

StatusFileSize
new62.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,750 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new4.04 KB

For now here's a brute force removal of that code. The long comment in the interdiff explains what's happening. Dynamically setting a field value on an entity, rendering it, then setting it back is not going to work with entity caching at all. So we really need the real view mode and/or proper previews.

One other thought that occurred to me - this is only an issue if the comment form is shown on the same page as the entity, and the entity gets rendered on the comment reply form. In that case the only way to get to this page is to manually navigate it, so we could just skip it for that case maybe and leave if the form won't be shown.

catch’s picture

Status:Needs work» Needs review

CNR for the bot.

catch’s picture

StatusFileSize
new63.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,753 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.17 KB

Fixes RowEntityRenderersTest.

The last submitted patch, 98: 2099131-98.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 100: 2099131-100.patch, failed testing.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new885 bytes
new64.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,752 pass(es).
[ View ]

Fixes NodeTitle test - just the comment reply behaviour change.

Should be green this time.

jessebeach’s picture

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.

Wim Leers’s picture

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 :)

jessebeach’s picture

StatusFileSize
new14.46 KB
new78.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,781 pass(es).
[ View ]

I've put the following tests back:

testDrupalRenderRenderCachePlaceholder
testDrupalRenderChildElementRenderCachePlaceholder

catch’s picture

StatusFileSize
new64.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,834 pass(es).
[ View ]
new6.18 KB

Thanks for putting the tests back Jesse!

@Wim, thanks for the review. I've fixed the easy stuff, commented on some I don't think we should tackle here and/or need a bit more of a look.

#1. Fixed.
#2. Yes I think that's fine, exactly where it should go I went back and forth on, but 'near the end', seemed good :)
#3. I don't know, will ask Jesse and/or try removing in a side issue to see what happens.
#4. Fixed.
#5. node_test_entity_view_mode_alter() implements it. I fixed the argument for now.
#6. Fixed.
#7. It is bizarre, but it was already here. It would be fantastic to just use $build here, but let's open a separate issue to clean just that up.
#8. Fixed.
#9. Reading through I'm not convinced we need this code, will try removing in a helper issue and see what breaks.
#10. Jesse did this to make things consistent with code elsewhere. Leaving in for now but we could possibly revert that change and handle it somewhere else.
#11. Fixed.
#12. per #7 I think we should have a dedicated issue to deal with $entity->content
#13/#17 will look at those in a bit.
#14. Yes it's possible to do that now, but it won't have any practical difference. I'm not sure we can absolutely lock it down or need to particularly.

catch’s picture

StatusFileSize
new79.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 61,222 pass(es), 5,455 fail(s), and 1,119 exception(s).
[ View ]

Gahhh wrong patch uploaded.

catch’s picture

Status:Needs review» Needs work

The last submitted patch, 108: 2099131-107.patch, failed testing.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new79.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,785 pass(es).
[ View ]
new1.44 KB

Fixing some silliness.

Status:Needs review» Needs work

The last submitted patch, 111: 2099131-110.patch, failed testing.

catch’s picture

111: 2099131-110.patch queued for re-testing.

Wim Leers’s picture

#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.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new82.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,880 pass(es), 13 fail(s), and 3 exception(s).
[ View ]

New patch. I've removed FormatterBase::getDefaults() because it shouldn't know about the specific field types. Added cache tags to the render array returned by the formatter plugins instead. Note that the full entity reference formatter plugin doesn't need to explicitly add tags, because the referenced entity rendering handles that itself.

I think this addresses everything in Wim's review now except the $entity->content stuff. I tried removing one of the changes there, but it broke horribly - pretty sure it's because we add the defaults before it gets there. We'd have to add the all again if we went back to completely clearing out $entity->content again. Jesse do you remember how you ended up with those changes?

Cache tags tests pass locally, hopefully I caught them all.

catch’s picture

StatusFileSize
new6.18 KB
yched’s picture

+++ 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 :-)

Wim Leers’s picture

#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.

Status:Needs review» Needs work

The last submitted patch, 115: 2099131-112.patch, failed testing.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new85.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,857 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new3.85 KB

Should fix the test failures...

Status:Needs review» Needs work

The last submitted patch, 120: 2099131-115.patch, failed testing.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new669 bytes
new85.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,904 pass(es).
[ View ]

Missed one apparently.

catch’s picture

StatusFileSize
new84.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,927 pass(es).
[ View ]
new3.18 KB
new28.92 KB

OK new patch with Berdir's fix from #30 for the comments, which I missed. There's now no regression in comment preview for the entity reviews, so this is mostly a revert of those changes.

Also did some profiling, saves 15% of function calls when viewing an article node on the standard profile with a handful of tags and comments. The main saving here is that when viewing the node, we don't have to load any comments or taxonomy terms at all since that only happens on cache misses now.

Would be worth some profiling with a much more loaded node or more fields on comments and/or a longer list of nodes, but this is a really nice improvement.

effulgentsia’s picture

Status:Needs review» Needs work
  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.

effulgentsia’s picture

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.

catch’s picture

I made a start on $entity->content in #2228043-25: Helper for #2099131 but it's very far from passing. I don't mind if we do it here, but also wouldn't want to hold this issue up on that.

catch’s picture

Issue tags:+beta blocker

#124 yeah I think the #build is a legacy from a previous patch, just needs the comment fixed and re-naming. I'll try to re-roll soonish and see if we can revert the other change.

There's not much of an explicit API change here, but it does affect what you get back if you call entity_view() very significantly. Given the 16k function call saving and the fact this is passing now, moving back to beta blocker.

Wim Leers’s picture

Assigned:Unassigned» Wim Leers
Status:Needs work» Needs review
Issue tags:+Spark, +sprint
StatusFileSize
new114.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,667 pass(es).
[ View ]
new30.29 KB
new22.29 KB

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.

Wim Leers’s picture

StatusFileSize
new125.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,582 pass(es).
[ View ]
new19.29 KB
  • 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.

Wim Leers’s picture

Assigned:Wim Leers» Unassigned
StatusFileSize
new107.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,699 pass(es).
[ View ]
new29.63 KB

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.
rbayliss’s picture

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

rbayliss’s picture

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.

jessebeach’s picture

StatusFileSize
new108.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,583 pass(es).
[ View ]
new7.15 KB

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.

jessebeach’s picture

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:

Run #534c2324e9a8b Run #534c25620bf0b Diff Diff%
Number of Function Calls 155,837 134,708 -21,129 -13.6%
Incl. Wall Time (microsec) 698,445 621,373 -77,072 -11.0%
Incl. MemUse (bytes) 15,173,536 13,862,968 -1,310,568 -8.6%
Incl. PeakMemUse (bytes) 15,419,496 14,106,680 -1,312,816 -8.5%

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.

Run #534c17dc6347c Run #534c18b2cb4cd Diff Diff%
Number of Function Calls 860,899 855,690 -5,209 -0.6%
Incl. Wall Time (microsec) 3,924,249 3,943,316 19,067 0.5%
Incl. MemUse (bytes) 33,354,576 34,005,888 651,312 2.0%
Incl. PeakMemUse (bytes) 33,628,960 34,300,240 671,280 2.0%

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.

jessebeach’s picture

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!

catch’s picture

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.

Having #pre_render work on multiple for the cold cache issue would be great. I tried this a bit with performance_hacks but couldn't find a clean solution (hence the name of that module). The problem I ran into is that the cache get on the render cache is single - so to know which items you want to do multiple operations on, you need to be able to operate only on those items that are cache misses, but when rendering an individual item, it doesn't know if it's part of a list or not. 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.

jessebeach’s picture

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.

jessebeach’s picture

StatusFileSize
new109.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,730 pass(es).
[ View ]
new5.17 KB

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.

jessebeach’s picture

Status:Needs review» Needs work

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

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new7.83 KB
new114.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-140.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 140: pre-render-2099131-140.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new114.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,763 pass(es), 7 fail(s), and 9 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 142: pre-render-2099131-142.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new983 bytes
new114.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-144.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should sort the failing tests.

Status:Needs review» Needs work

The last submitted patch, 144: pre-render-2099131-144.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new114.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,792 pass(es).
[ View ]

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

jessebeach’s picture

StatusFileSize
new115.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,874 pass(es).
[ View ]
new9.47 KB

Code cleanup and consolidation.

jessebeach’s picture

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.

Run #534ed5e356ab2 Run #534ed56303253 Diff Diff%
Number of Function Calls 194,200 162,678 -31,522 -16.2%
Incl. Wall Time (microsec) 815,321 763,828 -51,493 -6.3%
Incl. MemUse (bytes) 15,789,920 14,077,928 -1,711,992 -10.8%
Incl. PeakMemUse (bytes) 16,236,568 14,515,456 -1,721,112 -10.6%

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).

Run #534ed8aa1a561 Run #534ed9032d455 Diff Diff%
Number of Function Calls 75,233 64,809 -10,424 -13.9%
Incl. Wall Time (microsec) 374,003 329,826 -44,177 -11.8%
Incl. MemUse (bytes) 13,014,336 11,739,000 -1,275,336 -9.8%
Incl. PeakMemUse (bytes) 13,125,032 11,850,304 -1,274,728 -9.7%

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.

Drupal\comment\CommentViewBuilder::buildContent Run #534edcd30e3df Run #534edae197353 Diff Diff%
Number of Function Calls 1 1 0 0.0%
Incl. Wall Time (microsec) 74,811 60,562 -14,249 -19.0%
Incl. Wall Time (microsec) per call 74,811 60,562 -14,249 -19.0%
Excl. Wall Time (microsec) 3,807 3,092 -715 -18.8%
Incl. MemUse (bytes) 601,712 617,392 15,680 2.6%
Incl. MemUse (bytes) per call 601,712 617,392 15,680 2.6%
Excl. MemUse (bytes) 209,576 230,568 20,992 10.0%
Incl. PeakMemUse (bytes) 556,280 596,792 40,512 7.3%
Incl. PeakMemUse (bytes) per call 556,280 596,792 40,512 7.3%
Excl. PeakMemUse (bytes) 142,008 82,608 -59,400 -41.8%

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.

jessebeach’s picture

Assigned:Unassigned» catch

#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.

yched’s picture

  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 ?)

xjm’s picture

Issue tags:+Entity Field API
catch’s picture

Did you profile a warm cache between #138 and the latest patch? Reviewing visually I'd expect the the multiple #pre_render to always run, therefore a performance regression in the cached case. If it doesn't it'd be good to document how that doesn't happen.

sun’s picture

jessebeach’s picture

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.

catch’s picture

The multiple build #pre_render is only called when EVB::viewMultiple() is called outside of a call to EVB::view().

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

jessebeach’s picture

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?

jessebeach’s picture

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.

catch’s picture

OK here's a scenario in core/views. Agreed though, when the full list gets render cached as a single chunk of HTML, this is significantly less of a problem anyway.

Node gets rendered with 50 comments.

One comment gets updated - this invalidates both the render cache item for the node, and for that comment. The other 49 comments stay the same.

Node gets rendered again, now we have 49 comments render cached, and one that isn't.

There's two trade-offs:

1. On the full cache miss. We'll either render all the items individually (regression), or multiple build.

2. On the partial cache miss, we'll either render only the items that need it, or multiple build all of them to render one or two (not a regression compared to HEAD but a regression compared to previous patches).

Same situation for views listings.

The only way to completely avoid the trade-offs would be check the cache entries for the child render arrays first, then do the multiple pre-render on the remaining items, which we don't have a mechanism for.

I think it's fine for this issue to go in with either approach (apart from resolving yched's review), and we should open a major follow-up for the single/multiple stuff to figure it out more.

Also I'd stick with doing this on the main EntityViewBuilder - whatever we do it makes sense to do that consistently across all entity types.

jessebeach’s picture

StatusFileSize
new112.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,953 pass(es), 88 fail(s), and 12 exception(s).
[ View ]
new20.56 KB

It took me a couple hours to respond to this :) Thank you for the review yched!

#150:1

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

Agreed, I've made this change.

#150:2-1

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 ?

I've changed the method names to buildEntity and buildEntityMultiple, in parallel with view, viewMultiple and buildContent.

#150:2-2

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 :-/

Good point. EVB:view() and EVB:viewMultiple() is the pattern we're trying to emulate and delay here, so I've done that in the build functions as well. buildEntity now works like view in that it packages an array, sends it to buildEntityMultiple and then returns the zeroeth item from the array to the calling function.

#150:2-3

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 ?

Let's just do that now with the new methods.

#150:3

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 ?)

Yes, totally. Great suggestion. Done :)

#150:4 - Removed.
#150:5 - space added.
#150:6

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

Added more commentary.

#150:7

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.

in my understanding, these are defaults for cache checking and, if needed, kicking off a full build on a cache-miss. The bare minimum set of information we need to know about a renderable to make a decision to get a cache entry or proceed with building and some info about the view mode, langcode and theme.

I agree that view and viewMultiple are a bit misleading now, as evidenced by the method WebTestBase:drupalBuildEntityView, which performs a mix of build and render to get the full renderable for an entity. I think this complexity is the current cost of a render process (drupal_render()) that has procedural build steps and render steps baked into it.

This is the way that EntityViewBuilder implements EntityViewBuilderInterface in order to optimize for performance. I think outside of this class, those method names make sense given how another class of this interface might choose to interpret what view and viewMultiple should do.

I'm definitely holding my nose a little here for the sake of faster cache-hit requests. Is it a little better with the pre_render method renames and refactoring the method order?

#150:8 - ya!

#150:9

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

Agreed, and \Drupal\Core\Entity\EntityViewBuilder doesn't include these defaults in its signature. I've pulled them out of the sub class method signatures.

#150:10

In current HEAD, $formatter->view() returns a build array that is *not* wrapped in a $field_name key,

Right, we must have started this work while the fields were still keyed by field name. I've updated the code.

// Then let the formatter build the output for each entity.
foreach ($entities as $key => $entity) {
  $items = $entity->get($field_name);
  if (!isset($build[$key][$field_name])) {
    $build[$key][$field_name] = array();
  }
  $build[$key][$field_name] += $formatter->view($items);
  $build[$key][$field_name]['#access'] = $items->access('view');
}

#150:11

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 ?

The anonymous function caused a minor performance regression (~3.5ms) because it gets created thousands of times. I was looking at this with msonnabaum. We were able to eliminate the regression by defining a method and passing it as a callback.

#150:12

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 ?)

catch, I think you added this code. Can you speak to this?

Status:Needs review» Needs work

The last submitted patch, 159: pre-render-2099131-159.patch, failed testing.

catch’s picture

The cache tags for the ID formatter I added because the new entity cache tags tests were failing. Also yes we need them in real life due to deletions.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new6.92 KB
new115.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,087 pass(es).
[ View ]

This should resolve the failing tests. I neglected to reference the entity $key on the $build array passed to the view hook implementations during the build step.

I also noticed that the $referenced_entity variable in the EntityReferenceidFormatter is not assigned an initial value and when this code path is followed, results in a whitescreen. Wondering how that would have passed tests, I learned that we do not have any test coverage for this code. Indeed, none of the entity reference formatters have test coverage for their view methods. So I've added test coverage for the code this issue changes. I logged an issue to add coverage for the other formatters. I'd rather not take on that work in this issue, which is already quite large.

#2246655: Expand the entity reference field formatter test coverage

jessebeach’s picture

Issue summary:View changes
StatusFileSize
new65.39 KB
new95.78 KB
jessebeach’s picture

Issue summary:View changes
jessebeach’s picture

Issue summary:View changes
Wim Leers’s picture

StatusFileSize
new115.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,268 pass(es), 85 fail(s), and 0 exception(s).
[ View ]
new17.64 KB
new8.29 KB
new3.89 KB
new4.25 KB
new2.12 KB

Great progress here!

In this reroll:

  1. I found the interplay between the ::buildEntityMultiple() and ::buildEntity() #pre_render callbacks unnecessarily complex. Having one #pre_render callback unset another feels … dirty. We can make it much simpler. Instead of setting ::buildEntity() in getBuildDefaults(), we just only set it in ::view(), just like ::buildEntityMultiple() is set by ::viewMultiple().
    This works fine in theory, however… Views calls ::viewMultiple(), but then extracts individual entities… which breaks this. However, there is no performance advantage to calling ::viewMultiple(), but then still calling drupal_render() on individual entities (this still triggered ::buildEntity(), not ::buildEntityMultiple()), so the solution was simple: just make Views' \Drupal\views\Entity\Render\DefaultLanguageRenderer and \Drupal\views\Entity\Render\RendererBase a lot simpler: always use ::view(). Problem solved, and much simpler code both in EntityViewBuilder and Views' entity rendering.
  2. Moved EntityViewBuilder::buildContent() higher in the file again, now the diff for it is tiny instead of huge.
  3. The updates to all field formatters to correctly set cache tags are nice, but they're not yet using #2217749: Entity base class should provide standardized cache tags and built-in invalidation, which went in in the mean time. Plus, most of those changes wrongfully included the "_view" cache tag as well: that tag should only be added if the referenced entity's view builder is actually rendering the entity, which was never the case (entity reference id, label or link formatters). Fixed.
  4. Minor docs fixes.

(See the interdiff-166-[NUMBER].txt files for each of the numbered changes above.)


Review

We are very close, I have very little left to remark upon :)

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -205,60 +170,141 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +      \Drupal::moduleHandler()->alter(array($entityType . '_defaults', 'entity_defaults'), $build[$key], $entity, $view_mode, $langcode);

    This new hook is not yet documented in entity.api.php, node.api.php, etc.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -205,60 +170,141 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +  public function buildEntity(array $build) {
    ...
    +  public function buildEntityMultiple(array $build) {

    Why not just rename these build() and buildMultiple()?

    There is no ambiguity around what we're building, so IMO that'd be just fine.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -205,60 +170,141 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +    // Find the keys for the ContentEntities in the build; Store entities for
    ...
    +        if ($entity instanceof ContentEntityInterface) {

    Why are the ContentEntityInterface checks necessary?

Status:Needs review» Needs work

The last submitted patch, 166: pre-render-2099131-166.patch, failed testing.

catch’s picture

The updates to all field formatters to correctly set cache tags are nice

Those are the ~85 test failures.

yched’s picture

Thanks @jessebeach and @Wim Leers, that's a ton clearer indeed !

Howewer :

there is no performance advantage to calling ::viewMultiple(), but then still calling drupal_render() on individual entities (this still triggered ::buildEntity(), not ::buildEntityMultiple())

Hm, this is true with the patch, but wasn't true so far: when it included building the formatted field values, viewMultiple() ran formatter's prepareView() (which typically includes loading referenced entities) on all entities in one pass, while this now runs entity by entity on cache misses.

But I guess that can't be avoided as long as Views does separate drupal_render()s.

Also, this means Views does a series of single cache gets instead a multiple one ? That's a bit sad :-/
In short, not for this issue, but Views doing separate drupal_render()s has a lot more negative impact now than in current HEAD (even if the overall result of HEAD vs. patch is still likely a win).

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -118,9 +116,9 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    -        $entity->content += $build[$id];
    +        $build[$id] = array_merge($build[$id], $display_build[$id]);

    Not sure we really need to move from += to array_merge ? The former is more fluent.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -133,17 +96,19 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
    +      // Collect cache defaults for this entity.
           '#cache' => array(
             'tags' =>  NestedArray::mergeDeep($this->getCacheTag(), $entity->getCacheTag()),
           ),
         );
         if ($this->isViewModeCacheable($view_mode) && !$entity->isNew() && $entity->isDefaultRevision() && $this->entityType->isRenderCacheable()) {
    -      $return['#cache'] += array(
    +      $build['#cache'] += array(
             'keys' => array(

    Not introduced here, but : we add $build['#cache']['tags'] even if we find out on the next line that the entity / view mode do not support caching ?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -195,7 +194,15 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
         $buildList = $this->viewMultiple(array($entity), $view_mode, $langcode);

    camelCase var ? Some folks in here must write JS code from time to time ;-)
    (buildEntity() calls it $buildlist)

    -> $build_list ?

  4. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -205,60 +212,125 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +    $build = array(

    Probably not introduced here, but for clarity, we should rename the $build variable used within viewMultiple() (it's the "build" for the multiple entities, whereas everywhere else $build is used for the "build" of a given entity)

    --> $build_list like in view() / buildEntity() ?

  5. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -205,60 +212,125 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +      \Drupal::moduleHandler()->alter(array($entityType . '_defaults', 'entity_defaults'), $build[$key], $entity, $view_mode, $langcode);

    $this->moduleHandler ?

  6. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -205,60 +212,125 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +        \Drupal::moduleHandler()->alter(array($view_hook, 'entity_view'), $build[$key], $entity, $display);

    $this->moduleHandler ?

  7. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewDisplay.php
    @@ -230,7 +230,10 @@ public function buildMultiple(array $entities) {
    -          $build[$key][$field_name] = $formatter->view($items);
    +          if (!isset($build[$key][$field_name])) {
    ...
    +          }
    +          $build[$key][$field_name] += $formatter->view($items);

    Still don't get why we suddenly need to be extra cautious now. Why would the $build[$key][$field_name] entry exist already ?
    If it really can exist, then it probably deserves an explanation in a comment :-)

  8. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -365,16 +365,23 @@ protected function getFieldDefinitions() {
    +    $display_context = $this->displayContext;
    +    return $definition->getDisplayOptions($display_context);

    Then we might as well inline the $display_content var, it was only there because you can't "use" $this in closures in 5.3 :-)

  9. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -365,16 +365,23 @@ protected function getFieldDefinitions() {
    +  }
    }

    Missing empty line after last method

yched’s picture

Also :

  1. +1 on renaming buildEntity() / buildEntityMultiple() to build() / buildMultiple(),
    but buildContent() feels a bit out of place in the middle.

    --> rename it to buildEntity() ? We'd have build() / buildMultiple() / buildEntity()...

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -133,17 +131,18 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
       protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
    -    $return = array(
    +    $build = array(
           '#theme' => $this->entityTypeId,
           "#{$this->entityTypeId}" => $entity,
           '#view_mode' => $view_mode,
           '#langcode' => $langcode,

    No biggie but looks like '#theme' doesn't need to be set here, and would rather belong in buildContent() / buildEntity() ?

  3. Still not too convinced on the naming regarding the "pre-cache" phase : getBuildDefaults() & hook_[ENTITY_TYPE]_defaults_alter()

    - getBuildDefaults() runs in a completely different phase than build() / buildMultiple() / buildEntity(), and is mostly in charge of assembling the #cache data and saving its params for the pre_render steps. It's not really about "defaults" anymore.
    - hook_node_defaults_alter() is not really telling about what the hook does and when it kicks in the process.

jessebeach’s picture

Still not too convinced on the naming regarding the "pre-cache" phase

I'm right there with you. What about something with "pre render", for example

preRender()

hook_pre_render_alter

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new117.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new3.21 KB

This should resolve the EntityReferenceFormatterTest fail and the Views translation test fails. Wim, the code in the preRender function that your removed regarding $count_langcodes was necessary to build an entity multiple times in a view in different languages. I pushed that processing up to TranslationLanguageRenderer. I also had to fix a fatal in CurrentLanguageRenderer; it needs a no-op getLanguage() method so that it will return null and force the view builder to use the current language.

The rest of the fails appear to be in the Entity and Page cache checking, which leads me to believe the root of the problem is in some base or common class.

I'd like to resolve these fails in #166 before addressing the subsequent reviews.

Status:Needs review» Needs work

The last submitted patch, 172: pre-render-2099131-172.patch, failed testing.

jessebeach’s picture

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

The last submitted patch, 172: pre-render-2099131-172.patch, failed testing.

Wim Leers’s picture

Assigned:catch» Wim Leers

Working on this, so assigning to prevent double work. I have 99% of yched's feedback addressed, but have now run into a problem that will probably keep me busy for a bit longer.

Wim Leers’s picture

Assigned:Wim Leers» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.27 KB
new10.83 KB
new7.95 KB
new19.85 KB
new125.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-177.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#169:

Hm, this is true with the patch, but wasn't true so far: when it included building the formatted field values, viewMultiple() ran formatter's prepareView() (which typically includes loading referenced entities) on all entities in one pass, while this now runs entity by entity on cache misses.

True.

But I guess that can't be avoided as long as Views does separate drupal_render()s.

Exactly.

Also, this means Views does a series of single cache gets instead a multiple one ?

Render cache always retrieves (now and in the past) single cache entries — because drupal_render() only considers a single the current subset of the renderable array, and therefore must always do individual cache gets.

Views doing separate drupal_render()s has a lot more negative impact now than in current HEAD (even if the overall result of HEAD vs. patch is still likely a win)

Correct on both counts.
However… once we get to the point where Views bubbles up cache tags correctly, we'll be able to render cache the entire View output. So this will become moot.

#172: Aha! I didn't realize the same entity could be rendered multiple times, but in different languages. Good catch!


Tests fixed. The tests were correctly asserting that the <entity type>_view tag was present of the referenced entity, but were only using the EntityReferenceLabelFormatter. Now I've switched to EntityReferenceEntityFormatter, so now the assertions pass. Having done that, though, it turns out that a few other assertions were incomplete: some assertions correctly asserted that the entity type's "view" cache tag was present, but failed to assert that the cache tags associated with the fields within a fully rendered view of the entity were also present. So, fixed that also.
Finally, doing all that revealed a big problem (the one that kept me busy for quite a bit longer) in EntityReferenceEntityFormatter::viewElements(): that function runs during the #pre_render phase (it's called by EntityViewBuilder::buildContent(), which is called by EntityViewBuilder::buildMultiple(), which is called by EntityViewBuilder::build(), which is the #pre_render callback of a single entity view), after which all cache tags must be collected. But… ::viewElements() itself renders another entity, for which another #pre_render callback would need to run to collect its cache tags in turn! The only solution there is to immediately invoke the rendered referenced entity's #pre_render callbacks immediately.
Feel free to convince yourself that this is necessary by commenting the added code in EntityReferenceEntityFormatter (if the updated tests' failures are insufficient), creating a node using the "Basic HTML" text format, commenting on it using the "Plain text" text format, clearing the render cache, visiting /node/1 and then looking at the cache entries in {cache_render}: you'll see that the entity_view:comment:1:… render cache entry has the filter_format:plain_text cache tag, but that entity_view:node:1:… render cache entry doesn't have it, even though it does contain the comment! Uncomment the code, clear the render cache again, reload the page, and you'll see that the comment's text format's cache tag is correctly bubled up.
(See interdiff-177-cachetags.txt.)

#169:

  1. Agreed, fixed.
  2. For good reason: even if a renderable itself is not going to be render cached, it must inform higher level render caches (such as the block cache or page cache) by which cache tags it should be invalidated. Cache tags must *always* be set.
  3. Fixed.
  4. Agreed, fixed. For the same reason, also renamed buildEntityMultiple()'s $build parameter to $build_list and EntityViewDisplay::buildMultiple()'s local $build variable.
  5. Fixed.
  6. Fixed.
  7. Agreed, fixed. And maybe I'm missing something, but I think that in fact zero changes are necessary here. So: reverted all those changes.
  8. Fixed.
  9. Fixed.

(See interdiff-177-169.txt.)

#170:

  1. Great, thanks for the confirmation, done! Note that EntityViewDisplay also has ::build() and ::buildMultiple(), so it's perfectly in line with that :)
    RE: ::buildContent(), I haven't done that rename yet because I don't wholeheartedly agree with ::buildEntity(). To me, either ::buildComponents() or ::buildFields() sounds like a better name, because that's precisely what it does: it fills the renderable array with the components corresponding to the Entity's Fields. Thoughts?
  2. I'm not sure: ::buildContent() is responsible for only the components (fields) within the entity (i.e. children of the renderable array), whereas this #theme property affects the entire renderable array.
  3. I don't feel strongly about this, but here are my thoughts:
    1. I think renaming hook_[ENTITY TYPE]_defaults_alter() to hook_[ENTITY TYPE]_build_defaults_alter() may be sufficient.
    2. I think getBuildDefaults() is still at-least-kind-of-appropriate — we haven't changed anything noteworthy about it? If you really want to modify the actual built render array, then you must override ::alterBuild(), just like before.
    3. It doesn't matter that the various methods now run in different phases, what matters is that the order they run in is still the same: ::getBuildDefaults() for the very sparse, high-level renderable array; ::alterBuild() for altering the final renderable array. It just happens to be that the sparse renderable array is transformed to the final one in a later phase, through a #pre_render callback, but that doesn't affect any of the (many) alter hooks.
    4. The complete set of changes for the developer is limited to this: if you want to dynamically alter an entity, then you have to make sure you modify its cache keys, or otherwise the cached version will be returned. So if you want to dynamically change the view mode, change the cache key corresponding to the view mode. If you want to do something else that's dynamic and external to the entity (e.g. a different color depending on the time of the day), append to the cache key to reflect that. You must make those kinds of changes via hook_[ENTITY TYPE]_defaults_alter() now, otherwise you're too late. So, I think renaming it to hook_[ENTITY_TYPE]_pre_render_alter() may be acceptable, though not preferable.

Thanks to the preceding explanation, I realized hook_entity_view_mode_alter() cannot possibly work anymore, since it runs too late. Apparently we don't have test coverage for that hook… fixed it.
(See interdiff-177-170.txt.)

catch’s picture

However… once we get to the point where Views bubbles up cache tags correctly, we'll be able to render cache the entire View output. So this will become moot.

Right, this is really important, and I missed it when discussing the single vs. multiple load stuff. Render caching of views lists means a single cache entry for the entire list.

If one entity in the list is updated, then the cache entry will be invalidated. However it will then fall back to the render cache entries for each item being rendered, which may or may not be hits. It's only when the list isn't cached that we even check those.

In Drupal 7 views you could cache the output of the entire list, but when that was a cache miss, you'd have to do a full render of each entity - it's this that we're replacing with the render cache get.

jessebeach’s picture

StatusFileSize
new125.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,612 pass(es).
[ View ]

Straight re-roll. Patch no longer applied.

jessebeach’s picture

RE: ::buildContent(), I haven't done that rename yet because I don't wholeheartedly agree with ::buildEntity(). To me, either ::buildComponents() or ::buildFields() sounds like a better name, because that's precisely what it does: it fills the renderable array with the components corresponding to the Entity's Fields. Thoughts?

buildContent() is building Displays. So let's call it buildDisplays().

The last submitted patch, 177: pre-render-2099131-177.patch, failed testing.

yched’s picture

  1. re: buildContent() / buildEntity() / buildFields() / buildDisplays()

    - buildEntity(): yeah, I can see why it's shaky. We're not "building" an $entity.
    - buildDisplays() is misleading IMO. "Display" / $display in the context of EVB is EntityViewDisplay objects. This doesn't "build an EntityViewDisplay" :-)
    - buildFields() : yeah, you could say that it's what the base implementation does, but it's only the base implementation. The method is also intended to be the go-to override entry point for entity-type-specific subclasses to add ad-hoc elements to the render array. So buildFields() seems too specific in that regard...

    -> buildEntityContent() ? or, well, maybe just stick to buildContent() then :-)
    (in which case, well, assigning '#theme' = $this->entityTypeId could fit in the scope of the method :-p)

  2. re: pre-cache phase :
    OK, I guess keeping getBuildDefaults() + renaming hook_[ENTITY TYPE]_build_defaults_alter() works for me.
  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -137,6 +137,10 @@ public function buildContent(array &$build, array $entities, array $displays, $v
       protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
    +    // Allow modules to change the view mode.
    +    $context = array('langcode' => $langcode);
    +    $this->moduleHandler()->alter('entity_view_mode', $view_mode, $entity, $context);
    +

    @@ -226,9 +228,6 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    -      // Allow modules to change the view mode.
    -      $this->moduleHandler()->alter('entity_view_mode', $view_mode, $entity, $context);
    -

    Oops, sorry, I was the one who asked for this change :-(. Don't fully get the reason, but if it's needed, well...

  4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
    @@ -95,6 +95,23 @@ public function viewElements(FieldItemListInterface $items) {
    +        // Entity field formatters are run during the #pre_render phase of
    +        // rendering an entity. Therefore, that is the time when this function
    +        // runs. After that #pre_render callback, all cache tags must be
    +        // available. However, this field formatter is highly exceptional: it
    +        // renders *another* entity (i.e. an entity within an entity). That
    +        // other entity comes with its own #pre_render callback that must be
    +        // called in order for the cache tags associated with that other entity
    +        // to be included with those of the main entity. Hence we must run the
    +        // other entity's #pre_render callbacks *immediately*, otherwise its
    +        // associated cache tags won't bubble up correctly.
    +        if (isset($elements[$delta]['#pre_render'])) {
    +          foreach ($elements[$delta]['#pre_render'] as $callable) {
    +            $elements[$delta] = call_user_func($callable, $elements[$delta]);
    +          }
    +        }
    +        unset($elements[$delta]['#pre_render']);
    +

    A bit beyond me :-), but that makes me wonder whether this formatter should do a multiple_view() across all deltas ?
    (or even across all entities of the parent multiple_view(), by pushing the entity_view() up in EntityRefEntityFormatter::prepareView() - this is hair pulling territory, probably for a followup if at all...)

catch’s picture

#4 confuses me.

In the case of a referenced entity, then we specifically don't want to force running the #pre_render callbacks, because that bypasses the entity render caching for that entity. If it's cached, we get the cache tags from cache.

So either we should just pass the render array back up (and eventually the cache tags will get collected). Or worst case do a drupal_render() and pass back cache tags and #attached back up.

Wim Leers’s picture

#182

  1. What about buildComponents()? :)
  2. Ok.
  3. It's explained at the *very* end of #177, below all bullets!
  4. For my next comment.

Will reply to #182.4 & #183 in a next comment.

jessebeach’s picture

StatusFileSize
new4.78 KB
new129.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,651 pass(es).
[ View ]
new9.7 KB

OK, I guess keeping getBuildDefaults() + renaming hook_[ENTITY TYPE]_build_defaults_alter() works for me.

Renamed hook_ENTITY_TYPE_defaults_alter to hook_ENTITY_TYPE_build_defaults_alter and hook_entity_defaults_alter to hook_entity_build_defaults_alter.

I added these hooks to entity.api.php and updated the draft change notice.

Also, buildContent() is now buildComponents() (I hope that's ok!). I put that interdiff in a separate file so it can be rolled back easily :) Also, if folks disagree I can roll back easily from my local branch.

Now, that should leave us with just the entity reference cache tag issue to resolve before we're at the final review, perf testing and maybe committing(??). I can dream...

Wim Leers’s picture

StatusFileSize
new136.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pre-render-2099131-186.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new15.32 KB
new135.97 KB
new4.09 KB

#182.4/#183: Without this change (and the improved change in this patch, thanks to #183 — see below), this is the bug being fixed:

  1. node 5 would have the node_view:1, node:<node ID>, user:3 and filter_format:full_html cache tags
  2. a comment then posted to node 5 would have the comment_view:1, comment:<comment ID> and filter_format:basic_html cache tags
  3. the re-rendered node 5 containing the above comment would have the node_view:1, node:<node ID>, filter_format:full_html and the comment_view:1 and comment:<comment ID> cache tags — note that filter_format:basic_html is missing!

In other words: by moving the bulk of the work to a #pre_render callback, cache tags set by field formatters of entities that are rendered inside of another entity through some formatter are not bubbled up! (In D8: EntityReferenceEntityFormatter and CommentDefaultFormatter.)

Why exactly: because of how the theme system and the render system interact. Roughly, this is what happens:

  1. at node/1, we have a render array (let's call it RRA, for Root Render Array) that contains a node (array('#theme' => 'node, '#node' => NODE_OBJECT, 'title' => …, 'body' => …, 'comment' => …)).
  2. For rendering that level, drupal_render() will call _theme($elements['#theme'], $elements), which will cause template_preprocess_node() to be called, which will copy the children of RRA (so the values for the keys 'title, 'body' and 'comment), like this: $variables['content']['comment'] = $page['nodes'][1]['comment'], etc.
  3. Then, the node.html.twig template will render {{ content|without('links') }}, which will cause $variables['content']['comment'] to be rendered.
  4. Hurray, our comments are rendered!
  5. But… because this operates on a copy of RRA, and drupal_render() does not recurse over subtrees of a tree for which a #theme function (or equivalent template) is successfully applied… #pre_render callbacks added by field formatters are never executed in the scope of RRA! Or, more generally, drupal_render() has not run on any of the subtrees of RRA.
    (This is generally fine, because the #pre_render callbacks are also executed when individual subtrees are rendered by a Twig template, so everything will be rendered correctly. However, we have a more advanced need: we need them to run on the RRA, because that's how we get our cache tags to bubble up!)

#183: hrm, good point! I didn't consider that. You're right that would mean a rendered referenced entity would always need to be rendered… but I wonder if that's such a big problem, since it's going to be cached as part of the referencing entity anyway?
To answer your two alternatives:

  1. Passing the render array back doesn't work. That's what we originally had. Give it a try. It'll trigger the failure I mentioned in #177 and explained at length above.
  2. Calling drupal_render() almost works. If I also fix render() to not call show() + drupal_render() (which sets #printed = FALSE, causing the content to show up doubled) in case the content is already pre-rendered/.

#186: lovely :)


In this reroll:

  1. Implemented #183.2: instead of applying #pre_render callbacks directly, run drupal_render() instead.
  2. … and to make the above possible: fixed a newly revealed-but-existing-since-forever problem in show()
  3. Deleted drupal_pre_render_render_cache_placeholder(), which was still a remnant from #type = render_cache_placeholder.
  4. #177 fixed EntityReferenceEntityFormatter, but now CommentDefaultFormatter is also fixed.
  5. Both of those Field Formatters now have additional test coverage to ensure that they correctly bubble up cache tags of the Entities they render inside of them.
  6. In working on these, I found a bug introduced by this patch in both TextDefaultFormatter and TextTrimmedFormatter that would cause the first filter format cache tag to remain the only one (because a string instead of an array was being used, NestedArray::mergeDeep() would not be able to merge multiple different filter format cache tags).

The *-fail.patch is the same as the actual patch, but with interdiff-fix-only.txt subtracted from it.

The last submitted patch, 186: pre-render-2099131-186.patch, failed testing.

jessebeach’s picture

StatusFileSize
new136.96 KB

Rerolled. Wim, your patches keep failing on the removal of the file core/modules/node/lib/Drupal/node/Tests/NodeBuildContentTest.php. Are you using git rm to remove it?

Status:Needs review» Needs work

The last submitted patch, 188: pre-render-2099131-188.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new140.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,847 pass(es), 3 fail(s), and 212 exception(s).
[ View ]
new19.15 KB
new139.73 KB
new4.09 KB

Rebased. HEAD changes very quickly these days!

Apparently I also forgot to include CommentDefaultFormatterTest in #186. So ignore everything in #186, this redoes it completely.

jessebeach’s picture

StatusFileSize
new627 bytes
new140.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,604 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

This is all the goodness of the patch in #190, now with fewer warnings :)

Status:Needs review» Needs work

The last submitted patch, 191: pre-render-2099131-191.patch, failed testing.

The last submitted patch, 190: pre-render-2099131-188.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 191: pre-render-2099131-191.patch, failed testing.

jessebeach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
new142.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,687 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixed \Drupal\image\Tests\ImageThemeFunctionTest. The test was being spar about reuse of a renderable and it was getting caught in the new check for double-rendering in render().

Now, \Drupal\comment\Tests\CommentDefaultFormatterCacheTagsTest poses a bit of a problem. It's failing on Testbot, but not locally. The failing case produces the same markup locally, so maybe it's just a difference in the way two arrays are compared in some set of conditions that's different on Testbot. Needs more investigation...

Status:Needs review» Needs work

The last submitted patch, 196: pre-render-2099131-196.patch, failed testing.

sun’s picture

Honestly, recent comments in here didn't sound at all very reassuring...

How can we ensure that "Rendering of special case XYZ" won't break due to this patch?

Absolutely none of our current test coverage takes a potential render cache (or for that matter, any possibly existing caching) into account. Existing tests are obviously not guilty — a test author cannot preemptively account for any possible future framework enhancements that might introduce a render cache layer (or not). In addition, (all) integration tests essentially have to be doubled/duplicated; once with enabled caching, and once without, whereas expectations are identical for both.

Committing this pretty much means to hope that everything still works as expected. I didn't look at the patch yet, but in general, I'm certainly in favor of the idea. However, is everyone ready to accept that this will back-fire in the sense of many follow-up issues over the course of the next months?

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new144.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,681 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.05 KB

#198: I understand your concern, but it's not quite that bad. The only test failures fail because

  1. they reuse the same render array instance for different tests, which is not what actual, non-test code does
  2. they use render() rather than drupal_render() (not a single other test does this AFAICT), but that's wrong, because render() should only be used by the theme engine/layer

In other words: this has only uncovered flaws in existing tests, and there's only one of them.

The key problem is that our render + theme system are convoluted and awkwardly intertwined. Most rendering-related tests are also convoluted. We have to make do with what we have. Refactoring those systems towards sanity is a D9 thing.

If there will be follow-up issues, I promise I will handle them.


#196: While your proposal works, I disagree with the fix. In this reroll, I'm fixing things in a "better" way. And to find the cause of that last failure, I'm temporarily removing the comment in CommentDefaultFormatterCacheTagsTest to see what cache tags *are* found.


Finally: why does testbot no longer test the -fail.patch patch files? :(

Status:Needs review» Needs work

The last submitted patch, 199: pre-render-2099131-199.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new145.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,956 pass(es).
[ View ]
new1.79 KB

After two attempts over at #2228043: Helper for #2099131, this patch should be green.

May this be the last reroll!


The cause: run-tests.sh has subtle differences in the logged in user.
Plus, $entity->get('comment_field_name')->comment_count remains at zero. That was masked in #186 and later because the web test runner creates a user that has permission to administer comments. The condition to render comments in CommentDefaultFormatter looks like this:
if ((($entity->get($field_name)->comment_count && $this->currentUser->hasPermission('access comments')) ||
       $this->currentUser->hasPermission('administer comments'))) {

In the web test runner, the comment count was zero but the user was allowed to administer comments, hence comments were rendered.
In the CLI test runner, the comment count was zero, and the user was not allowed to administer comments, hence comments were not rendered, hence cache tags were missing.
I should have loaded $commented_entity again, because only then does comment_count get initialized. So I did that. But then I discovered another flaw: calling $comment->save() should reset the commented on entity's storage cache, but that's not yet happening. I talked to Berdir in IRC, and #597236: Add entity caching to core is already fixing that, so for now doing an entity load with $reset = TRUE, #597236 can then remove that.

In other words: a scary mix of subtle mistakes, subtle environment differences and subtle assumptions… but not in any way related to #pre_render!

jessebeach’s picture

In other words: a scary mix of subtle mistakes, subtle environment differences and subtle assumptions

This issue surfaced so many of these. I'm glad we undertook it. And as Wim mentions, the #pre_render code was a catalyst that precipitated these issues, not a reagent in their production!

I'm profiling the patch in #201 now. I'll post results soonly.

jessebeach’s picture

I found this comment threading issue while setting up the performance cases. It exists on 8.x HEAD, so not introduced by this patch, in case you run into it while testing: #2254181: Comment indentation is incorrect for comments following a replying-comment: don't render cache comments for which threading is enabled.

jessebeach’s picture

Some performance numbers.

Single article

Scenario: single article with additional text, longtext and list fields. The article has 40 comments on it. This test compares the fastest run of 6, warm cache to warm cache. The run1 is 8.x HEAD; run2 is 8.x with the patch from #200 applied. xdebug is turned off at the server level.

Run #536000cd363d6 Run #536000400199f Diff Diff%
Number of Function Calls 168,876 134,399 -34,477 -20.4%
Incl. Wall Time (microsec) 741,002 634,484 -106,518 -14.4%
Incl. MemUse (bytes) 15,101,872 13,291,024 -1,810,848 -12.0%
Incl. PeakMemUse (bytes) 15,386,512 13,567,256 -1,819,256 -11.8%

The #pre_render build postponing results in a 106ms gain in processing time for a single node page.

Next I'll look at a list of nodes on the front page.

jessebeach’s picture

And some numbers for lists of articles:

10 articles, standard teasers

Scenario: 10 articles, standard teasers, 5 with images. This test compares the fastest run of 6, warm cache to warm cache. The first is 8.x HEAD; second run is 8.x with the patch from #200 applied. xdebug is turned off at the server level.

Run #5360100d54519 Run #53601170d4dfe Diff Diff%
Number of Function Calls 94,442 66,871 -27,571 -29.2%
Incl. Wall Time (microsec) 459,598 350,717 -108,881 -23.7%
Incl. MemUse (bytes) 16,772,112 14,725,416 -2,046,696 -12.2%
Incl. PeakMemUse (bytes) 16,799,216 14,742,928 -2,056,288 -12.2%

We get a gain of 108ms which is similar to the single article. I think this is because the entity views are built in a viewMultiple call in the EntityViewBuilder, so the savings for one entity will be the same as the savings for multiple entities in a group.

We start to see savings accrue as we add more stuff to the entities.

10 articles, 3 extra fields and comments

Scenario: 10 articles, enhanced teasers, each with three extra fields displayed (2 text fields and a list field), 5 with images and three articles with comments (one with 30 displayed). This test compares the fastest run of 6, warm cache to warm cache. The first is 8.x HEAD; second run is 8.x with the patch from #200 applied. xdebug is turned off at the server level.

Run #53600e10ce77f Run #53600e7591a4b Diff Diff%
Number of Function Calls 295,931 245,049 -50,882 -17.2%
Incl. Wall Time (microsec) 1,317,463 1,100,828 -216,635 -16.4%
Incl. MemUse (bytes) 21,153,728 19,133,512 -2,020,216 -9.6%
Incl. PeakMemUse (bytes) 21,507,000 19,482,240 -2,024,760 -9.4%

Now we get a savings of 216ms. As the nodes become more complex, the cost savings to build them accumulates.

jessebeach’s picture

Issue summary:View changes
jessebeach’s picture

Issue summary:View changes
jessebeach’s picture

I just did a full review of the code changes again. I think this is it. I'm so psyched about this patch!

jessebeach’s picture

Assigned:Unassigned» catch

All code review points have been addressed in #201. We're green and the perf numbers (these should be verified) look good.

Assigning to catch.

catch’s picture

Numbers looks great. I found a few nitpicks but it's mostly in tests at this point.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentDefaultFormatterCacheTagsTest.php
    @@ -0,0 +1,115 @@
    +    $this->assertEqual($build['#cache']['tags'], $expected_cache_tags); //, 'The test entity has the expected cache tags when it has comments.');

    Assertion message is commented out, we could just delete that, more useful to have the two arrays in the test output.

  2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
    @@ -93,9 +93,20 @@ public function viewElements(FieldItemListInterface $items) {
    +        // renders *another* entity (i.e. an entity within an entity). That
    +        // other entity comes with its own #pre_render callback that must be
    +        // called in order for the cache tags associated with that other entity
    +        // to be included with those of the main entity. Hence we must render
    +        // the other entity *immediately* (which includes running its
    +        // #pre_render callbacks), otherwise its associated cache tags won't
    +        // bubble up.

    We still have #2168111: Allow drupal_render() to pass up #attached to parents open but I think there's another issue somewhere for the way that #attached and cache tags go missing in preprocess. If I can't find it, worth creating one for this I think. The comment here should try to explain the theme() problem a bit more IMO, it's not clear from the comment why this is necessary.

  3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -342,6 +343,53 @@ protected function drupalCreateContentType(array $values = array()) {
    +   * functions in order to maximize cache efficacy. This means that the full
    +   * rendable array for an entity is constructed in drupal_render(). Some tests
    +   * require the complete renderable array for an entity outside of the
    +   * drupal_render process in order to verify the presence of specific values.
    +   * This method isolates the steps in the render process that produce an
    +   * entity's renderable array.

    I think we probably need a follow-up to fix the tests with these assumptions. It ought to be possible to have the tests check the rendered markup instead or similar.

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
    @@ -491,19 +491,22 @@ function testLanguageFallback() {
    -    $this->assertIdentical($entity2, $translation, 'When the entity has no translation no fallback is applied.');
    +    // @todo, this assertion requires rendering.
    +    //$this->assertIdentical($entity2, $translation, 'When the entity has no translation no fallback is applied.');

         // Checks that entity translations are rendered properly.
         $controller = $this->entityManager->getViewBuilder($entity_type);
         $build = $controller->view($entity);
    -    $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[$current_langcode]['name'], 'By default the entity is rendered in the current language.');
         $langcodes = array_combine($this->langcodes, $this->langcodes);
         // We have no translation for the $langcode2 langauge, hence the expected
         // result is the topmost existing translation, that is $langcode.
         $langcodes[$langcode2] = $langcode;
         foreach ($langcodes as $desired => $expected) {
           $build = $controller->view($entity, 'full', $desired);
    -      $this->assertEqual($build['label']['#markup'], $values[$expected]['name'], 'The entity is rendered in the expected language.');
    +      // @todo, this assertion requires rendering.
    +      //$this->assertEqual($build['label']['#markup'], $values[$expected]['name'], 'The entity is rendered in the expected language.');

    Looks like these still need to be resolved?

jessebeach’s picture

StatusFileSize
new145.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,252 pass(es).
[ View ]
new7.75 KB

#210:1

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentDefaultFormatterCacheTagsTest.php
@@ -0,0 +1,115 @@
+    $this->assertEqual($build['#cache']['tags'], $expected_cache_tags); //, 'The test entity has the expected cache tags when it has comments.');

I don't see this commented out code in the file adding in the patch from #201. This is the patch file I'm reviewing, just so we're on the same page: https://drupal.org/files/issues/pre-render-2099131-201.patch

#210:2

but I think there's another issue somewhere for the way that #attached and cache tags go missing in preprocess.

This is currently "works as designed". We moved the processing attachments above theme processing in #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses. This was done so that attachments could be added to top-level elements such as html. Before this change, #attached was processed after theming had taken place, which meant that pre_/process function could add #attached to the render array and have them attached. But in the case of a top level element like html, the output string would be rendered and thus, none of the attachments would be included because the string ship had already sailed. So now we process all #attach before theming, which means you can't alter the build array with #attached and have them injected into the output; you need to (1) run the #attached array through drupal_render() in place or (2) as the table type does, add #attached in a #pre_render.

#cache is processed after theming, so pre_/process functions can still alter a build array to add these. But I would tell any themer to avoid this. because we won't be able to retrieve any cache tags added in this manner when we do cache entry check at the top of drupal_render().

I hope I understood your comment correctly :)

I've reworked the comments in the EntityReferenceEntityFormatter::viewElements() and the CommentDefaultFormatter::viewElements() methods.

#210:3

Added #2255031: Refactor tests to compare rendered output rather than values in build arrays.

#210:4

I've turned the tests back on with a few changes to render the entities before asserting values.

Status:Needs review» Needs work

The last submitted patch, 211: pre-render-2099131-211.patch, failed testing.

The last submitted patch, 211: pre-render-2099131-211.patch, failed testing.

jessebeach’s picture

Nothing I did in #211 should be causing LanguageFallbackTest to fail so either something changed in HEAD that our patch is undoing or testbot is being wonky.

I also can't reproduce the test failure locally; LanguageFallbackTest passes just fine. So I'm inclined to believe that testbot is being wonky.

catch’s picture

Status:Needs work» Needs review

From the interdiff there's still a commented-out assertion message.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
@@ -490,23 +490,32 @@ function testLanguageFallback() {
+      $this->assertEqual($build['label']['#markup'], $values[$expected]['name']);//, 'The entity is rendered in the expected language.');

Otherwise the changes look good. I don't think I have anything left and looks like patch was green after a retest.

Could fix that nitpick on commit so leaving CNR so there's a chance for others to look. I don't think I can commit this without at least an in-depth review from someone else since I've been pretty heavily involved. I'll try to beg Alex Pott for one.

Opened #2255543: Cache tags of nested entity renders are not bubbled up for the nested entity render issue - not introduced here and that's a tricky one.

jessebeach’s picture

StatusFileSize
new145.59 KB
new145.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,243 pass(es).
[ View ]

Ack! Leftover from debugging. I added the string back to the assertion.

catch’s picture

Assigned:catch» alexpott
Status:Needs review» Reviewed & tested by the community

OK I think this is ready, or at least I've looked at it too much to spot any other issues.

Moving to RTBC, assigning to Alex Pott to review/commit since I've worked on this a bit too much to be neutral.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

I just looked at this and the code looks great. One of the best issue summarizes I've seen in a while, and I absolutely love the performance results. Great work. Committed to 8.x.

  • Commit 99b367d on 8.x by Dries:
    Issue #2099131 by jessebeach, Wim Leers, catch, Berdir, benjifisher,...
effulgentsia’s picture

Great work, everyone! I'll have several clean up follow ups to open. Here's the first: #2257421: Convert CommentDefaultFormatterCacheTagsTest from web test to entity unit test.

yched’s picture

As per #150-2 :

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 ?

Patch posted in #2258091: Reorder methods in EntityViewBuilder.

Wim Leers’s picture

Issue tags:-sprint
Wim Leers’s picture

Xano pointed out that the #post_render_cache change notice was still taking about the render_cache_placeholder element type over at https://drupal.org/node/2151609, even though that was removed in this issue. Updated the change notice to reflect the new situation/best practices introduced here. See https://drupal.org/node/2151609/revisions/view/6762699/7240115.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.