Updated: Comment #0

Definition of "personalization"

There are many kinds of personalization. Personalization is based on one or more contexts: user, geographical location of the user, time, a specific node ("X new comments" on this node for user X), organic group, and so on.

Let's look at an example. Usually in Drupal, a certain page contains some content that is the same for everybody (e.g. viewing node 2118703 on d.o), but some parts of the page are unique for each user (e.g. the comment "new" markers). Because the comment "new" markers are different for every user (and even different on every page load for every user), this means that we cannot cache the resulting HTML. If we would, then the same and hence wrong "new" marker would be shown to each user. So that means we have to generate the *entire* page for each user on every single page load. That is very expensive. The consequence is that each page load is slower, because the entire page has to be built, there is not a single blob of HTML that can be cached and reused.

That is a very specific example, but it's a problem that we frequently encounter in Drupal core. We have the comment "new" and "updated" markers. The "X new comments" links on node teasers. The "edit" link on comments that is user-specific when "edit own comments" permission is enabled. Contextual links are page-specific (due to the ?destination=<current path> query string). Some kind of "Login block" on many sites (see the "Logged in as ." at the top of this page if you're logged in). And so on, and so on.

So, effectively, we're saying "screw caching" in much of Drupal core. Often, the personalization is nested somewhere deep in something that's otherwise perfectly cacheable (e.g. the "edit" links on comments is user-specific, but the rendered node is not; because comments are rendered inside of the node, this prevents any caching of nodes)!

That's what we're trying to solve.

Roughly, there are two types of personalized content:

  1. Primary content (e.g. the rendered blog post node)
  2. Secondary content (e.g. "new" content marker on comments, contextual links metadata, comment "edit" links, toolbar menu tree …)

For primary content, it's essential that it's part of the delivered HTML: otherwise the user would have to wait for it to show up (which is bad UX and bad perceived performance). For a lot of secondary content (like the ones above), it is okay for there to be a slight delay for it to show up. Then there are things in between, such as a "related content" block and a "navigation" block. For those, it may or may not be acceptable to show up with a delay. It depends on the site.

Besides that, there is a personalization data size concern: does the personalization data require a lot of data (which is probably always true for primary content) or very little (which is often but not always true of secondary data: it's not true for the toolbar menu tree). Because of the greatly varying sizes of different personalized things, different delivery mechanisms may be suitable.

Problem/Motivation

Short problem description

In the current state of drupal_render() & the render cache, any personalization breaks the render cache, unless you do it in JavaScript.

Full problem description

The only way to personalize and not break the render cache with the current API, is to implement a solution like the one in #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user:

  1. embed universal truths as data- attributes in the HTML
  2. attach JavaScript that will do the personalization
  3. let the JavaScript make requests to retrieve user-specific data if necessary
  4. apply heuristics and client-side caching (localStorage/sessionStorage) to minimize the number of HTTP requests

However, for some sites/hosting setups/use cases, this is bad: some sites prefer to avoid the HTTP requests and embed the necessary data in the page itself, yet other sites prefer to avoid JavaScript altogether.

So far, no other solution was possible, because we didn't have any way to do react to loading data from the render cache.

Proposed resolution

Short proposed resolution description

Introduce #post_render_cache callbacks.

Requirements

Any solution must meet these six requirements:

  1. Conceptually easy to reason about. In other words: sufficiently simple DX. (Points 2, 3 and 4 aid in this.)
  2. It should be possible to use the same mechanism to either replace a placeholder in the markup, or attach additional JavaScript settings/libraries, or both.
  3. Must continue to work when multiple render cacheable things are merged into one (i.e. #cache is set on an element, but also on its child or grandchild or …). In other words: nested #post_render_cache callbacks must continue to work even when they're no longer nested, after having been stored in/retrieved from the render cache
  4. Even when not using render caching (i.e. an element that does not have #cache set), #post_render_cache callbacks must be executed. This allows (contrib module) developers to inject/alter render arrays with personalized data without breaking the render cache, and more importantly, without having to implement the same functionality in two ways: one implementation for when render caching is disabled, another for when it is enabled.
  5. Must have a unique/random identifier for each placeholder, to guarantee the #post_render_cache callback will never accidentally replace user-generated content.
  6. The default solution should be optimized to work out-of-the-box on any hosting. Roughly, there are two broad types of hosting: shared hosting and enterprise hosting (with a reverse proxy such as Varnish in front of the web servers). There are more types, but these are the extremes. Drupal core should accommodate both cases out of the box, and if that is impossible, it should favor shared hosting.

Three desirable "personalized page" delivery mechanisms

After careful deliberation and prototyping, I've come up with three "personalized page" delivery mechanisms that are needed to cover all use cases and requirements.

Two out of three are impossible today.

They can all be implemented using a single new #post_render_cache callback!

Where "personalized page" is defined as a page that contains something-specific (user-specific, request-specific, end user location-specific …) content.

Finally: a soft assumption is that in the final version of Drupal 8, JSON/AJAX requests will not require a full bootstrap, i.e. will be cheaper than HTML requests.

1. "Only HTML"

At first sight, this is the most desirable delivery mechanism: no JavaScript!

In a nutshell: store placeholders in the render cache, then have some kind of #post_render_cache callback that receives the rendered HTML, finds the placeholders and replaces them with the personalized content.
This is the only mechanism that is acceptable for primary content (e.g. rendered nodes), because it guarantees the personalized content is available as soon as the user sees the page. It is suitable for large chunks of content.

Once we start thinking it through, some significant downsides arise:

  1. Inherently incompatible with a reverse proxy-based setup (Varnish, CDN …), because any page that contains personalized contents.
  2. Inherently less scalable/higher server hosting cost, because each page needs to be completely built on the server side.
  3. Inherently higher minimum page load time, because the server inherently needs more time to generate the page (to find and replace the placeholders).

So, while this seems better in general, it in fact is worse for the majority of sites out there. On shared hosting, sites would be more likely to fail when slash dotted. On enterprise hosting, the reverse proxy is barely effective.

Only when a number of highly specific requirements are met, this is the better delivery mechanism:

  1. The site has already been optimized very heavily for performance: it contains only the essential JavaScript, has very few images, the theme removes all Drupal contrib modules' CSS and reimplements it and loads in <1 s on powerful devices connected to fast networks.
  2. Your site has very important visitors that access the site via high-latency networks on low-CPU power devices and you need to serve the page in <1 s on these devices as well.
  3. Increased server hosting cost is acceptable.

You see, unless you already fulfill requirement number 1, you probably wouldn't even see the benefit of going "only HTML". Without requirement number 2, there is no compelling reason to go "only HTML". And requirement number 3 is a consequence.

IMHO it's clear that this is a bad default for Drupal. On the other hand, it is crucial that we at least support it.

2. JavaScript + HTTP requests + localStorage/sessionStorage

This is the mechanism we've used to prevent these from breaking the render cache (because it's the only method supported by Drupal core today):

In a nutshell: personalized content is hidden or non-personalized initially, and contains a placeholder or identifier. Some #attached JavaScript then finds these placeholders or identifiers and renders the personalized content. To do that, it may have to either talk to localStorage (fast & cheap) or … perform a HTTP request to talk to the server (slow & expensive).
This mechanism is not acceptable for primary content (e.g. rendered nodes) because it may depend on a round trip to the server (and hence network latency) to retrieve the content. Hence it should only be used for secondary content (e.g. "new" content marker). It is suitable for large chunks of content. It is also suitable for metadata or tiny chunks of content.

The downside is extremely obvious: HTTP requests. This keeps a mobile device's antenna awake longer than necessary, thus draining the battery more. Not only that, but on every device, more HTTP requests implies a slower website — though especially on a mobile device.

However, the upside is that we can leverage heuristics and client-side caching to avoid letting the server doing any work at all: only talk to the server when heuristics don't allow you to avoid doing so, and even in that case, first check if the needed data isn't already inlocalStorage/sessionStorage to avoid HTTP requests. When done well, this can actually decrease server load.

It is much better than "only HTML" for enterprise hosting, because this is inherently compatible with reverse proxy-based setups.

The big downside of this approach is not just "HTTP requests" but that if you have multiple personalized things on the page, that results in multiple HTTP requests. Particularly for shared hosting, that could become problematic: they're typically less optimized, which means that the minimum response time will be higher, resulting in less-than-stellar perceived performance and higher server load.

3. JavaScript + drupalSettings

In a nutshell: personalized content is hidden or non-personalized initially, and contains a placeholder or identifier. The attached JavaScript then finds these placeholders or identifiers and renders the personalized content. To do that, it merely has to look the data in drupalSettings, which were added to the page's <HEAD> using some kind of #post_render_cache callback.
This mechanism is not acceptable for primary content (e.g. rendered nodes) because it would cause an excessively large drupalSettings JavaScript declaration in the HTML <HEAD>. Hence it should only be used for secondary content (e.g. "new" content marker). It is not suitable for large chunks of content. It is only suitable for metadata or tiny chunks of content.

The downside is extremely obvious: inherently incompatible with reverse proxy setups, i.e. enterprise hosting.

However, the upside is that no additional HTTP requests are necessary, since all the necessary information is already in the HTML response itself. Since we don't want to break the render cache, we just #attach the necessary information.

This means everything in the <BODY> can be render cached. And only the <HEAD> needs to be generated dynamically. (Note: this distinction is exactly what #2068471: Normalize Controller/View-listener behavior with a Page object aims to formalize.)

This is clearly faster than the "Only HTML" approach, because there's no "let's do many string replacements on a huge blob of HTML" going on.

Why not just use ESI

ESI does not scale very well when handling large numbers of ESI tags, which is exactly the personalized content in Drupal core that makes render caching difficult: contextual links, comment edit links, comment author classes etc. can all appear up to hundreds of times per page each. Each cache miss from ESI tags (or uncacheable requests) means a separate call back to Drupal. Given entities might appear dozens or hundreds of times on a single page, with multiple personalized elements per entity this could lead to thousands of requests against Drupal to render a single page. ESI is great for caching different areas of content with different TTLs and cache keys (roughly equivalent to block level rather than within a block), and the approach here is compatible with using ESI, ensuring a much higher cache hit rate when using the client side replacement.

Conclusion

Delivery mechanisms 1 and 3 are impossible in today's Drupal 8, because we don't have something like #post_render_cache. Hence mechanism 2 is what we have today: it was the only possible solution that didn't break the render cache.

Delivery mechanism 1 is not very desirable: it's only suitable for extremely optimized, high-budget sites.

Delivery mechanism 2 is ideal for enterprise hosting.

Delivery mechanism 3 is ideal for shared hosting.

To accommodate both shared and enterprise hosting, we should default to delivery mechanism 3 and make it easy for enterprise hosting to override it to delivery mechanism 2. And in fact, that is trivial: alter away all #post_render_cache functions that are responsible for attaching personalized data in drupalSettings!
The only requirement is that the accompanying JavaScript first checks whether the necessary information is available in drupalSettings, and if it is not, then it should make the necessary HTTP request.

And again: they can all be implemented using a single new #post_render_cache callback!

Note: delivery mechanism 2 is also ideal when the number of personalized pieces of content per page is low, i.e. when there would only be few HTTP requests over time, because the necessary data would usually be in localStorage. So sites would have the ability to evaluate both approaches, with almost zero changes in the code, to find the best balance of "embed information in drupalSettings" versus "leverage localStorage". It is even possible to use a hybrid approach, where you use delivery mechanism 3 on a landing page to prime the client-side cache for when the user navigates to other pages that are personalized using mechanism 2!

Proposal

I propose to ship Drupal with personalized page delivery mechanism 3 implemented by default. As explained in the above conclusion, it is then easy to have the same JavaScript also support delivery mechanism 2. Combined, they achieve the most critical requirements.

That being said, delivery mechanism 1 should also be made possibly to achieve through #post_render_cache. There are valid use cases for this. (Sites that may not use JavaScript or whose users are connected through very high latency networks.)

Remaining tasks

  1. Comprehensive write-up to explain all considerations (see above)
  • Validate feasibility by implementing #post_render_cache, support all three identified "personalized page" delivery mechanisms
  • Comprehensive test coverage to validate correctness/ensure lack of side effects (!)
  • Implement for concrete use cases in Drupal core
  • Build consensus

User interface changes

None.

API changes

  • New #post_render_cache callback.
  • New #type = render_cache_placeholder element type.
  • Private API change: drupal_render_cache_get() now returns the cached $element rather than its output (`$element['#markup']`), to allow #post_render_cache callbacks to run.

Thanks

Go to catch and amateescu, with whom I had several discussions about this :)

Frequently Asked Questions

Why not simply move #post_render to run after the render cache instead of introducing a new callback?
  1. Makes it impossible to have a post-render callback whose results are cached in the render cache.
  2. Would break existing semantics: #post_render callbacks receive the rendered #children as a string and the element itself, #post_render_cache needs to receive the element itself, the context, and optionally a token to uniquely identify the placeholder. As you can see, very, very different.
With which "personalized page" delivery mechanisms can I use the new #type = render_cache_placeholder element type?
Only with mechanism 1, because only that mechanism is suited for primary content. (See the "in a nutshell" parts for each mechanism as to why that is.) Furthermore, it's only designed for "replace with chunk of HTML".
Mechanisms 2 and 3 are about conditionally attaching CSS/JS and getting personalization metadata to the client, and then having some attached JavaScript use that metadata. Mechanisms 2 and 3 are intended only for use cases like "new/updated" content markers, flags/bookmarks, links, and so on.
CommentFileSizeAuthor
#28 comment-interdiff.txt1.66 KBWim Leers
#28 comment_form_breaks_render_cache-2099133-7-do-not-test.patch3.48 KBWim Leers
#28 interdiff.txt3.83 KBWim Leers
#28 post_render_cache_and_placeholder-2118703-28.patch39.31 KBWim Leers
#18 2118703-alternate-proposal-do-not-test.patch5.29 KBamateescu
#17 post_render_cache_and_placeholder-2118703-17.patch38.63 KBWim Leers
#17 interdiff.txt2.93 KBWim Leers
#11 post_render_cache_and_placeholder-2118703-10.patch38.77 KBWim Leers
#11 interdiff.txt4.29 KBWim Leers
#1 post_render_cache_ONLY.patch31.28 KBWim Leers
#1 post_render_cache_and_placeholder-2118703-1.patch38.68 KBWim Leers
#1 post_render_cache_USE_CASE_history-do-not-test.patch9.5 KBWim Leers
#1 post_render_cache_USE_CASE_comment_form-do-not-test.patch4.18 KBWim Leers
#1 post_render_cache_DEMO_EVERYTHING-do-not-test.patch51.84 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Active » Needs review
FileSize
post_render_cache_DEMO_EVERYTHING-do-not-test.patch51.84 KB
post_render_cache_USE_CASE_comment_form-do-not-test.patch4.18 KB
post_render_cache_USE_CASE_history-do-not-test.patch9.5 KB
post_render_cache_and_placeholder-2118703-1.patch38.68 KB
post_render_cache_ONLY.patch31.28 KB

Attached are five different patches:

  1. post_render_cache_ONLY.patch: to show only the changes to the render system plus test coverage (60% of the patch is test coverage)
  2. post_render_cache_and_placeholder-2118703-1.patch: same as 1, plus the new #type = render_cache_placeholder element type (>50% is test coverage)
  3. post_render_cache_USE_CASE_history-do-not-test.patch: only the changes for the "new and updated markers" use case; this patch would solve #2073535: Node history markers require an HTTP request — this implements delivery mechanism 3, and by simply unset()ting the #post_render_cache callbacks, the JS settings won't be added anymore, and the JS will start making HTTP requests again, i.e. automatic fallback to mechanism 2
  4. post_render_cache_USE_CASE_comment_form-do-not-test.patch: only the changes for the "comment form on same page" use case; this patch would solve #2099133: Comment form on same page as comments forces node render caching to be per user — this implements delivery mechanism 1
  5. post_render_cache_DEMO_EVERYTHING-do-not-test.patch: all of the above combined, for demo purposes

Hopefully that will make this easier to review. Patch 2 is the one being proposed for inclusion in Drupal 8 core.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

One disadvantage of client side / JavaScript solutions that isn't noted is that at best you are including the unpersonalised data in the source code, and at worst you are showing it to the user while waiting for the personalised data to be downloaded. One use case where this might not be desirable would be if you were using the personalisation to add up to the second prices to the page, as the source code might show lower prices from when they were last cached a few hours ago.

Option 3 seems to be the worst of both worlds: You're dependent on JavaScript (which 1 avoids), yet can't cache the output (which 2 avoids). The only advantage over option 1 seems to be that it moves the work of replacing the tokens onto the client side, but is that really too expensive to always do server side? Have we got performance figures to show that replacing tokens is too slow, and if so are there methods we could use to improve this performance rather than use option 3? This is basic functionality for twig, so I imagine it has optimised it well.

If you've got a site serving hundreds of times its usual traffic then even option two is unlikely to work, as the ajax requests cannot be cached in a reverse proxy. The only solution is to add hardware or turn off personalisation until traffic calms down.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Issue summary: View changes

Credit catch & amateescu :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes

Explain why we can't just change #post_render instead of introducing a new callback.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#2: Unfortunately, pretty much all of your points are incorrect. That's either because you don't fully grok what's going on, or — far more likely — because I've failed to explain everything sufficiently clearly in my issue summary.
I think all of your concerns can be clarified by an updating of the issue summary. So I did that: https://drupal.org/node/2118703/revisions/view/2887805/2887861.

Nevertheless, I still formulated a reply:

One disadvantage of client side / JavaScript solutions that isn't noted is that at best you are including the unpersonalised data in the source code, and at worst you are showing it to the user while waiting for the personalised data to be downloaded.

Both are utterly wrong. Unpersonalized data in the source, as in "universal truths", such as data-comment-author-user-id="99777" don't harm anyone. And of course you must not show anything until the personalized data is available.
That's precisely what #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user already has done for many weeks in D8 HEAD, and it's what I explain in http://wimleers.com/talk/really-fast-drupal-8.

One use case where this might not be desirable would be if you were using the personalisation to add up to the second prices to the page, as the source code might show lower prices from when they were last cached a few hours ago.

If your main content needs to be personalized, of course this is not your solution. In that case, you always want mechanism 1. In fact, that's also why I use mechanism 1 for #2099133: Comment form on same page as comments forces node render caching to be per user: the comment form is not a small thing, it's a primary component on the page, so you sacrifice performance for personalization.

Option 3 seems to be the worst of both worlds: You're dependent on JavaScript (which 1 avoids), yet can't cache the output (which 2 avoids). The only advantage over option 1 seems to be that it moves the work of replacing the tokens onto the client side, but is that really too expensive to always do server side?

I think you're entirely misunderstanding mechanism 3. The entire <BODY> of the page can be cached by the server in the render cache. Only drupalSettings will be calculated for each page load.
Only mechanism 1 is intended to be used for replacing placeholders! Consequently, only mechanism 1 is intended for "primary components on the page" (large chunks of content). Mechanisms 2 and 3 are only intended for "secondary components on the page" (metadata/tiny chunks of content). You could in theory also use mechanism 1 for "secondary components", at significant cost (see later).

Have we got performance figures to show that replacing tokens is too slow, and if so are there methods we could use to improve this performance rather than use option 3? This is basic functionality for twig, so I imagine it has optimised it well.

All of this is entirely unrelated to Twig. Generating 10 bits of metadata to be stored in drupalSettings is clearly much faster than having 10 callbacks each render a whole chunk of HTML, then searching 50 KiB of HTML.
Render time + huge string manipulation times the number of personalizations is clearly inherently slower than generating a few hundred bytes of metadata.

If you've got a site serving hundreds of times its usual traffic then even option two is unlikely to work, as the ajax requests cannot be cached in a reverse proxy. The only solution is to add hardware or turn off personalisation until traffic calms down.

No.

First: everything would continue to work just fine, since the site is being served from a reverse proxy. The only thing that might fail, is the personalized stuff. Loss of personalization in case of high load: I'd sign up for that any time.
Second: AJAX/JSON requests are cacheable. You just have to make sure you cache them by user (or location or whatever context they depend on). You pretend like they're inherently uncacheable, and that is utterly wrong.
Third: hundreds of times usual traffic does not imply hundreds of times AJAX/JSON requests. Remember, the accompanying JS should apply client-side caching of the results it gets from talking to the server. So it should only be a fraction. And even then, remember that the web servers are mostly idle, because there's a reverse proxy in front of them. Furthermore, AJAX/JSON requests typically require less computation.

Hence I think that your reasoning is not well-founded.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes

Explain for each of the three mechanisms whether they're suitable for large chunks of content or metadata/tiny chunks of content.

attiks’s picture

attiks CreditAttribution: attiks commented

Some remarks about the summary (which is very nicely written, kudos):

1/ +1 to support this scenario and not only implement another solution
2/ other downside is that you add a dependency on javascript. To solve the multiple request people could start using SPDY (which they should do anyway ;-))
3/ Question: "Suppose the page is altered depending on the user role, is all data for all roles injected inside the same drupalSettings?"

Another question: Can one site use a combination of this proposal, or do we have to select one for all requests?

twistor’s picture

twistor CreditAttribution: twistor commented

Very well written. I know you have explained this till you're blue in the face.

A slightly tangential idea:
Can we build an API on the client side that groups all customization requests into one HTTP request? This API could also use localStorage behind the scenes.

Actual review coming.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Issue summary: View changes

I hate filter_autop.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Issue tags: -API change, -API addition

Yes, I had thought you were saying option 2 was unsuitable for primary content, so use option 3. If we're agreed options 2 and 3 are only suitable for seconary content, then that resolves some of my concerns, but means there are many use cases where we would still need option 1. [sentence edited for clarity]

Generating 10 bits of metadata to be stored in drupalSettings is clearly much faster than having 10 callbacks each render a whole chunk of HTML, then searching 50 KiB of HTML.
Render time + huge string manipulation times the number of personalizations is clearly inherently slower than generating a few hundred bytes of metadata.

I'm not sure it is much faster. Both options mean:
a) bootstrap Drupal
b) retrieve / calculate 10 pieces of data
c) render

[edit: this paragraph trivialises what could be major changes and may not even be possible without breaking APIs, which would rule it out of Drupal 8]
Clearly the render step will be slower for option 1, but I would expect the difference to be insignificant when a and b are taken into account. Option 1 doesn't mean we have to call str_replace on large rendered strings of HTML. Instead the render process [edit: could] create a php script and cache that. Subsequent requests just call that php script with the personalised data. I know that's what Smarty does internally, I'd imagine twig is the same [so the approach is possible].

The only disadvantages I can see relate to reverse proxies, as you can't use them to cache the pre-personalisation template (which you can for option 3), and you'll fill them up much faster if you cache post-personalisation templates (compared to caching drupalSettings).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#4:

1/ :)
2/ Yes, there's a dependency on JavaScript in mechanisms 2 and 3. But that's not necessarily a bad thing — it may be for your use case. It does inherently give us more possibilities to do smart caching! Plus, by moving some simple logic from the server side to the client-side (i.e. into JS), certain personalization things (e.g. the "by-viewer" class on comments, to allow sites to style comments by the current viewer differently) become 100% cacheable; zero web server CPU time needed.
3/ Per-role settings don't have to be injected using #post_render_cache, since they don't break render caching. It's still possible to do that though. And in any case, there's always going to be only one drupalSettings declaration in the page's <HEAD> if that's what you mean.

RE: "another question": Yes, you can use a combination! That's the whole point, that you can use all three mechanisms. Each has its own pros and cons, and depending on which kind of content it is, its size, and the requirements of your site, you should choose different mechanisms. I've updated the issue summary to make that more explicit: https://drupal.org/node/2118703/revisions/view/2887871/2888719 — clearly this is another thing that I've failed to explain sufficiently clearly :(
BTW, search for "hybrid approach", where I explain how you could use mechanism 3 on one page and 2 on another, to leverage the advantages of each to get a very nice combined end result :)
And finally: see #1, points 2 and 3. There you can see what the implementations of all three mechanisms look like!

#5: request batching! Yes! I/We have mentioned that several times now in other issues. That'd be a very powerful feature! I'd love to have that.

However, that will probably require pretty deep routing system work, potentially requiring big API additions/changes. If we'd have that, it'd make mechanism 2 much more efficient and hence attractive. However, as you say, that's only tangentially related: it's a further improvement, one that would be useful to have independent of the problem being tackled here :)

#6:

Yes, I had thought you were saying option 2 was unsuitable for primary content, so use option 3. If we're only talking secondary content, then that resolves some of my concerns, but means there are many use cases where we would still need option 1.

You're misinterpreting again. Mechanism 3 is also not acceptable for primary content. I've even explicitly stated that in the issue summary now, and linked you to the diff view. How else do you want me to explain this? :(
And yes, of course we still need mechanism 1 in some cases. That's precisely attiks' question: " Can one site use a combination of this proposal, or do we have to select one for all requests?". The answer is: the goal of this issue is to support ALL THREE, and the patch in #1 already does that!

The rest of your comment I will also reply to. But please stop speculating out of thin air in future comments. Please think within the constraints of existing APIs. Thank you.

Clearly the render step will be slower for option 1, but I would expect the difference to be insignificant when a and b are taken into account. Option 1 doesn't mean we have to call str_replace on large rendered strings of HTML.

Wrong. It means exactly that we have to call str_replace() on a huge chunk of HTML.

Instead the render process creates a php script and caches that.

WAT? Are you actually suggesting we generate PHP scripts that are facing the world? Not to mention file permissions problems and the number of potential PHP scripts we'd have to generate, and whatever other security concerns there may be: this clearly is far beyond the APIs that we currently have and is hence unacceptable.

Subsequent requests just call that php script with the personalised data. I know that's what Smarty does internally, I'd imagine twig is the same.

Sigh. Again. This is entirely unrelated to Twig or template systems in general. I see why you might think that though: because of the mention of placeholders. However, if we'd be using a template system for this, we'd be doing it wrong: the point is that all (template) rendering has been done and is cached (for better performance), except for the final bits of personalization, which we need to inject at the very last point possible, after (template) rendering, after caching, because otherwise the personalization would be cached. Hence by necessity, this *is* about simple string placeholder replacement, and not about complete template systems.

The only disadvantages I can see relate to reverse proxies, as you can't use them to cache the pre-personalisation template (which you can for option 3), and you'll fill them up much faster if you cache post-personalisation templates (compared to caching drupalSettings).

As explained in the summary, option 3 is inherently incompatible with reverse proxies.

In order to avoid an endless bikeshed, which is a waste of both of our times: please don't post any further comments until you've had a chance to fully understand the patches and have observed the way each mechanism works. I spent hours writing the issue summary precisely to avoid this kind of misinformed back-and-forth. I don't mind clarifying things in the issue summary and answering questions, but this kind of (superficial) speculation is most unproductive (and extremely frustrating).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes

Clarify that the proposed solution supports all 3 mechanisms!

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Well hard to add anything after that issue summary. Another huge benefit of going with 3 by default is that it address my brand new concern: #2119299: Make sure Drupal 10 works with mobile proxy browsers. I'm for this big time.

#5: request batching! Yes! I/We have mentioned that several times now in other issues. That'd be a very powerful feature! I'd love to have that.

Also it'd mean less browser cacheability on the requests (once we can actually cache them, which isn't the case right now…) as usual attiks is right, SPDY all the way.

Totally unrelated but funny nonetheless: "enterprise hosting (with a reverse proxy such as Varnish in front of the web servers)" I've seen 'enterprise websites' without varnish in front of it (which it should have but that's even further beyond the point :)

twistor’s picture

twistor CreditAttribution: twistor commented

Avoiding nitpicks for the moment. This seems to accomplish the 3 goals at hand. No manual testing yet.

Overall, I would like to see a bit more hand holding regarding the #post_render_cache callbacks. What if, comment_insert_form() only returned a render array and the str_replace() and #attached handling were done on its behalf in _drupal_render_process_post_render_cache()?

A similar idea for comment_attach_new_comments_link_metadata(). What if it just returned the sub-array that should be added to js settings? 

  1. +++ b/core/includes/common.inc
@@ -3791,7 +3805,7 @@ function drupal_render_page($page) {
+function drupal_render(&$elements, $is_recursive_call = FALSE) {

    It seems like we could do this without adding the $is_recursive_call. Nothing comes to mind immediately however. 

  2. +++ b/core/includes/common.inc
@@ -3802,11 +3816,14 @@ function drupal_render(&$elements) {
+    $cached_element = drupal_render_cache_get($elements);
+    if ($cached_element !== FALSE) {
+      $elements = $cached_element;
+      _drupal_render_process_post_render_cache($elements, $is_recursive_call);
+      return $elements['#markup'];

    This subtly changes the behavior. hmm 

  3. +++ b/core/includes/common.inc
@@ -4119,6 +4161,169 @@ function drupal_render_cache_set(&$markup, $elements) {
+function _drupal_render_process_post_render_cache(array &$elements, $is_recursive_call = FALSE) {

    The caller should check if $is_recursive_call rather than this function. It's weird to have a function accept $is_recursive_call when it is not recursive. 

  4. +++ b/core/includes/common.inc
@@ -4119,6 +4161,169 @@ function drupal_render_cache_set(&$markup, $elements) {
+    $original_elements = $elements;
...
+    $original_elements['#markup'] = $elements['#markup'];
+    if (isset($elements['#attached'])) {
+      $original_elements['#attached'] = $elements['#attached'];
+    }
+    $elements = $original_elements;

    We can avoid this dance by using a different variable for the return value of call_user_func_array(). 

  5. +++ b/core/includes/common.inc
@@ -4119,6 +4161,169 @@ function drupal_render_cache_set(&$markup, $elements) {
+function drupal_render_collect_post_render_cache(array $elements, $return = FALSE) {
+  $post_render_cache = &drupal_static(__FUNCTION__, array());
...
+      drupal_render_collect_post_render_cache($elements[$child]);
...
+  if ($return) {
+    $return = $post_render_cache;
+    $post_render_cache = array();
+    return $return;

    $return can be removed here. Just have drupal_render_collect_post_render_cache() always return and build up $post_render_cache inline. This also avoids the weird static thing.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

for 3.

The caller should check if $is_recursive_call rather than this function. It's weird to have a function accept $is_recursive_call when it is not recursive.

Renaming to $within_recursive_call in _drupal_render_process_post_render_cache signature should do the trick?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +API change, +API addition
FileSize
interdiff.txt4.29 KB
post_render_cache_and_placeholder-2118703-10.patch38.77 KB

#9: Thanks for the review! :)

I'm fine with more hand holding in general. But then we must be certain that that is better. Right now, #post_render_cache callbacks just receive $element = array('#markup' => 'HTML', '#attached' => array('attachments')), and those are precisely the things it can manipulate. (Test coverage ensures it can't manipulate anything else.) That means two things can be modified. So simply returning things won't work: it's possible to both add attachments and replace placeholders in one #post_render_cache callback.
Or am I missing something? :)

  1. This is necessary to deal with the "nested #cache'd elements" case. Feel free to remove/disable the $is_recursive_call stuff, but then run tests again and observe which things fail. If you can make those tests pass without adding this parameter, go ahead :)
    I personally hate that new parameter. But it's the simplest solution catch and I could think of (I discussed this with catch in IRC, this is the idea he came up with).
  2. Indeed. And it is mentioned in the API changes:

    Private API change: drupal_render_cache_get() now returns the cached $element rather than its output (`$element['#markup']`), to allow #post_render_cache callbacks to run.

    Which reminds me: I should add the appropriate tags. Done.

  3. You're right. :) That's much better! Thanks, fixed.
  4. I was able to slightly simplify it, but I don't think I can simplify it further, because the code aims to guarantee only #markup and #attached can be modified by #post_render_cache callbacks. If you can simplify it further while keeping the tests passing, go ahead! :)
  5. This is the exact same code structure as drupal_render_collect_attached() uses. I'd rather follow established patterns than come up with new ones. I personally agree with you though that the code is weird because of the unneeded static.
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#10: that would be better also, but thanks to twistor's comment, I noticed it's possible to remove that parameter altogether :)

twistor’s picture

twistor CreditAttribution: twistor commented

A very quick sketch of what I meant regarding hand holding. This is only the placeholder half.

/**
 * #post_render_cache callback; returns the comment form.
 *
 * @param array $element
 *   A render array with the following keys:
 *   - #markup
 *   - #attached
 * @param array $context
 *   An array with the following keys:
 *   - entity_type: an entity type
 *   - entity_id: an entity ID
 *   - field_name: a comment field name
 *
 * @return array
 *   A render array with the new markup.
 */
function comment_insert_form(array $element, array $context) {
  // Build comment form based on stored context.
  $entity = entity_load($context['entity_type'], $context['entity_id']);
  $comment_form = comment_add($entity, $context['field_name']);

  return $comment_form;
}

function _drupal_render_process_post_render_cache() {
...
    foreach ($elements['#post_render_cache'] as $callback => $options) {
      foreach ($elements['#post_render_cache'][$callback] as $token => $context) {
        if (is_numeric($token)) {
          $elements = call_user_func_array($callback, array($elements, $context));
        }
        else {
          $fragment = call_user_func_array($callback, array($elements, $context));
          // Update the renderable.
          if (!isset($elements['#attached'])) {
            $elements['#attached'] = array();
          }
          $elements['#attached'] = drupal_merge_attachments($elements['#attached'], drupal_render_collect_attached($fragment, TRUE));
          $placeholder = drupal_render_cache_generate_placeholder(__FUNCTION__, $context, $token);
          $elements['#markup'] = str_replace($placeholder, drupal_render($fragment), $elements['#markup']);
        }
      }
    }
}
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

We can remove the static from drupal_render_collect_attached(), it's useless in there, I actually thought there was an issue that did this that was committed already, but obviously not. Not for this issue but maybe a side one?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue summary: View changes

Explain how render_cache_placeholder can be used.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

@catch, I think you recall doing it for the new drupal_render_collect_cache_tags() ;)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

In case it comes up, I've added a "Why not ESI for this?" section to the issue summary. The overall approach here is orthogonal to whether a site is using ESI or not, and it's solving a problem that ESI isn't very well suited for.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
FileSize
interdiff.txt2.93 KB
post_render_cache_and_placeholder-2118703-17.patch38.63 KB

#13: Oh, I see now :) Yes, that could work. I like it. Let's see what other people think first.
Initial concern: wouldn't that make the implementation of #post_render_cache callbacks too confusing? We could in both cases just always pass $context; then in the mechanism 1 case we allow for a render array to be returned, just like you did; in the mechanism 2/3 case we allow for just an #attached array to be returned. Same signatures (yay), but very different return values (boo). Thoughts?

#14/#15: Oh, we do have a precedent then! Rerolled for a cleaner drupal_render_collect_post_render_cache() :)

#16: lovely, thanks! For those wondering, here's the diff that shows catch's additions: https://drupal.org/node/2118703/revisions/view/2888765/2888975

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes

Updated issue summary.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Issue tags: +API change, +API addition
FileSize
2118703-alternate-proposal-do-not-test.patch5.29 KB

Please bear with me while I try to gather my thoughts on this and present a simpler alternative :)

First of all, about delivery method 1. "Only HTML":

Once we start thinking it through, some significant downsides arise:

  1. Inherently incompatible with a reverse proxy-based setup (Varnish, CDN …), because any page that contains personalized contents.
  2. Inherently less scalable/higher server hosting cost, because each page needs to be completely built on the server side.
  3. Inherently higher minimum page load time, because the server inherently needs more time to generate the page (to find and replace the placeholders).

- 1) is not a concern for this issue/patch because, as @catch pointed out, we're not trying to reinvent ESI here. So if you have the need for a reverse proxy setup, you want to be able to deliver personalized content at a different level than what we're adressing here (e.g. block level, not "new comment indicator" level).
- 2) and 3) are mostly the same (I could go on with 4) Inherently less happy site visitors; 5) Inherently less ROI; 6) Inherently your entire business comes crashing down, etc. - you get my point :))

The entire argument on this delivery method where you try to make it look as a bad default is made by implying that we *lose* something here. Problem is that if we're looking at it from a D7 perspective, we can't lose something we never *had*, we just don't gain as much.

I do realise that your patch supports all three delivery methods, my problem with it is that I think it provides a "less than stellar" DX, thus breaking requirement #1. Specifically, why would I want to write *another* callback and provide it with "context" when my render element already has all the context it needs in order to do its job. Even more, why should I use a special element type to output the placeholder when the system can easily do it for me.

This silly DX can be seen in action in the patch from #2099133-4: Comment form on same page as comments forces node render caching to be per user. Ideally, we should only rely on a special value given to the render element that tells it to "render me later, after dealing with the render cache if it's enabled".

With that in mind, my concrete proposal is to start with the first delivery method as *the baseline*/default and only enhance it where we deem necessary (I believe this is called progressive enhancement in frontend speak). This will transform the followup patches to be as simple as adding one line to the render array, the "render me later" attribute, and as 'complicated' as adding a second line, a "process me in JS" attribute.

I was very close to reaching this goal in the patch attached, but it doesn't work because we need the change proposed by Fabian in Prague, about making drupal_render() return an array of '#markup' and '#attached' instead of a string. This is needed because we want recursive drupal_render() calls on child elements to bubble up their attachments.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#18:
Indeed, we're not trying to reinvent ESI. But the point there is that for personalized secondary content, we have one mechanism that is compatible with reverse proxies (mechanism 2) and one that isn't (mechanism 3). For personalized primary content, ESI is the only feasible solution in reverse proxy setups. When not working with a reverse proxy setup, mechanism 1 is the solution.

You're also right that this is about gaining speed, not about losing speed.
When I originally wrote the issue summary, I was particularly thinking about secondary content. When looking at personalized secondary content (e.g. "new" markers or comment "edit" links; which are all similar in pattern, all just slightly different, and typically a large number per page), mechanisms 2 and 3 are superior. It is in that light that I marked mechanism 1 as being the worst. But it's also the only one that's usable for personalized primary content. That's why I am using mechanism 1 in e.g. the "comment form on same page" patch.

I agree the DX is not stellar. But neither is using anything more than the mere basics of the render system! However, I do think it's acceptable, and definitely not overwhelming. If you could improve it, I'm all in favor of though :)

You label the DX in #2099133-4: Comment form on same page as comments forces node render caching to be per user as "silly". I think you say that because it's moving part of the render array into another callback? I can see that. I'd prefer not to have to do that either. I think it makes sense conceptually though: all the generic (non-personalized, or solely depending on user role) stuff sits in the main render array, anything that is personalized for another context than just "role" must use a #post_render_cache callback if you don't want to break the render cache.

Analysis

To ensure I fully grasp the consequences of your proposal, I wanted to rewrite the "silly" patch at #2099133-4 on top of your proposal. However, because it's not completely functioning, I can't. But that doesn't prevent me from analyzing how it would work.
So: when the comment form is displayed on the same page as a node, it breaks the render cache for authenticated users. The code looks like this:

// Only show the add comment form if the user has permission.
if ($this->currentUser->hasPermission('post comments')) {
  $output['comment_form'] = comment_add($entity, $field_name);
}

In your proposal, that would change to:

// Only show the add comment form if the user has permission.
if ($this->currentUser->hasPermission('post comments')) {
  $output['comment_form'] = comment_add($entity, $field_name);
  $output['comment_form']['#delay_render'] = TRUE;
}

Wow — magic! Elegant! Simple! Beautiful, even! I'd love it if we could make mechanism 1 work like this. This is definitely a stellar DX!

So let's now look at the next step in your proposed algorithm: drupal_render() detecting #delay_render, transforming it into an #attached callback, with $token to find the automatically generated placeholder and $elements to render the comment form after it's been retrieved from the render cache, both of which will be serialize()d.

if (!isset($elements['#attached']['drupal_render_delayed'][$token])) {
  $elements['#attached']['drupal_render_delayed'][$token] = $elements;
}

This works. But it has one huge consequence. Depending on what's inside $elements, I fear it's going to serialize a rather large amount of data — think Node and View objects. We can check precisely how much by adding some debug output:

// Only show the add comment form if the user has permission.
if ($this->currentUser->hasPermission('post comments')) {
  $output['comment_form'] = comment_add($entity, $field_name);
  $output['comment_form']['#delay_render'] = TRUE;
  // Debug output.
  var_dump(strlen(serialize($output['comment_form'])));
  var_dump($output['comment_form']);
  exit;
}

The output looks like this:

int 60020
array (size=37)
  '#attributes' => 
    array (size=1)
      'class' => 
        array (size=2)
          0 => string 'node__comment-comment-form' (length=26)
          1 => string 'comment-form' (length=12)
  '#id' => string 'comment-form' (length=12)
  '#theme' => 
    array (size=2)
      0 => string 'comment_form__node__article__comment' (length=36)
      1 => string 'comment_form' (length=12)
  '#action' => string '/comment/reply/node/1/comment' (length=29)
  'author' => 
[… snip …]
  '#validate' => 
    array (size=1)
      0 => 
        array (size=2)
          0 => 
            object(Drupal\comment\CommentFormController)[423]
              ...
          1 => string 'validateForm' (length=12)
  '#submit' => 
    array (size=1)
      0 => 
        array (size=2)
          0 => 
            object(Drupal\comment\CommentFormController)[423]
              ...
          1 => string 'submitForm' (length=10)
  '#processed' => boolean true
  '#defaults_loaded' => boolean true
  '#required' => boolean false
  '#title_display' => string 'before' (length=6)
  '#array_parents' => 
    array (size=0)
      empty

60020 bytes. ±60 KiB!
I don't think that's acceptable. That's probably more than the entire HTML markup that's being render cached (for the node plus all its comments). It fills the cache with huge amounts of metadata, hence decreasing the number of render cache entries that can be stored. It makes retrieving data from the render cache more expensive.

Comparison

That problem does not occur in my implementation, because it allows us to enforce object identifier-based serialization: you have to specifically define the context — in the comment form case that would be:

+              '#context' => array(
+                'entity_type' => $entity->entityType(),
+                'entity_id' => $entity->id(),
+                'field_name' => $field_name
+              ),

Conclusion

I'd love to see mechanism 1 work the way you proposed. It's a zillion times more elegant. But unless we can solve the "serialize huge blobs of data" problem, I don't see how we can consciously choose this. :(

amateescu’s picture

amateescu CreditAttribution: amateescu commented

I'm glad you like it :)

The huge size of serialized objects is already a big pain point of D8, which we need to fix regardless of this patch, and we have issues to deal with that for config and content entities: #1977206: Default serialization of ConfigEntities and #2027795: Optimize content entity serialization. We don't have one yet for optimizing EntityFormController serialization, but we need it anyway to reduce the size of $form_state cache.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Hmm so what if instead of serializing the output of comment_form, we used the #pre_render pattern that already gets used for the render cache anyway. That way you set up the context, and the comment_form() call happens in the pre_render callback, but we keep the #delay_render stuff. I realise that #pre_render is the bit that only about five people understand about render caching so perhaps this isn't good. The advantage of both this and the current patch here though is that using an actual ESI tag for the comment form isn't far off what's being done (and ESI will actually make sense for the comment form - it's a big chunk of HTML of which there's usually only one on the page).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#20: optimizing serialization of huge objects still results in huge objects. Look at #2027795-5: Optimize content entity serialization: from 13000 bytes to 5800 bytes (assuming that's bytes). Plus, that still won't help with pretty darn complex and huge $elements render arrays either — even if you assume the serialized objects become tiny, they're still going to be huge! For example, this is just for the "name" field of the author of a comment form. A tiny part of a super simple form:

  'author' => 
    array (size=15)
      'name' => 
        array (size=23)
          '#type' => string 'item' (length=4)
          '#title' => string 'Your name' (length=9)
          '#default_value' => string 'root' (length=4)
          '#required' => boolean false
          '#maxlength' => int 60
          '#size' => int 30
          '#value' => string 'root' (length=4)
          '#theme' => string 'username' (length=8)
          '#account' => 
            object(Drupal\Core\Session\UserSession)[93]
              ...
          '#input' => boolean true
          '#markup' => string '' (length=0)
          '#theme_wrappers' => 
            array (size=1)
              ...
          '#defaults_loaded' => boolean true
          '#tree' => boolean false
          '#parents' => 
            array (size=1)
              ...
          '#array_parents' => 
            array (size=2)
              ...
          '#weight' => int 0
          '#processed' => boolean false
          '#attributes' => 
            array (size=0)
              ...
          '#title_display' => string 'before' (length=6)
          '#id' => string 'edit-name' (length=9)
          '#name' => string 'name' (length=4)
          '#sorted' => boolean true

That's why I believe that regardless of any such optimizations, this is the wrong way to go.

The comparison to $form_state doesn't hold IMHO: form state must be able to be arbitrarily complex and large, because forms can be arbitrarily large and complex. Render caching needs to be efficient.

#21: I don't understand what you mean. Are you referring to the #pre_render usage of drupal_render_cache_by_query()?
(And by "current patch", do you mean amateescu's or mine?)

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Which patch do you refer to as the current one, #18? I'm not sure I understand the implications of what you suggest or how it would look like.. I'll try harder tonight.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I meant Wim Leer's current patch, not the new proposal.

So a common use in drupal render caching is to also use #pre_render - this saves building the majority of the render array if there's a cache hit. For example #2099131: Use #pre_render pattern for entity render caching would mean this gets implemented for entity render caching itself.

So you'd have something like:

<?php

$element = array(
'#late_render' => TRUE,
'#pre_render' => array('_build_the_comment_form'),
'#some' => $context,
'#which' => $gets,
'#passed' => $with,
'#the' => $element',
'#when' => $the,
'#pre_render_callback' => $is_called,
);

So the #pre_render callback takes an $element array and returns it with the comment form added, which drupal_render() can then swap in instead of the placeholder. It'd be doing similar work to what's in the #post_render_cache callback just using an existing API - so we just add the #late_render and placeholder handling as suggested by amateescu.

The advantage of this is that the only data that has to be serialized is enough of an $element to reliably build the same thing in the pre render callback, which ought to be a similar amount to post_render_cache anyway, and that we're using an existing callback in the same way it's already used, instead of adding a new one.

The disadvantage is that very few people understand how #pre_render callbacks work.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Re: #24 Ok, I got it now. Another big disadvantage is that you would have to know *exactly* what the $element needs in order to do its job.

If we really want to offer the possibility to pass the exact context that will get serialized, we could make '#delay_render' *also* accept an array and only what gets passed in there is serialized. This way, we have the basic use case covered "I don't care/know enough about this element, just let me render it later" and also "I know exactly what the elements needs in order to function, here it is".

We'd have something like this:

$element = array(
  '#delay_render' => array(
    '#some' => $context,
    '#and' => $more_context,
  ),
);

I'm not tied in any way to this exact syntax, #delay_render could always be an array with two values keys, 'enabled' and 'context', that doesn't matter. What matters is that I think the proposal in #18 should be the starting point on which to improve upon, as it's way less 'invasive' and provides much better DX, as we already agreed :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#24: Thanks for the clarification, now I understand :)

I see the rationale, but I fear this is more difficult than what I'm proposing, because it changes the semantics of #pre_render entirely. Normally, anything that is #pre_rendered or #post_rendered will end up in the render cache. In your proposal, there would be a modifier for #pre_render to make it effectively run after the render cache. Which IMHO is very tricky/confusing/easy to miss.

That being said, we could of course just call it #post_render_cache like in my patch instead of a modifier for #pre_render, to avoid any confusion, but still make it work exactly like #pre_render: this would save us from introducing a new pattern. Then, it seems, it's possible to do what you propose, yet still maintain conceptual clarity.

However … there is one problem with this approach that none of us has noticed so far, and it's hiding in plain sight. It's this: how do you determine the #some, #which, #passed etc. properties that provide context in your example? That's indeed how #pre_render gets its context. But render caching will only store #markup and #attached (as well as #post_render_cache in my proposal). How can the render caching system determine which of those #properties are there to provide context? (I'd be happy to be proven wrong here!)
AFAICT it's that aspect of the entire #pre_render architecture that makes it impossible to adopt the #pre_render architecture for our post-render cache needs, which means that we must choose a different architecture… such as the one I proposed :)

Conclusion: So, unless I'm wrong, the possibility of using the #pre_render pattern in any shape or form is inherently impossible. #pre_render cache seems a great way to reduce the cost on render cache hits though.

This was all wrong. #delay_render would cause the entire $element array to be cached, including all those #properties, and hence #pre_render would just be able to use that. No problems.

#25: working on a reply!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I thought a bit about this yesterday and came to a somewhat similar conclusion to Wim - we shouldn't change the semantics of #pre_render but having #post_render_cache work the same way as it could be nice assuming it works.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue summary: View changes

Explain "personalization".

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes
FileSize
post_render_cache_and_placeholder-2118703-28.patch39.31 KB
interdiff.txt3.83 KB
comment_form_breaks_render_cache-2099133-7-do-not-test.patch3.48 KB
comment-interdiff.txt1.66 KB

My goal ever since amateescu posted #18 was to incorporate its much simpler DX into my patch, to come up with the best of both. However, after long deliberation, I can't find a way to combine his proposal into what I built. I found several fundamental problems with it.

Alternative proposal's "modifier flag" is bad DX

catch and I have both argued now that a "modifier flag" such as #delay_render to modify the behavior of #pre_render (and #post_render, too, then) is a bad idea. The idea of the alternative proposal results in a better DX but due to the "modifier flag" the effective DX is worse.

Alternative proposal's problematic aspects

There are more aspects of the implementation that are problematic though:

Shouldn't leverage #attached
(This is the only problematic aspect that may only be problematic because #18 was a quick 'n dirty proof of concept.)
The piggy-backing of "drupal_render_delayed" on #attached is unacceptable, because it is only being used to get the metadata for delayed rendering to be stored in the render cache; it is not handled by #attached-handling code at all: drupal_process_attached() was specifically modified to ignore it.
Nesting
Unfortunately, in the alternative proposal, it is very easy to make render caching extremely expensive and inefficient: just set the #delay_render flag on an element that is the top of a large tree of child elements. Now Drupal has to serialize and store an enormous array in the render cache. Plus, upon unserialization, Drupal may have to run potentially dozens of #pre_render/#post_render callbacks, including some that were not intended to be run after the render cache. All of that simply because a user set a single property somewhere in a render array!
The #post_render_cache approach I proposed always renders as much as it can before render caching, stores that in the render cache, and always stores only a very minimal amount of extra information in the render cache: a callback plus the context for it.
Only supports 1:1, not 1:n (single callback to update many placeholders)
The alternative proposal forces one to replace only a single placeholder. I did not mention this explicitly yet, but an option I wanted to keep open in my proposal, is for people to generate their own placeholders (which is impossible in the alternative proposal), and more importantly, to have a single #post_render_cache callback replace many placeholders. I know that this is the 1% use case, but it can have a very big performance impact.
e.g.: A 1:n implementation could be used for #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links, to provide a no-JS solution for authenticated users; placeholders for that use case could be generated, and a single callback could efficiently inject the "active" class for just the appropriate links.

Alternative proposal: does it truly have a superior DX? And: improving main proposal's DX!

But most importantly, I think it's questionable whether this approach really has a superior DX. It is not truly declarative, it only is on the surface (i.e. "set #delay_render and you're done"). In practice, it only works if you have a #pre_render or #post_render callback.

From #18:

  1. Specifically, why would I want to write *another* callback and provide it with "context" when my render element already has all the context it needs in order to do its job.

    Because the semantics of the callback are different than #pre_render or #post_render. And as I'll show below, it's not a matter of providing the same "context" again (i.e. duplication), it's a matter of moving the context just a little bit.

  2. Even more, why should I use a special element type to output the placeholder when the system can easily do it for me.

    I'd love to improve this aspect of the DX of my proposal.
    But it's not as simple as you make it seem. The only reason you were able to do it in a seemingly super simple way is because you're dynamically changing the semantics of other callbacks. That's very bad for DX!
    Furthermore, we should not prevent developers from generating their own placeholders if they chose to do so.

    The DX of how the callback function should be written is the most problematic part of the implementation of mechanism 1 in my proposal, because that currently requires calling drupal_merge_attachments() and drupal_render_cache_generate_placeholder(), the result of the latter must be used to call str_replace(). That's not very nice. So I improved that in this reroll. With this reroll, a #post_render_cache callback that uses the included placeholder system (which is also used when you use #type => render_cache_placeholder) will only receive array $context and will simply have to return a render array. That's it. Drupal will automatically take that and replace the placeholder with that and use its attachments.
    I've also updated the comment form patch from #2099133: Comment form on same page as comments forces node render caching to be per user accordingly and attached it here: the difference in code/DX is huge.

When you're doing

$element = array(
  '#pre_render' => array('_build_the_comment_form'),
  '#delay_render' => TRUE,
  '#some' => $context,
  '#and' => $more_context,
);

or 

$element = array(
  '#pre_render' => array('_build_the_comment_form'),
  '#delay_render' => array(
    '#some' => $context,
    '#and' => $more_context,
  ),
);

then that is not fundamentally simpler than

$element = array(
  '#type' => 'render_cache_placeholder',
  '#callback' => '_build_the_comment_form',
  '#context' => array(
    'some' => $context,
    'and' => $more_context,
  ),
);

But the latter is not messing with existing semantics, and makes everything very explicit (and hence clearer).

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Cool, carry on then.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
+++ b/core/includes/common.inc
@@ -4119,6 +4172,157 @@ function drupal_render_cache_set(&$markup, $elements) {
+function drupal_render_cache_generate_placeholder($callback, array $context, $token) {

What happens if callback is an array here? eg array(SomeObject, someMethod)?
Doesn't look like that is supported?

Other than that this looks awesome, opens up serious potential

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#30: good point. I'll definitely need to fix that.

catch, what do you think?

eaton’s picture

eaton CreditAttribution: eaton commented

2/ Yes, there's a dependency on JavaScript in mechanisms 2 and 3. But that's not necessarily a bad thing — it may be for your use case. It does inherently give us more possibilities to do smart caching! Plus, by moving some simple logic from the server side to the client-side (i.e. into JS), certain personalization things (e.g. the "by-viewer" class on comments, to allow sites to style comments by the current viewer differently) become 100% cacheable; zero web server CPU time needed.

Just as an FYI, I've been experimenting with some very very rough personalization mechanisms in D8 that happen on the server side for security reasons. It's not quite the same as some of the scenarios described here, but it's something like:

  1. Generate a "superset" document that contains all permutations of the desired content, with data-attributes identifying which elements on the page should be invisible to which audiences
  2. Allow the full page to be cached
  3. Use a Symfony Response Filter to post-process the final output, based on identification cookies in the user's browser
  4. If no identification cookie is found, filter at the most restrictive level, stripping out any "audience-targeted" page elements

This approach is very similar to the JS one described, in that it assumes data-attributes to identify personalize-able elements, a packet of information (either in drupalSettings, or cookies) to determine what changes get made, and a process that operates on the page markup after the cached version has been retrieved.

So while the specific mechanism being described in this issue (JS transformation of the delivered markup) relies on functioning client-side script, there are similar ways to affect sweeping markup transformation without totally breaking Drupal's caching. They key is to ensure that the client and server side transform step can happen without making additional expensive calls like entity lookups or secondary bootstraps.

Also, it doesn't actually work yet.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

Re #30, you might just support the controller syntax that we use for routes.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#32: Interesting! Thanks for sharing that :)

I think a key difference is that you can output all potential outputs next to each other, and in your Symfony Response Filter you can then just strip away the stuff that doesn't apply.

In a nutshell: you generate content that applies to all roles, then strip away the content that doesn't match the current user's role. It's a solution for role-specific content.

But the key problem the current patch solves is this about user-specific content. We can't generate e.g. the "last read" timestamps for every user on the site and then remove those that aren't for the current user: that wouldn't scale.

Finally, if it's role-specific content that you're dealing with, you might as well leverage the render cache: that's role-specific by default. I think the reason you're not going that road is that your data-audience attributes are in a filtered text field, and filters don't have context. Since a filter won't do, you need to filter at a later stage, where you do have access to the request context, and that's apparently Symfony's Response Filter.
(To add context to filters, see #8000: Filter contexts (and fix some small issues in filtering) and #226963: Context-aware text filters (provide more meta information to the filter system).)

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

After going through this patch and thinking through some of the issues I had with it, I'm just not sure we can do much better.

The API is definitely a bit confusing, but documentation that emphasized the render_cache_placeholder approach could significantly improve that. Also, it's no more confusing (probably much less so) than the #pre_render cache pattern.

Most of my concerns are around the intelligence of the data structures, but I don't see how we can get away from that until we move away from render API altogether.

I don't see a way to improve the design without a very large refactor, so I'm ok with this as is.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I've had a look at this and I also think it's fine as is, and agree it's no more confusing than #pre_render. Also it's functionality that's very important for solving these particular issues.

eaton’s picture

eaton CreditAttribution: eaton commented

I think the reason you're not going that road is that your data-audience attributes are in a filtered text field, and filters don't have context. Since a filter won't do, you need to filter at a later stage, where you do have access to the request context, and that's apparently Symfony's Response Filter.

Yep, that's exactly what led us down that path. Also, Drupal roles are only the initial demonstration implementation we're working on. In the future, other audience identifiers (like Google advertising campaigns) could be used as audience targeting points.

Also, I want to make clear I wasn't proposing an alternative to the approach outlined in this patch. Rather, I was pointing out that this sort of approach is one that's useful in related use cases. Watching this thread with interest.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

spent some time with this patch, and i like it. i think it's the best we can do for now, and a real improvement on what we currently have. WimLeers++

i'm going to RTBC it given catch and msonnabaum have both signed off on it.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community
amateescu’s picture

amateescu CreditAttribution: amateescu commented

i think it's the best we can do for now

Did anyone besides Wim (and probably catch) look at the patch from #18?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

has #30 been addressed - if not are we sure we want to tie ourselves to purely procedural callbacks?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#41: Good question — no, #30 has not yet been addressed. In fact, that's the last thing we need to make a decision for.
The thing about supporting object method callbacks is that that inherently means serializing the object the method is on and storing that in the render cache… which means it'd be very easy to fill up the render cache much faster (and making render cache hits slower) than would be the case if we'd just support procedural callbacks. For that reason alone, I propose to stick to purely procedural callbacks.

(I worked on implementing this, and only then realized this problem.)

Thoughts?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

What if we use the controller notation like in routing files? Or we limit it to services, so we don't need to worry about instantiation.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Services sounds reasonable for that, not sure if it should be a follow-up or not so leaving RTBC for a bit longer.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Introduce #post_render_cache callback to allow for personalization without breaking the render cache » Change notice: Introduce #post_render_cache callback to allow for personalization without breaking the render cache
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Let's do this as is - might want to add service support for other things in drupal_render() as well.

Committed/pushed to 8.x, thanks!

Will need a change notice for the API addition.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

So #40 was left unanswered..

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

@amateescu: beejeebus said he reviewed the patch in #38.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Yes, he reviewed Wim's patch. My question was if anyone else looked at the other proposal.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

*crickets*

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs change record
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Change notice: Introduce #post_render_cache callback to allow for personalization without breaking the render cache » Introduce #post_render_cache callback to allow for personalization without breaking the render cache
Status: Active » Fixed
Issue tags: -sprint, -Needs change record

Change notice at https://drupal.org/node/2151609.

yched’s picture

yched CreditAttribution: yched commented

@Wim: great notice, thanks! I finally understood how #post_render works :-)

One nitpick : the explanation could be a bit more specific about what gets out of a cache hit.
From the "motivation for render cache" at the top, you'd expect that it's a flat rendered string.
Then, the first example (add an #attached['js']) works because it's actually a render array.
Then, the second example (switch placeholders) works because it's actually a render array containing the flat string as #markup :-)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#52: Thanks — glad you like it :) I see your potential confusion there: the change notice indeed assumes that you already know how render caching itself works. I've now added an additional explanation to explain the part you had to figure out for yourself: https://drupal.org/node/2151609/revisions/view/6758123/6762699 — thanks for the feedback!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Priority: Major » Critical

Retroactively marking critical: a blocker to a critical issue should itself be critical, and this blocked #2151459: Enable node render caching, which is critical.

yched’s picture

yched CreditAttribution: yched commented

Thnaks! @Wim++

Status: Fixed » Closed (fixed)

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Did we get this follow up:

Services sounds reasonable for that, not sure if it should be a follow-up or not so leaving RTBC for a bit longer.

I'm keen to kill comment_add() but we would need services in order for that to happen.
If not, let me know and I'll create it.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Added #2247779: Allow #pre_render, #post_render and #post_render_cache callbacks to be service methods as follow-up