Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render

I started looking at this, because we're seeing this problem in the menu render caching patch (#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees), in the "messages as a block" patch (#2289917: Convert "messages" page element into blocks) and will quite likely affect #507488: Convert page elements (local tasks, actions) into blocks as well.

I updated the issue title and summary to reflect a very important part of the problem, that wasn't very explicitly present before: the problems described in this issue happen only because Drupal 8 leverages the #pre_render pattern much more than Drupal 7! In Drupal 7, it was only used for minor things. In Drupal 8, it's used for very big things, e.g. for blocks and entities, to allow us to do the very least amount of work possible on a cache hit.

Consequently, in Drupal 8, the render array (tree) for the page is relatively sparse: it doesn't contain all the information yet. In Drupal 7, it contained everything.

Combine that with the fact that whatever is passed to the theme layer (theme() in D7, _theme() in D8) gets passed by value, which means that #pre_render callbacks invoked by drupal_render() calls made in the theme layer will not affect the original (root) render array that represents the entire page. Consequently, any cache tags, assets or #post_render_cache callbacks added in those #pre_render callbacks will not be available to the "root" drupal_render() call (the one that renders the entire page), and therefore those cache tags, assets and #post_render_cache callbacks will be "lost". (Well, assets will work fine due to an implementation detail: they still use a global static, but we've worked to get rid of that).

And finally, in Drupal 7, we had (almost?) no render caching in Drupal core, and only rarely used #pre_render for simple things, so we simply didn't notice this problem.

Therefore, the solution to the problem is actually quite simple: we must make sure that the "page-wide" render array is the full render array, i.e. with all #pre_render callbacks executed.

This patch does that.

In writing this patch, I also:

• removed all the hacks that were introduced by #2099131: Use #pre_render pattern for entity render caching and other issues!
• discovered that the loading of defaults is kind of broken: we never set #defaults_loaded, so it's possible that defaults are loaded twice. Previously, any render array was only touched once, now it may be touched twice (first for executing #pre_render callbacks to ensure it's fully built, the second time for actual rendering).

I wonder why the test-only patch was not reviewed by testbot? Testbot--

Uploaded the same "test-only" patch, and fixed the actual patch.

#12:

RE: hiding patch file on initial post: oh… HAH! That'd explain it. Attaching the test-only patch just once more then, without hiding it. Thank you very much for the tip!

RE: to the patch itself: I don't like it much either (in general, there's a lot to be disliked about drupal_render()), but it's the only solution I can think of.

RE: need to think about […]: That will work just fine; #post_render_cache callbacks performing rendering use drupal_render($build), not drupal_render($build, TRUE) (which must be used for recursive calls, e.g. drupal_render() does this itself internally), which means you'll get a fully rendered result back, which includes performing any #post_render_cache callbacks.

Sorry for the noise here. Re-uploading the test-only patch that will fail (as shown in #13) and the actual patch up for review that will pass (as shown in #11). Renumbering them to remove all confusion, but they're effectively unchanged from above.

This should leave the issue at the "needs review" status, because, yes, this is now blocked on your reviews!

Sigh, does the order of those patches matter also? This is definitely NR, not NW.

#19: everything you reviewed is copy/pasted existing code in drupal_render(). So I'm afraid all three points are out of scope… Sorry!

#21: Fine, fixed #19.1 and #19.3 — those are simple enough changes: it's clear that changing those won't have consequences. Fixing #19.2 can easily have consequences though, so please see #2323301: Deprecate the "two double colons" check in Renderer::doCallback() for that.

Having addressed #19/#21, let's now get back to architectural reviews.

The main concern I had with the patch in #15 is that it duplicated logic, instead of moving logic. DRY and all that. So I spent some time cleaning that up. In this reroll:

1. Three changes that together have allowed me to remove code duplication:
   1. drupal_render() no longer loads #type defaults nor applies #pre_render callbacks. Those are tasks for _drupal_render_ensure_fully_built(), because those are the two crucial things for "fully building a render array".
   2. drupal_render() now only handles the render cache HIT case for the non-recursive ("root") call to drupal_render(), because in this case, there is no need to fully build the render array, since the full render array has a cache hit. When the full render array has a cache hit, we must also apply all #post_render_cache callbacks. This is not the case for render cache hits for render cache hits at any child/descendant level of the render array. (We always only apply #post_render_cache callbacks at the very latest moment possible, i.e. when the full render array has been built, which happens immediately when we have a render cache hit for the full render array.)
      Sadly, it's possible for theme preprocess functions to generate render arrays (or forms), which then won't have been accessible during the non-recursive ("root") call to drupal_render(), and hence they will not be fully built. Hence we not only have to call _drupal_render_ensure_fully_built() during the non-recursive ("root") call to drupal_render(), but also if we see in recursive ("non-root") calls to it that either #defaults_loaded is not set or #pre_render still exists (_drupal_render_ensure_fully_built() would have taken care of that, if this render array had not been added in a theme preprocess function).
      (This happens in e.g. table rendering, View rendering, but likely also in other places.)
2. The work-around to render summaries in TextTrimmedFormatter has now been completely removed; no more stray drupal_render() call there!
3. WebTestBase::drupalBuildEntityView() has now been simplified: it simply calls _drupal_render_ensure_fully_built() rather than applying defaults and executing #pre_render callbacks itself. (Which was also implemented incorrectly, for it only did this at the "entity level", it did not recurse down to the "field level".)

Just to make sure that this doesn't cause a performance regression, I did some profiling.

Setup:

1. node 1, with a title, body and two taxonomy terms
2. render caching disabled
3. authenticated (admin user), so also a toolbar being rendered
4. PHP 5.5.11, OpCache, XHProf

Loading node/1, best out of 10 runs:

As you can see, essentially status quo. Slightly more function calls (which is only logical, given that we move part of what drupal_render() used to do in a new helper function). Slightly faster. Slightly more memory (this must be because due to the changes in text summary rendering, because no additional work is being done). Overall, essentially the same performance.

Thanks for the review yched!

Trimming moves to pre_render in order to avoid needless computation on cache hit, right ? Might be worth a comment ?

Yes. But to be completely honest, I didn't do it for that purpose intentionally: the way the formatter worked today, it needed #markup to contain the filtered text, because otherwise text_summary() couldn't be applied to it. That meant I had to run drupal_render() in a place where it didn't belong — regardless of performance considerations.
But, of course, a drupal_render() that doesn't belong really is just another way of saying "is an indication of needless computations on cache hits"/"is bad for performance".
I could add a comment, but I don't think it's necessary: any drupal_render() invocations outside code building responses are a code smell. If you prefer, I don't mind adding that comment though :)

could we add that callback as a static method on the formatter class rather than as a new procedural function ?

Absolutely! Done.

- nothing is done for the "summary_or_trimmed" formatter ?

Indeed, because there's nothing to do:

class TextSummaryOrTrimmedFormatter extends TextTrimmedFormatter { }

because

/**
 * Plugin implementation of the 'text_trimmed' formatter.
 *
 * Note: This class also contains the implementations used by the
 * 'text_summary_or_trimmed' formatter.
 *
 * @see \Drupal\text\Field\Formatter\TextSummaryOrTrimmedFormatter
…
 * )
 */
class TextTrimmedFormatter extends FormatterBase {

(Yes, that is confusing!)

Walked moshe through my patch, addressed his feedback in this reroll:

1. cleaned up my changes to drupal_render(), so that it's now very clear what happens during the root call, and what happens in other cases.
2. in the test, renamed children to foo, to avoid any initial confusion by reading children as #children
3. removed the hack to render() that was introduced in #2099131: Use #pre_render pattern for entity render caching

#27:

Great questions! I'm afraid proper answers are long by necessity. The short version:

• We don't want to support preprocess functions creating render arrays. Because if the application logic isn't aware of what is going to be rendered, then how could it possibly invalidate the render cache correctly? Plus: 1) there is very, very little code left that does this, so we just want to fix those, 2) we've been working towards removing the preprocess layer, haven't we?!
• #1 can't work because drupal_render() has no sense of hierarchy. #1 can only work if a recursive call knows/receives the state of the parent call, but because the recursive calls go through the theme system and hence (have to) operate on copies of render arrays, it is impossible to send hierarchy/state to child calls. Therefore, render array subtrees that have render caching enabled may get the wrong cache tags. The page will indeed receive the correct cache tags, but that would always be the case when using a static: both the page and the static are "request-wide".

Besides the two above points that show this patch is complete, this does not break anything, so if #1 turns out to be possible after all, it can be done in a follow-up.

This issue is a hard blocker for the critical performance issue #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, so it would be great to get that finally unblocked.

1. Isn't the problem that a preprocess function can create a render array that a Twig file can print?

   (Emphasis mine.)
   Great catch! But AFAIK we decided a long time ago that we don't support this? We say we don't support attaching assets in theme preprocess functions. (Precisely because we cannot bubble attached assets up from preprocess functions; assets belong in the "content structure" (render) layer, not in the "printing" (theming) layer).
   That is, we say that the render array must contain the full structure of the content. Because… if the application logic isn't aware of what is going to be rendered, then how could it possibly be held responsible for invalidating it correctly?
   Note that we specifically want to get rid of the preprocess layer, which will also allow us to remove the cited code block: #2060783: Remove the preprocess layer..
   And last but not least, I went through all HOOK_preprocess_* functions and I only found a handful remaining ones that are problematic (all others have been fixed), which indicates how far we've come with eradicating this bad practice from the past. See [1] below.

2. As per the above: no, you're right, this does not add support for bubbling up stuff for render arrays created in preprocess functions. But as I explain above, that's broken by design anyway, and we've been working to get rid of that. This patch does support bubbling up stuff for render arrays rendered (but not created) in preprocess functions).
3. (RE: Additionally, […]) Wow. :D
   Setting #access does not belong in preprocess functions. That's once again application logic. If the render array does not contain the "truth" about which things are going to be rendered and which won't, then we will collect the wrong cache tags as well. Setting #access belongs in #pre_render/#process/alter hooks (e.g. file_managed_file_pre_render(), filter_process_format(), seven_form_node_form_alter()).
   Setting #access in a preprocess functions happens exactly zero timesin HEAD.
   Hence this is a non-issue.
4. (RE: […] re-evaluate the idea in #1 […]) I don't think what's proposed in #1 can work. Let me explain.
   

   For the latter, we could introduce a temporary static inside drupal_render(), and populate it with #attached/#cache information (probably only when $is_recursive_call is true). Then when drupal_render() is called $is_recursive_call = false, reset that static because we can assume the #attached/#cache is getting passed up properly from that point onwards.

   #1 only offers a correct result for the overall page rendering: all assets, cache tags and #post_render_cache callbacks are collected in a global static, and that static applies to the page level. That's doable and easy. A static is an easy way to achieve that. But the picture becomes much bleaker when we take into account that parts of the page may be render cached as well, which is the case.

   Let's take a simple example:

   Page
    |- Block A
    |- Block B
    |- Node: node:4
        |- Author: user:1
        |- Tags: term:1 term:2
        |- Entity reference: node:1
            |- Author: user:2
            |- Tags: term:3
    |- Block C
    |- Block D
   

   Let's say for the sake of simplicity that Blocks aren't render cached nor have cache tags, only Node, Author (user entity) and Entity reference (another node entity) are render cached and have cache tags. The above tree representation is already sorted by #weight.

   Let's call "the static" S. S is a set. Initially, S is empty. (S = {}).

   Facts

   • drupal_render() will have $is_recursive === TRUE for every element in the tree, except for the drupal_render() call for the Page. This means that
     

     Then when drupal_render() is called $is_recursive_call = false, reset that static because we can assume the #attached/#cache is getting passed up properly from that point onwards.

     is unfortunately wrong AFAICT.

   • A drupal_render() call does not have any context/hierarchy information.
   • To render the node, first Author is rendered, then Tags, then Entity reference.
   • drupal_render() renders elements in the tree in sequence, and depth-first.

   Render flow

   1. First, Author is rendered and render cached. This happens correctly. Now S = { user:1 }.
   2. Second, Tags is rendered (and not render cached). Now S = { user:1, term:1, term:2 }. This is still correct.
   3. Third, Entity reference is rendered, which consists of several substeps:
      1. The first step is to render its Author. This is rendered and render cached. Assuming the render caching doesn't use S here, the render cache item contains the correct information. But now S = { user:1, term:1, term:2, user:2 }.
      2. The second step is to render its Tags, which gets rendered without render caching. Now, S = { user:1, term:1, term:2, user:2, term:3 }.
      3. Finally, we can render cache the Entity reference field's output. S = { user:1, term:1, term:2, user:2, term:3, node:1 }. But S contains cache tags from the containing node! So the cache tags are WRONG!
   4. We've now rendered all fields of Node, and now we're rendering and render caching Node itself. S = { user:1, term:1, term:2, user:2, term:3, node:1, node:4 }. Node contains the correct information, and bubbles up to Page.

   I think this sample flow shows how #1 cannot work, because the render system does not have any sense of place/hierarchy.

   With #1, correct bubbling/metadata cannot be guaranteed for nested render cache items, only for the page-level render array.

[1]: the remaining HOOK_preprocess_*() functions that either call drupal_render() or add a new renderable array:

1. template_preprocess_table()
2. template_preprocess_comment()
3. template_preprocess_file_widget_multiple()
4. Three in forum.module (unsurprisingly so, this module has gotten little love this cycle, just like other cycles)
5. template_preprocess_node()
6. template_preprocess_node_preview()
7. Views — because Views has so far always used string-passing in its render pipeline, rather than using render arrays when rendering HTML. Being fixed at #2323335: [meta] Improve Views' caching, #2183017: Views doesn't bubble up rendered entities' cache tags.

And a few call drupal_render() just to attach assets (which happens to still work because we still use a global static under the hood, but we've been working towards removing that):

For assets/drupal_add_html_head*:

• template_preprocess_maintenance_page()
• template_preprocess_book_navigation()
• seven_preprocess_install_page()
• seven_preprocess_maintenance_page()

#29: This patch does not change anything WRT preprocessing. It just doesn't fix it (it is already broken in HEAD from a bubbling POV). You can still render things just fine in the preprocess layer (HEAD does that too), but with one big caveat: cache tags and #post_render_cache callbacks will not work. If I'm right in #28, there is literally no way around that. But that means as long as you're adding fairly simple things in preprocess functions (that don't need either cache tags or #post_render_cache callbacks), you'll be fine. Note that I've explicitly omitted assets: because assets still use a global static internally, hence attaching assets from preprocess functions continues to work as long as D8 keeps that implementation detail.

Short update: @effulgentsia is AWESOME! We've been chatting all afternoon, and we've (well, mostly he) figured out how to make a stack-based approach work. Most crucial tests are passing already. We'll post here when we have a working patch!

An alternative solution: the stack-based approach

I'm providing a condensed version of the conversation I had with @effulgentsia.

I asked effulgentsia how the stack would then yield correct results in the following example:

Assume this tree:

Node
 |- FA (Field A)
 |- FB (Field B)
 |- FC (Field C, will be render cached because it's an entity reference field that uses the "Rendered entity" formatter)
   |- NA (Nested Field A)
   |- NB (Nested Field B)
 |- FD (Field D, will be render cached, because it's a user reference field)
 

drupal_render() will render these in this order:

Node (7)
 |- FA (1)
 |- FB (2)
 |- FC (5)
   |- NA (3)
   |- NB (4)
 |- FD (6)

To which he answered:

This is how I envision the evolution of the stack (#attached is short-hand for all things that bubble: i.e., #attached + #cache['tags'] + #post_render_cache):

1. Node (empty)
2. Node (empty), FA (empty)
3. Node (empty), FA (with FA's #attached)
4. Node (with FA's #attached)
5. Node (with FA's #attached), FB (empty)
6. Node (with FA's #attached), FB (with FB's #attached)
7. Node (with FA's and FB's #attached)
8. Node (with FA's and FB's #attached), FC (empty)
9. Node (with FA's and FB's #attached), FC (empty), NA (empty)
10. Node (with FA's and FB's #attached), FC (empty), NA (with NA's #attached)
11. Node (with FA's and FB's #attached), FC (with NA's #attached)
12. Node (with FA's and FB's #attached), FC (with NA's #attached), NB (empty)
13. Node (with FA's and FB's #attached), FC (with NA's #attached), NB (with NB's #attached)
14. Node (with FA's and FB's #attached), FC (with NA's and NB's #attached)
15. Node (with FA's and FB's #attached), FC (with NA's, NB's, and FC's #attached)
16. Node (with FA's, FB's, NA's, NB's, and FC's #attached)
17. Node (with FA's, FB's, NA's, NB's, and FC's #attached), FD (empty)
18. Node (with FA's, FB's, NA's, NB's, and FC's #attached), FD (with FD's #attached)
19. Node (with FA's, FB's, NA's, NB's, FC's, and FD's #attached)
20. Node (with FA's, FB's, NA's, NB's, FC's, FD's, and Node's #attached)

So, push an empty placeholder onto the stack at the beginning of drupal_render(). Then populate that item with #attached after the thing is rendered. Then at the very end of drupal_render(), merge the contents of the current stack item to the one before it and pop it.

I prototyped it… and it worked!

The approach I've used in all patches so far has one big advantage: debuggability. It becomes much easier to debug render arrays, because the render array is fully built at the very beginning, rather than only a single level at a time, and potentially on a copy of a render array in a template.

However, there are three even bigger advantages to the stack-based approach:

1. it explicitly supports bubbling of cache tags, assets and #post_render_cache callbacks even from the theme preprocess layer. So those kinds of one-offs will work just fine. Of course it still won't be recommended, because the structure should live in the render array/application layer, not in the theme layer.
2. if the template doesn't render something (e.g. it doesn't render a taxonomy term field of a node), then its cache tags also won't bubble (assuming the cache tags on the maybe-maybe-not-rendered-thing are set in a #pre_render callback)! This is an improvement compared to HEAD, because HEAD will associate all cache tags, even if something is not actually printed. After all, if the markup for something isn't present, then we shouldn't invalidate a page when that something changes!
3. Massive clean-up/simplification of the current bubbling-handling logic. All of the following can be deleted:
   1. drupal_render_collect_attached()
   2. drupal_render_collect_post_render_cache()
   3. drupal_render_collect_cache_tags()

   … and it so happens that these were responsible for a lot of function calls, recursing down trees to collect needed data. Especially the one for #post_render_cache was expensive, because it even had to do render cache gets for correct results. All of that complexity is now *gone*!

Concrete example of bubbling from the theme layer

Let me give one concrete example. HEAD currently does this in template_preprocess_node():

  if ($variables['display_submitted']) {
    if (theme_get_setting('features.node_user_picture')) {
      // To change user picture settings (e.g. image style), edit the 'compact'
      // view mode on the User entity. Note that the 'compact' view mode might
      // not be configured, so remember to always check the theme setting first.
      $variables['author_picture'] = user_view($node->getOwner(), 'compact');
    }
  }

The cache tags for this rendered user entity were never bubbled before. But now they are!

Important consequence #attached bubbling fixed!

Also quite important: because the bubbling of attached assets is now working correctly, #2168111: Allow drupal_render() to pass up #attached to parents is in fact fixed. To prove that it's indeed fixed, this patch should (and does) remove the call to drupal_process_attached() in drupal_render().
Why? Because that call caused everything to seem to "work just fine" for assets, thanks to the global statics in _drupal_add_css() and _drupal_add_js() (which are called by drupal_process_attached()). But it actually causes assets to "load just fine" on cache misses (because of the global statics), but utterly fail on cache hits (because the code that caused the global statics to be updated wouldn't run, precisely because it's a cache hit).
Removing drupal_process_attached(), _drupal_add_js() and _drupal_add_css() in this patch would be a huge scope expansion, so we still call drupal_process_attached()… but only after entire the entire render array has been rendered! I.e. in DefaultHtmlFragmentRenderer and friends. This is a huge step forward for asset handling, is something we've been working towards for more than a year, and makes #1762204: Introduce Assetic compatibility layer for core's internal handling of assets much more doable.

This patch versus previous patches

This patch uses the same test coverage as added by previous patches, it only changes the solution (completely). The key changes live in common.inc, just as before.

I updated drupal_render()'s docs as requested in #38.

In doing so, I found one small bug: if a #pre_render callback added bubbleable metadata (e.g. cache tags) and set #printed = TRUE, then the bubbling wouldn't have worked. There is nothing in core that does this, and it just makes the handling of bubbleable rendering metadata in the "early return if #printed, abort rendering" case exactly the same as in the "render cache HIT, abort rendering" case, so I don't think additional test coverage is necessary. This small change only applies the same pattern consistently.

#37, #38: thank you for the kind words! :) But as effulgentsia says, this also wouldn't have possible without the initial idea of Fabianx!

Suggested commit message:

Issue #2273277 by Wim Leers, effulgentsia, Fabianx: Fixed Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render.

Chasing HEAD.

Now profiled the stack-based solution. Results are in line with the expectations: more function calls (for updating the stack at every point along the way, but these are simple, fast function calls), similar memory consumption, but the more complex/the bigger the render array, the bigger the CPU time improvement.

Tested with PHP 5.5.11, OpCache, 5 article nodes with only their title and body fields filled in. All tested with the root user (UID 1), so includes rendering of the toolbar.

Front page

	Run #head-front	Run #patch-front	Diff	Diff%
Number of Function Calls	86,491	90,719	4,228	4.9%
Incl. Wall Time (microsec)	298,336	295,764	-2,572	-0.9%
Incl. MemUse (bytes)	22,188,376	21,523,560	-664,816	-3.0%
Incl. PeakMemUse (bytes)	22,330,608	21,691,360	-639,248	-2.9%

Node 5 (including a comment form that's rendered via #post_render_cache

	Run #head-node-comment-form	Run #patch-node-comment-form	Diff	Diff%
Number of Function Calls	92,990	97,365	4,375	4.7%
Incl. Wall Time (microsec)	321,396	310,709	-10,687	-3.3%
Incl. MemUse (bytes)	22,178,944	22,180,232	1,288	0.0%
Incl. PeakMemUse (bytes)	22,360,136	22,364,448	4,312	0.0%

/admin/config/development/testing (the most expensive/slowest to render page we have in Drupal core)

	Run #head-tests-list	Run #patch-tests-list	Diff	Diff%
Number of Function Calls	1,405,705	1,480,192	74,487	5.3%
Incl. Wall Time (microsec)	6,667,029	5,128,305	-1,538,724	-23.1%
Incl. MemUse (bytes)	44,571,800	44,364,968	-206,832	-0.5%
Incl. PeakMemUse (bytes)	86,890,080	86,164,864	-725,216	-0.8%

Conclusion: a small performance improvement for simple pages, a big improvement for complex pages. This patch simplifies rendering, and hence makes it faster.

We can still try to optimize the performance of the stack bubbling in the future further (and hopefully improve performance further), but the current patch is good to go from a performance POV.

#46: very glad to read that! :)

#48: That's correct! But this is a very complex page, you won't see such big improvements on page loads. Nevertheless, nice consequence of simplification :)

Hurray :)

@catch: very glad to see your approval :)