[meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance

Great write up Wim. For what its worth, as someone who works with D7 in the D8 and tinkers on D8 in my spare time - I'm finding D8 feels faster already - keep up the great work.

I think we want the concept of 'global' cache contexts.

Motivation:

One of the biggest use cases in Drupal 7 for varying the page cache was 'device detection' based caching, e.g. on a cookie set by varnish.

While Varnish could well change the path to add a request parameter, such differing by path, the cache_context of a 'mobile version' is way more granular - as it only differs on the contexts of the cookie.

e.g. device = desktop | mobile | tablet

However usually this should apply globally as any code e.g. in a hook_preprocess_foo() {

}

could be changing this for mobile purposes.

While sometimes returning the cache context explicitly and bubbling it up is fine, having the possibility to define it globally is important, too.

This can also be used by sites knowing their cache profile to optimize certain cases.

Other possible global cache contexts that usually we want to always apply (for display and security reasons) are: theme, role

But some sites might decided to vary everything on 'language', too, e.g.
(@todo maybe create an issue for adding cache contexts globally)

On the other hand there might be sites that chose to remove cache contexts globally, e.g. that never want to differ on the users configured TZ.

We should think about allowing something for that _maybe_, though adding things to a global list of always applied ones, is more important IMHO.
(@todo maybe create an issue for removing cache contexts globally)

The global definition would ideally live in the container, so its:

a) fairly static
b) very reliable and usable at page middle ware level pre-boot and pre-routing.

In other words, an advanced page cache sitting at the middle ware level as explained in the doc, would probably make heavy use of the cache context hierarchy (issue needed), to propagate the mix of:

[ menu_trail:tools, book.navigation, users.roles ] to e.g. just [ 'page', 'users.roles' ] and only take the most upper level cache contexts into account that can be executed with very low overhead.

The cookie context described would fall into the same category as 'cookie:device' is an operation that is ideally would just do something like return 'cookie.' . XSS::checkPlain($_COOKIE[$name])); (@todo Make an issue for a generic cookie cache context service? might be useful for such cases?)

Having fairly simple cache contexts is also important for any ESI / AJAX implementation that needs to pass those (authorized) contexts via cookie or HTTP headers to keep those simple.

Added #2449459: [PP-1] Make URLs cache context aware to the IS, needs discussion, especially a render scope could make many things simpler.

The core committers discussed this and since this meets the definition of a performance critical it is in fact critical. Tagging.

#1805054-141: Cache localized, access filtered, URL resolved, and rendered menu trees says it is postponed on this. Adding that to this issue summary.

Added #2463581: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age as a new important bug for this issue.

Tentatively tagging for the Performance sprint at Dev Days coming up.

removing duplicate word.

Added #2469277: Changing #cache keys during #pre_render or anywhere else leads to cache redirect corruption

Added a first rough prototype of BigPipe functionality here: #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts

Opened #2470715: cacheGet-case: #post_render_callback's that result from other #post_render_calback are not processed

Added #2489966: The Views table style plugin does not specify cache contexts for click sorting to the IS.

Created #2495001: [meta] Add a replacement for renderRoot() that returns a cacheable render array instead of a string

On #606840-133: Enable internal page cache by default I brought up the issue of timezones, and I was redirected to this meta issue.

I was wondering about date/time rendering using normal date/time formats (as opposed to "time ago" formats, which are a separate problem). This rendering is always timezone dependent -- it can depend on the setting for the overall site timezone, or if the setting that says "let the user choose time zone" is active, then it's dependent on the user's own time zone setting.

Are we capturing this adequately? Wim Leers mentioned on the other issue that there is a timezone cache context to handle this. But I didn't know about it. So:

a) It seems like we should add API documentation to all of our date format functions/methods mentioning that if you are using this output in a render array (which presumably you would be), you need to put a time zone cache context on that render array element.

b) If it hasn't been done already, we need to make sure that all Core uses of date formatting do this, such as the Views date/time field handlers and Field API date/time field formatters... and then any other calls making render arrays out of Field API and Views, if we still have some and I think we do.

c) Regarding the time zone cache context... hopefully that is being handled properly, depending on the time zone and user time zone settings? I would hope/assume so.

a) I'm not sure this is viable everywhere - we have/need documentation of the different cache contexts generally, but I feel like that belongs more in the render API documentation (i.e. 'when building a render array take care of cache contexts, for example timezone' as opposed to the individual date formatting functions.

b) has been done afaik, for example https://api.drupal.org/api/drupal/core%21modules%21datetime%21src%21Plug...

And c) we use date_default_timezone_get() which respects the user's timezone (because we set it early-on in the request). So yes this works fine.

Wow, nice page there on Cache Contexts! There's not a mention of them on the api.d.o topic on the Cache API -- do you think we should add something short (and point to that page for more info)?
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...

We do have a quick (appropriate) mention of cache tags, keys, context, and age on the Render API topic page, so that's good:
https://api.drupal.org/api/drupal/core!modules!system!theme.api.php/grou...
And that leads to
https://www.drupal.org/developing/api/8/render/arrays/cacheability
for more information. That page specifically mentions cached contexts and specifically the timezone context. Not only that, it links to the Cache Contexts page mentioned in #80 for details, so that is perfect.

And agreed that my suggestion of adding docs to the date format functions/classes is too much; it does belong in the render/cache docs instead.

So... Good! Given all of this, I think all we should do is add a short section to the Cache topic page on api.d.o about cache context, linking to https://www.drupal.org/developing/api/8/cache/contexts for more information, and we'll be good. So I filed
#2500443: Cache API topic says nothing about cache context, add something
for that. I'll assign it to myself for now, especially if Wim agrees to review it (or vice versa -- Wim feel free to take it on if you have time, as I might not for a week or so).

Thanks for thinking this through, and Wim I think you wrote most of those docs (with some help) -- great job, everyone who worked on them!

One thing which would be great to do here is to identify which subissues could get critical at some point.
Those subissues are issues which makes things impossible to fix in 8.1.x or contrib because they would involved necessary API changes.

Tentatively testing our new "Plan" issue type now that #2504039: Adjust critical issue counter/link at /drupal-8.0/get-involved to account for "Plan" issue category is in.

My main worry about this issue is that it's not clear which sub-issues are critical and which aren't. The new structure is certainly an improvement and helps, but it's not quite enough yet I think.. I think we agree that not everything here is release-blocking, but I'm not sure what...

IMHO, sooner or later, we need to do what we did with many other critical meta issues already.. mark the actual blocking issues as critical and downgrade this to major to have an overview. Will bump the critical count again a bit, but that's also more honest, and on the plus side, will make progress here more visible in that number ;)

Sounds like a good task for the upcoming critical sprint :)

Well, critical has to be, what can't be done between 8.0 and 8.1

From my humble point of view, just #2487600: #access should support AccessResultInterface objects or better has to always use it is really a big API change
The following bits like #2407195: Move attachment processing to services and per-type response subclasses #2443323: New convention: CacheContextInterface implementations should mention their ID in their class-level docblock and #2463581: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age are more like a nice to have

@wim
It would be indeed nice if you could categorize them rather in what you think needs to be done and what is optional ...

In which sense are big pipe and smart cache a must have?

Right I'd probably divide things up like this if it's useful:

Smartcache/BigPipe blockers that must happen before 8.0.0 (->critical)
Smartcache/BigPipe blockers that can happen in minor releases (->major + minor version target - we can still try to get these in before 8.0.0 but wouldn't block on them)
Smartcache/BigPipe blockers that can happen in patch releases (-> major)

Nice to have - must happen before 8.0.0 or move to 9.x
Nice to have - can happen in patch or minor releases.

Yeah sure, I don't question that at all, but I 'd like to see us focusing on what is needed in order to release Drupal 8.0.x, not what would be also nice in terms of authenticated user performance.

I'd like to get SmartCache into 8.0.0 if we can - even if it's close to release candidate. i.e. I wouldn't want to bump it to 8.1.x until the very last minute.

However as long as we're confident we're able to enable it in a minor release (i.e. no critical blockers and major blockers well documented/fleshed out), then I don't consider it critical in itself.

What I absolutely want to avoid is that we release 8.0.0 with an issue like #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed not properly documented/scoped and get a nasty surprise later.

Looks like #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity is in there twice now.

Other than that, looks like #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts is the only "must have" that isn't itself critical, which means the only "meta" part of this issue that remains critical is to determine if there are any critical blockers to that and make sure those are promoted to critical. Am I understanding that correctly?

Finally added #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case to the list.

I think we should check / assert whether this could lead to security problems ASAP.

If we decide its the callers responsibility, that is fine, but we should at least take a look ...

After thinking about #122 opened:

• #2526470: Audit core for things missing cacheable metadata (for authenticated users)
• #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable

I don't think they need API changes (maybe API addition for forms), but are important should-have's to reduce the unknown-unknown's.

If there's agreement on #124, let's get those into the right section of the issue summary.

Found a major bug:

* EntityViewBuilder::viewMultiple() is _not_ compatible with render caching.
* This only works, because we use views to render entities and views uses EntityViewBuilder::view() instead of viewMultiple(). (which is pretty surprising, too).

This bug is present since the original patch to add render caching to entities :D.

The reason is that the following array is returned for multiple entities:

[
  '#pre_render' => [EVB, 'buildMultiple'],
  '31 '=> [...],
  '32' => [...].
];

but the 31, etc keys contain the #cache keys entries ...

This is pretty surprising, because:

Overall we go to great lengths to ensure that buildMultiple() works efficiently with all the view modes, etc. and nothing in core uses it ...

Weird.

IIRC, we pretty much gave up on a performant view multiple when we switched to using pre_render for the field formatters. I remember that that was discussed back then.

Weird indeed.

viewMultiple() is important because it runs formatter's prepareView() on all the entities at once, making it possible to multiple-load referenced entities for ER fields.
Originally it was used by the various "hardcoded listing pages" like /node, taxonomy/[tid], but most of those progressively moved to Views. It is however very unfortunate that Views doesn't use viewMultiple(). Apparently this was changed in #2099131: Use #pre_render pattern for entity render caching.

IIRC, we pretty much gave up on a performant view multiple when we switched to using pre_render for the field formatters. I remember that that was discussed back then

Hmm - can you pretend I'm a total noob and axpand on that a bit ? ;-) why does pre_render kill performant viewMultiple() ?

Hm so yeah, this slowly comes back to me. We basically gave up on Views leveraging multiple-entity for formatter's prepareView() in #2099131: Use #pre_render pattern for entity render caching, with prepareView() now running as part of the #pre_render of each entity render array, instead of as part of the buidling of the array for all entities previously.

See #169 and #177 over there - in short, answer by @Wim back then :

Once we get to the point where Views bubbles up cache tags correctly, we'll be able to render cache the entire View output. So this [building formatters's prepareView() entity by entity] will become moot.

Not sure whether we have reached that point ? (Views bubbling up cache tags correctly) ;-)

So in the end, formatters (notably ER formatters) go through the non-trivial FormatterInterface::prepareView() API and logic to optimize for multiple entities, but our main listing builder, Views, ditches that anyway, because it runs entity-by-entity and render-caches on a higher level. This kind of questions the shape of the Formatter API and the interest of the convoluted prepareView() construct now :-/
Also, the building time on cold caches or cache misses is much much worse, since ER fields load referenced entities one by one. But yeah, that's kind of the deal with building formatters at #pre_render time.

However, there are still non-Viewified pages that use EVB::buildMultiple() in core atm (and it's still part of the API), so it seems we need to fix it if something's wrong...