Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)

The first PoC (also linked in the Google Doc): just dealt with Response objects. Hence was breaking things, because it was also caching the result of #post_render_cache callbacks (e.g. the comment form on a node). (*-poc0.patch)

The second PoC, then, fixed that, by only caching the top-level #type => HTML render array. But it was still only a hack of HtmlRenderer. Which means the controller was still being resolved, the main content render array was still being built (and then discarded), etc. Still, this already yielded a nice performance boost. (*-poc1.patch)

Environment: OpCache on, PHP 5.5, XDebug off, XHProf on. When the run say "noapcu", that means APCu and the APC classloader are both off, otherwise both are on. Fresh install, one node, with one term. Viewed as root.

APC on, frontpage

	Run #apcu-HEAD-front	Run #apcu-POC-front	Diff	Diff%
Number of Function Calls	66,143	29,594	-36,549	-55.3%
Incl. Wall Time (microsec)	163,740	79,967	-83,773	-51.2%
Incl. MemUse (bytes)	23,275,144	14,958,464	-8,316,680	-35.7%
Incl. PeakMemUse (bytes)	23,783,984	15,144,352	-8,639,632	-36.3%

APC off, frontpage

	Run #noapcu-HEAD-front	Run #noapcu-POC-front	Diff	Diff%
Number of Function Calls	74,623	34,180	-40,443	-54.2%
Incl. Wall Time (microsec)	183,692	91,946	-91,746	-49.9%
Incl. MemUse (bytes)	24,710,168	16,036,000	-8,674,168	-35.1%
Incl. PeakMemUse (bytes)	25,184,944	16,218,360	-8,966,584	-35.6%

APC on, /node/1

	Run #apcu-HEAD-node	Run #apcu-POC-node	Diff	Diff%
Number of Function Calls	88,402	54,476	-33,926	-38.4%
Incl. Wall Time (microsec)	193,594	123,133	-70,461	-36.4%
Incl. MemUse (bytes)	24,320,016	19,332,640	-4,987,376	-20.5%
Incl. PeakMemUse (bytes)	24,780,296	19,604,160	-5,176,136	-20.9%

APC off, /node/1

	Run #noapcu-HEAD-node	Run #noapcu-POC-node	Diff	Diff%
Number of Function Calls	95,099	59,148	-35,951	-37.8%
Incl. Wall Time (microsec)	208,204	135,374	-72,830	-35.0%
Incl. MemUse (bytes)	25,411,320	20,267,632	-5,143,688	-20.2%
Incl. PeakMemUse (bytes)	25,894,544	20,496,744	-5,397,800	-20.8%

Next comment: the latest PoC, which significantly improves upon these numbers still!

And here is the latest PoC (the profiling data of which is already in the IS). This one actually complies with the algorithm that Fabianx & I came up with, as is explained in the IS.

This makes Drupal 8 twice as fast!

The patch that includes the depended patches includes #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations (currently RTBC) and #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations (which depends on #2329101).

APC on, frontpage

	Run #apcu-HEAD-front	Run #apcu-betterPOC-front	Diff	Diff%
Number of Function Calls	66,143	26,937	-39,206	-59.3%
Incl. Wall Time (microsec)	163,740	66,382	-97,358	-59.5%
Incl. MemUse (bytes)	23,275,144	12,187,816	-11,087,328	-47.6%
Incl. PeakMemUse (bytes)	23,783,984	12,372,352	-11,411,632	-48.0%

APC off, frontpage

	Run #noapcu-HEAD-front	Run #noapcu-betterPOC-front	Diff	Diff%
Number of Function Calls	74,623	29,687	-44,936	-60.2%
Incl. Wall Time (microsec)	183,692	76,266	-107,426	-58.5%
Incl. MemUse (bytes)	24,710,168	13,065,392	-11,644,776	-47.1%
Incl. PeakMemUse (bytes)	25,184,944	13,246,776	-11,938,168	-47.4%

APC on, /node/1

	Run #apcu-HEAD-node	Run #apcu-betterPOC-node	Diff	Diff%
Number of Function Calls	88,402	51,561	-36,841	-41.7%
Incl. Wall Time (microsec)	193,594	113,076	-80,518	-41.6%
Incl. MemUse (bytes)	24,320,016	17,806,776	-6,513,240	-26.8%
Incl. PeakMemUse (bytes)	24,780,296	18,057,056	-6,723,240	-27.1%

APC off, /node/1

	Run #noapcu-HEAD-node	Run #noapcu-betterPOC-node	Diff	Diff%
Number of Function Calls	95,099	56,037	-39,062	-41.1%
Incl. Wall Time (microsec)	208,204	130,393	-77,811	-37.4%
Incl. MemUse (bytes)	25,411,320	18,790,512	-6,620,808	-26.1%
Incl. PeakMemUse (bytes)	25,894,544	18,998,472	-6,896,072	-26.6%

#5: Of course! Thank you! :) That allows me to remove a significant portion of the hackiness :) 1 KB smaller patch.

The depended patches have landed. Re-uploading the #7 patch without a single change, but now without the dependencies.

If this passes, then it's only because the test coverage elsewhere is insufficient. It should fail. Every failure points will point us to a missing cache context.

We are already aware of many of the missing cache contexts, and are fixing that in child issues of #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance. If you want to stay up-to-date on fixing those blockers, follow that issue.

Huge, huge numbers of failures caused testbot to take a whopping THREE HOURS instead of 20 minutes. Which means the test coverage is catching many problems, which is exactly what we hoped for (per #10). Yay!

Now actively working on the child issues of the meta to make this green.

Yes, I'm afraid #13 — #17 is a distraction that actually complicates this issue, rather than moving it forward.

My bad for not making this even more explicit than I already was in #12. Now assigning to myself also to signal that I'll reroll this when it's ready.

Update!

Drupal 8 performance today

In only a few days, it's Drupal Dev Days Montpellier. We'll have a performance sprint there, and we basically want to reduce the number of unknown cold cache & bootstrap performance problems. We already know where we spend a lot of time in rendering, and we already have a plan to reduce that: SmartCache (this issue) and BigPipe (see https://docs.google.com/document/d/1Gw7ohBOUKu38t4kMbN9zj6cX-4-_2ZNXGotv... + https://www.drupal.org/node/2429287).

By doing a lot of profiling work, and analyzing the profiling, writing quick prototypes to see where we can make gains and how many milliseconds we can regain. The difficulty with that in HEAD is that it's often quite difficult how much time is spent where, because we spend a significant amount of time rendering.

Profiling at DDD

That's why we want to apply this patch at DDD and do profiling with it applied. It still shows everything for rendering an entire page, including asset handling, but minus the big amount of time spent actually rendering the contents. This allows us to see the threes in the forest again.

To be able to do that, we want this patch 1) to apply, 2) to be able to install Drupal, 3) to get meaningful test results. Lots of work has already been done in #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance, which should make it possible to get testbot to run the entire test suite and "just" come back with lots of failures :)

Next steps

1. Post a straight (rebased) reroll.
2. Post a patch that is successfully run by testbot.
3. Then mark this issue as postponed again.
4. Finally, post the results of a round of profiling.

This is #23.1.

This is #23.2.

About this reroll/these changes:

1. #24 is able to install Drupal. #10 was already able to.
2. Many of the failures were due to SmartCache also intercepting POST requests. It shouldn't do that. Fixed.
3. Some of the failures were due to SmartCache also intercepting non-HTML requests. It shouldn't do that. Fixed.
4. The SmartCache patch predates the addition of #cache[max-age] and thus didn't actually look at that yet. Now it does. The wonderful consequence of adding max-age support to SmartCache and the fact that max-age is bubbled: any page that has anything that's not cacheable will automatically skip SmartCache.

Right now, the search block and breadcrumbs block both set max-age = 0, so almost every integration test using the Standard install profile is effectively bypassing SmartCache. Those blocks already have @todos to stop setting a maximum age of zero.

(A strategy for figuring out which pages are still setting inappropriate zero max ages would be for SmartCache to ignore the max-age and always cache the HTML response. Then we'll have test failures much like #606840: Enable internal page cache by default, most of which will be pages that indeed should be cacheable.)

This is #23.3.

#23.4 is for tomorrow.

#28: you're right, this isn't the right time. So I'm not going to address it point by point, but only the big remarks :) Hope that's okay.

• #28.1: class SmartCacheHtmlRenderer extends HtmlRenderer -> it interjects additional logic in a strategically chosen place :) We override HtmlRenderer::finish(), then call the parent method to resume as per usual.
• #28.3: Yes, I know. I've been frustrated by this too. I wish we/I had the foresight to give it a better name back when it was added more than a year ago. But at the same time, I can't think of a better name. If you can, please file a new issue!
• #28.5.A: you're right. We should let SmartCacheHtmlRenderer::finish() check if #type === 'html, and if so, immediately skip to HtmlRenderer:finish()
• #28.5.B: Yes, you're right.

I can't stress this enough: the patch was the simplest thing that could possibly work. It can be cleaned up. It can be simplified. That's why I found and fixed so many silly mistakes #26 :) Clearly, it needs to be cleaned up/simplified/optimized more, but what matter is that it proves this is viable! :)

#28 Sorry for the weird tone. It was very late and I was about to go to bed — I simply shouldn't have written that. Having slept, and being actually awake, I think it actually makes sense to already incorporate all of your feedback :)

#28:

1. See #29.1.
2. Fixed :)
3. See #29.3. Let's fix that. Filed #2468151: Rename the CacheContexts service to CacheContextsManager, and included a proposal there.
4. Removed the problem by closing this todo in the IS: Let smartcache use CacheContexts::optimizeTokens().
   When this patch was first rolled, we didn't have a route cache context yet. And we also didn't yet have a hierarchy of cache contexts. The hierarchy helps to keep the cache ID simpler. In the case of SmartCache, we know we are caching by route, so any cache context that is implied by the route cache context can be optimized away. See https://www.drupal.org/node/2451661. For this, that's usually going to be the active menu trail cache contexts.
5. Calculating the CID of where to store the cache contexts for this route needs to happen in both places. And yes, on a cache miss, that means it'll happen twice during one request.
   But you're completely right that the current code still goes through everything that SmartCacheHtmlRenderer does! It's not going to be a huge amount, but still, we were definitely doing useless work! Which means this reroll should be even faster than the profiling shown in the IS.
   AWESOME! Thanks, yched! :)

In this reroll, I've also added a fun snippet:

    // @todo DEBUG DEBUG DEBUG PROFILING PROFILING PROFILING — Until only the
    //   truly uncacheable things set max-age = 0 (such as the search block and
    //   the breadcrumbs block, which currently set max-age = 0, even though it
    //   is perfectly possible to cache them), to see the performance boost this
    //   will bring, uncomment this line.
//    $html_cache_max_age = Cache::PERMANENT;

So when you do profiling with this patch applied, you have to uncomment that line.

#26 has significantly fewer test failures already! (But it still causes testbot to crash at the end, so you actually have to look at the results log for e.g. the patch in #10 and then compare it manually. The difference is quite large.)

Quite a few test failures are happening for admin routes. And since it may be too much work to add the necessary cacheability metadata to all admin routes before 8.0.0, and adding that metadata can easily happen in 8.1.0 without a BC break, I think it makes sense to make SmartCache skip admin routes, even if just for now.

Let's see what testbot thinks about that.

Hurray! 644 failures, 77 exceptions. Finally we are getting actual results from testbot :)

The best part: all of those failures indicate places that are missing some cacheability metadata: either missing cache contexts or a missing max-age = 0. So we can apply the same strategy as we used for #606840: Enable internal page cache by default: file child issues to fix those specific failures.
We already have #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance for that, though, so now postponing again.

And this is #23.4, finally. The promised round of profiling. Profiled D8@a4e51c82e60fca8f9e46338fd2bf8c080fee019e.

Thanks to @yched's remark in #28.5, this became even slightly faster :)

I only profiled the frontpage, with APC on. This can be compared to the first table in #4, which I'm repeating here to simplify comparing.

#4

	Run #apcu-HEAD-front	Run #apcu-betterPOC-front	Diff	Diff%
Number of Function Calls	66,143	26,937	-39,206	-59.3%
Incl. Wall Time (microsec)	163,740	66,382	-97,358	-59.5%
Incl. MemUse (bytes)	23,275,144	12,187,816	-11,087,328	-47.6%
Incl. PeakMemUse (bytes)	23,783,984	12,372,352	-11,411,632	-48.0%

#32, with the @todo uncommented

	HEAD	SmartCache	Diff	Diff%
Number of Function Calls	75,608	28,172	-47,436	-62.7%
Incl. Wall Time (microsec)	164,111	68,736	-95,375	-58.1%
Incl. MemUse (bytes)	22,761,680	12,078,640	-10,683,040	-46.9%
Incl. PeakMemUse (bytes)	23,098,544	12,207,232	-10,891,312	-47.2%

And finally, here's a preliminary analysis, which we will continue at DDD Montpellier.

Because so much less time is spent on rendering, we can now see problems that we hadn't seen before.

• Of that 68 ms, 5 ms (or 7.6%) are spent in Drupal\Core\Asset\AssetResolver::getJsAssets(). That's inclusive wall time, but still: that seems excessively much, for something that really is quite simple. Similarly, Drupal\Core\Asset\AssetResolver::getCssAssets() takes 4 ms (or 6.1%). If we can get both down to just 2 ms, that'd be another 5 ms saved, or 7% faster (on top of SmartCache).
• Also ridiculously expensive are Drupal\Core\Theme\ThemeManager::getActiveTheme() and Drupal\Core\Theme\ThemeManager::initTheme(): each 3 ms, or 4.8% of page load time. Again, inclusive wall time.

I now also generated Flame Graphs. Note how This allows us to see the threes in the forest again. is also true here: the very early bootstrap becomes much more visible, DrupalKernel::boot() goes from 1.49% to 3.17% of total page generation time, and ::getHttpKernel() from 1.49% to 4.37%.

Drupal.org sadly doesn't allow SVGs to be attached or embedded. So just download the attached zip file to look at them.

#41: there are *two* zips. The "FlameGraphs.zip" one is the one you want :)

Chasing HEAD.

Some rebase cruft snuck into #43.

MiSc: no further profiling is necessary for now, this is still in the PoC stage.

All of you who would like to help here: please look at the test failures in #32, pick a specific test, file an issue for fixing that test failure, figure out why it's failing, and post your analysis, hopefully along with a patch. Feel free to ping me on IRC if you have questions. Thanks! :)

Just chasing HEAD.

… and now properly chasing HEAD. I only fixed the conflict in #49, didn't actually try running it.

Add a "how it works" section to the IS.

More clarifications, and a "how it relates to the BigPipe issue" section.

Forgot a few bits.

Chasing HEAD.

One super simple change to significantly reduce the number of test failures (down from 644 in #32): mark non-GET forms as uncacheable. It seems acceptable/reasonable to defer updating every single form in Drupal core to a major follow-up:

• it allows us to move forward here
• all pages that don't contain forms can already benefit
• when #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts lands, forms would simply be placeholdered
• worst case: shipping Drupal 8.0.0 with forms being uncacheable by default … that seems reasonable

Let's see how many failures we're at now.

SmartCache was breaking page titles defined in render arrays. Fixed that. This fixes at least 10 test failures.

Nice, it fixed 75 failures :)

I investigated the remaining failures, and already found two fairly simple issues:

• #2484611: Tracker responses don't set cache tags & contexts
• #2484619: Forum responses don't set cache tags

#2484611: Tracker responses don't set cache tags & contexts is already RTBC!

Two more child issues: #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user & #2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user.

Clearer "remaining tasks" section.

We had a (very long) discussion about the security implications of this issue at DrupalCon LA with Mark Sonnabaum, Wim Leers, Crell, dawehner, webchick, Alex Pott and fgm.

The problem

If a piece of code (an access result, a function building a render array …) forgets to add a certain cache context, it's possible for information disclosure to happen: if something should only be accessible for users with role A, and the user.roles cache context is missing, then if a user with role A first accesses the content, and then a user with role B accesses it, then the user with role B will be able to see the content.

Clearly, that is an important concern. But, the same underlying problem exists in Drupal 7 if the cache keys don't take all dependencies into account (in block cache, views cache and render caching). The key difference is that this affects all content on the page, rather than only fractions of it.

More importantly: any caching has security implications: if the cache ID is not taking some things into account, then there is the potential for security problems. By that reasoning, we should be doing almost no caching at all. The risk is lower for well-structured, well-defined renderables such as entities and blocks, because Drupal core does all the rendering, and is well-tested. Custom controllers usually are not quite as well tested. So we want to reduce the risk for those as well.

The solution

1. Document security considerations for custom/contrib modules (and custom especially): those who don't have the resources to either do the necessary QA nor write the necessary test coverage should opt out by setting max-age = 0.
2. Add another default required cache context: user.permissions, this seriously diminishes the risk, because permissions are by far the most common factor in determining the visibility (access) of certain content.
3. Tool: renderviz: give developers an x-ray to see the cacheability of everything on the page.
4. Tool: "render audit": wrap services to automatically warn/suggest cache contexts to the developer. (Wrap services, if a service is used and the matching cache context is not being added, then warn the developer.)

This diminishes the risks sufficiently to

I've been working on addressing all feedback. But, before posting that, I want to reupload the patch in #57, because interestingly, many of the failures are now actually gone. In no small part thanks to the many bugfixes that went in in the past 1.5 week that were blocking #2381277: Make Views use render caching and remove Views' own "output caching".

So, uploading a rebased #57. #57 had 124 failures. This should have at least 32 less failures, so 92 at the most.

In this reroll:

1. Implemented #62 (made user.permissions another required cache context). This required a whole bunch of test changes.
2. Added test coverage: SmartCacheIntegrationTest.

Note: if people think that's a good idea, I could move point 1 into a patch of its own, that'd make this patch much easier to review again. (As easy as #57/#75.)

#63:

• a) Indeed; caching is an afterthought in D7, in almost every aspect, with all the associated bad consequences. (It happens on production, it isn't properly tested, it's definitely not tested for security.)
• b) Indeed, everything is cacheability-aware in D8, including filters.
• c) Indeed, max-age = 0 in D8 opts out of all caching at the level where you set it and at all higher levels. Which maps 1:1 to how HTTP works.

#64: I disagree. For three reasons:

1. The most important reason: opt-out is much better than opt-in, because it forces developers to think about this. Otherwise, it's going to be far too easy for developers to just not think about this and make the Drupal 8 ecosystem slow.
2. CacheableDependencyInterface doesn't make sense for controllers, because to know the cacheability of the controller's output, you need to build the render array, and render it… Plus, very often, a single class contains many controller methods. But the interface can obviously only be implemented once.
3. Hence the only possibility left is to use an empty interface for signaling, but then the "multiple controller methods" problem still exists.

Wow, #66—#74 are quite the distraction. It's good stuff for sure, but it's not at all specific to this issue. Answering to key questions/points succinctly.

1. #66: Yes, Google Analytics is installed on most sites. But I strongly doubt most of those sites actually use all that advanced functionality. I certainly don't.
2. #67: Related: see the docs at https://www.drupal.org/developing/api/8/cache/contexts.
3. #68: absolutely, but with the #69 caveat. As soon as things become very dynamic (per user), then it'd be much, much better to use cookies instead (e.g. per-user tracking, opting out from tracking, etc.).
4. #71: What is a custom cache context? — see https://www.drupal.org/developing/api/8/cache/contexts + CacheContextInterface + the implementations in core of that interface.
5. #72: No, SmartCache applies basically to the HTML <body>, i.e. everything in page.html.twig. The rest (i.e. html.html.twig) is still generated dynamically. So, it's perfectly possible for the Google Analytics module to define a #post_render_cache callback that calculates the dynamic drupalSettings for the current request/user. This is also what we do for node links, comment links, the comment form and … the history module. The history module actually is the closest example to what you need: see history_attach_timestamp(): it adds a setting with data specific to the current user.

I've been working on removing as much logic as possible from SmartCache, and reuse RenderCache(Interface) instead. After all, this is just another implementation of the cache redirection logic. By reusing the existing logic, we don't need to write the extensive, detailed test coverage for that again.

However, I'm not yet satisfied with it, so here's a WIP interdiff. Fabianx has ideas on how to make it simpler/cleaner. This should be the bulk of the conversion work though. Interdiff is applied to #76.

#79.1 + #80.1: child issue created: #2493033: Make 'user.permissions' a required cache context. That allows me to remove that part out of #76, which should bring the number of failures down to 96 again. The interdiff is basically the reverse of #2493033.

#79.2 + #80.2: we already have a meta issue for that: #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance. But, I'll repeat what I said in #76: 99% of what is being asked in #66 has a very simple answer: use #post_render_cache and look at history_attach_timestamp() for an example.. (But, note that #post_render_cache is being removed in favor of a simpler concept, in #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache.)

Also filed #2493021: Remove unused & useless services from HtmlRenderer, which should allow this patch to become slightly simpler.

And also filed #2493047: Cache redirects should be stored in the same cache bin, which I discovered as part of #81.

Making SmartCache obey the no_cache route option further reduces the number of failures.

Removes another >=15 test failures.

SmartCache is storing rendered output. It should therefore also specify the 'rendered' cache tag. This further reduces the number of test failures.

ThemeSuggestionsAlterTest was failing due to the system.theme config not invalidating the rendered cache tag. Fixing that removes another 9 failures.

When the page cache killswitch is used (e.g. when an unhandled exception occurs), we shouldn't be caching the associated HTML render array in SmartCache. In other words: besides respecting the no_cache route option, and excluding _admin_route routes, we should also respect the page cache kill switch, and more. And all of that is already encapsulated in request & response policies!

Therefore, let us reuse that same infrastructure/API, but introduce request/response policies for SmartCache that are separate from PageCache. After all, PageCache is about caching responses for anonymous users, whereas this is about caching partial responses for *all* users. Hence the key change in this patch is this:

-    // Don't cache routes that have marked themselves as uncacheable.
-    // @see \Drupal\Core\PageCache\ResponsePolicy\DenyNoCacheRoutes
-    if ($this->routeMatch->getRouteObject()->getOption('no_cache')) {
-      return parent::finish($html);
-    }
-
-    // @todo For now, SmartCache doesn't handle admin routes. It may be too much
-    //   work to add the necessary cacheability metadata to all admin routes
-    //   before 8.0.0, but that can happen in 8.1.0 without a BC break.
-    if ($this->routeMatch->getRouteObject()->getOption('_admin_route')) {
+    // Don't cache the render array if the associated response will not meet the
+    // SmartCache request & response policies.
+    $response = new Response();
+    $request = $this->requestStack->getCurrentRequest();
+    if ($this->requestPolicy->check($request) === RequestPolicyInterface::DENY || $this->responsePolicy->check($response, $request) === ResponsePolicyInterface::DENY) {

This also makes it easy for sites/developers to add additional generic exclusion rules if they'd so choose.

This fixes the failures in ErrorHandlerTest (5) + PagerTest (4).

+1 on making it an opt-in feature per controller. If we can make this work for entity view and views, then we should already cover a huge amount of requests for most sites, and if they have custom stuff they can always opt-in to benefit from this as well.

-1.

See #76's reply to #64 for my rationale. In short: it needs to be opt-out, not opt-in, if we want the D8 ecosystem to actually support it. Otherwise we end up with the same as render caching in D7: core has the API, but nothing actually uses it.

Also still wondering if this should be a module, just like page_cache, then it would be easy to turn off...or actually, part of the page_cache module

PageCache is conceptually simple. SmartCache is conceptually much more difficult to explain. Therefore I don't think we want to expose this as a module, because it will simply be too intimidating.
I propose we keep it in core, but add the necessary services/service modifications through a compiler pass, and allow a container parameter to prevent that compiler pass from running, hence disabling SmartCache completely if that container parameter is set to false.

Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].

Sigh. Re-testing.

The first failing test (BlockTest) is failing because block visibility conditions don't set the necessary cache contexts. There's an existing issue for that: #2375695-42: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed. That is now another blocker for this.

#99: Excellent point. Except that that is already happening in BlockAccessControlHandler:

      // This should not be hardcoded to an uncacheable access check result, but
      // in order to fix that, we need condition plugins to return cache contexts,
      // otherwise it will be impossible to determine by which cache contexts the
      // result should be varied.
      // @todo Change this to use $access->cacheUntilEntityChanges($entity) once
      //   https://www.drupal.org/node/2375695 is resolved.
      return $access->setCacheMaxAge(0);

Excellent!

But, then \Drupal\block\BlockRepository::getVisibleBlocksPerRegion() causes a lot of sadness:

    foreach ($this->blockStorage->loadByProperties(array('theme' => $this->getTheme())) as $block_id => $block) {
      /** @var \Drupal\block\BlockInterface $block */
      // Set the contexts on the block before checking access.
      if ($block->setContexts($contexts)->access('view')) {
        $full[$block->getRegion()][$block_id] = $block;
      }
    }

i.e. access results' cacheability metadata for blocks never makes it into the render arrays of blocks. I did some git history debugging; this was just never tested; this max-age = 0 thing originates in the original "access result cacheability metadata" issue, which just updated all code, and didn't add test coverage for every particular use of it (#2287071: Add cacheability metadata to access checks). Because that was out of scope.

Filed a critical security bug #2495171: Block access results' cacheability metadata is not applied to the render arrays to fix this. Which means this is now blocked on #2495171, not on #2493033. Adjusted the IS.

Here's a straight reroll/rebase now that #2493021: Remove unused & useless services from HtmlRenderer has landed.

So #100 is very roughly what we want in #2495171: Block access results' cacheability metadata is not applied to the render arrays.

I updated the IS incorrectly in #101. Fixing that.

#2493091: Installing block module should invalidate the 'rendered' cache tag landed, which should remove the failure in TrustedHostsTest. Re-testing for that. The patch is currently at 59 failures, should go down to 58.

With menu render caching having landed, I was also able to get #2254865: toolbar_pre_render() runs on every page and is responsible for ~15ms/17000 function calls going again, which should go a long way in helping to address the 14 test failures in ToolbarAdminMenuTest.

Hah, looks like that actually fixed a bunch of other failures as well. New low: 55 failures :)

Adding #2500013: Add cacheability metadata information to translation overview to the IS. Thanks for that one! That'll push the number of remaining failures down significantly. :)

#2484611: Tracker responses don't set cache tags & contexts landed!

Re-uploading a rebased #100 patch, without any other changes. This should reduce the number of failures by 6. If no new failures were introduced, then this should bring it down to 49 failures :) Let's find out!

#2470693: Upgrade to Symfony 2.7.0 caused those many failures and impressively many exceptions. Simple fix :)

+++ b/core/lib/Drupal/Core/Render/MainContent/SmartCacheHtmlRenderer.php
@@ -174,12 +172,12 @@ protected function finish(array $html) {
-//    $html_cache_max_age = Cache::PERMANENT;
-
+    //   $html_cache_max_age = Cache::PERMANENT;
+    //

This is the only change I disagree with. And I disagree strongly.

This concatenates the two comment blocks together, as if they logically belong together. They do not.

And the changed indentation — from bizarre to the usual — is also bad, precisely because it was intended to be so bizarre. It makes it stand out. This part is there fore testing purposes only, it is not actually being proposed to be committed, so that's why I want to keep it standing out by being bizarre.

Reverted this, kept the rest. Thanks!

By fixing those two things — which basically boil down to chasing HEAD — we should be seeing the effective consequences of #2484611: Tracker responses don't set cache tags & contexts having landed: if I counted correctly, we should be down to 47 or less failures.

Yay, down to 39 failures! :)

Also, with #2484611: Tracker responses don't set cache tags & contexts landed, the issue title needs an update :)

#2484619: Forum responses don't set cache tags landed, which should fix one more failure! Re-testing.

Periodical rebase. Also to check something for the child issue #2500013: Add cacheability metadata information to translation overview.

#2483781: Move cache contexts classes from \Drupal\Core\Cache to \Drupal\Core\Cache\Context, #2480811: Cache incoming path processing and route matching, #2407195: Move attachment processing to services and per-type response subclasses caused quite a few things to change. The latter has allowed for a significant clean-up/simplification. While working on this, I also noticed a small oversight, opened #2512382: Follow-up for #2407195: #attached['http_header'] being added to Response in two places for that.

From 38 (in #132) to 33! A new low :)

The 3 failures in Drupal\system\Tests\Common\NoJavaScriptAnonymousTest have disappeared, thanks to #2407195: Move attachment processing to services and per-type response subclasses.

And the 2 failures in Drupal\system\Tests\System\FrontPageTest have disappeared as well, thanks to #2480811: Cache incoming path processing and route matching.

Getting ever closer to zero :)

Fixed the failure in UpcastingTest, it was a bug in RouteCacheContext (using raw parameters, hence not the upcasted parameters, hence not the upcasted, i.e. translated entities). Fixing it here, because here we have a clear failure showing the bug.

This should bring us down to 32.

Oh, wow! I definitely don't like this. It should've been 32, not 15.

This implicitly "fixed" 15 more failures, but it didn't really fix them, they only look fixed thanks to a bug. #133 changed the route cache context to use the non-raw (i.e. upcasted) parameters. But something is modifying the upcasted parameters (node entities) after parameter upcasting. Hence the route cache context has different values just after routing (when SmartCache's event subscriber runs) and at render time (when SmartCache does its caching). Hence there's a mismatch in these cases, and hence there are always SmartCache misses (in case of node route parameters). That causes these tests to no longer fail.
This points to a much deeper problem with our ParamConverters. Discussed with catch, he opened #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity for that.

So, reverting #133, despite it being a correct fix. That fix will now be introduced, along with removing the failing test method of UpcastingTest (because it tests something that is wrong), in #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity. We found a bug related to that: #2512712: Entities rendered by EntityViewBuilder should vary by content language cache context, which can already be fixed independently.

Re-uploading #130, to not have a misleadingly low number of failures. Should be back to 33.

Given that we're at 33 failures, and have patches in the works to bring it below 20, I investigated what it'd take to get us to zero, and to check that there are no more further criticals hiding here.

Of those, #2512580: NodeEntityViewModeAlterTest uses State to dynamically affect hook_entity_view_mode_alter(), #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity, #2512734: session_test routes/controllers don't specify the appropriate cacheability metadata and #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified are new issues. (And I think #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified may be another security critical.)

(Also added to IS, where we can keep it updated.)

Note that #2512580 and #2512734 have very simple patches. Let's land those ASAP, then we'd be down to 25 fails :)

#2512580: NodeEntityViewModeAlterTest uses State to dynamically affect hook_entity_view_mode_alter() also landed.

Per #143, the one remaining failure in BlockTest should be fixed here.

#2512734: session_test routes/controllers don't specify the appropriate cacheability metadata also landed.

Re-uploading the #140 patch to get the new number of test failures, which should be 22 :)

Per #62 + #79 + #80 + #82: to do SmartCache, we must also do #2493033: Make 'user.permissions' a required cache context to provide sufficient security. So this issue is also blocked on that, but it isn't responsible for test failures, which is why it wasn't listed before. Updated the IS.

(Thanks @plach for pointing that out!)

Reroll of #145, with the minimal changes necessary to keep things working (#2450993: Rendered Cache Metadata created during the main controller request gets lost and #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified require changes in SmartCache).

• #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity landed, but the test should still fail, because UpcastingTest fails due to \Drupal\paramconverter_test\TestControllers::testEntityLanguage() being plain wrong/silly. One-line fix for that test is now included in this patch.
• #2500013: Add cacheability metadata information to translation overview is RTBC.
• #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching. is blocked on manual testing (which I will do later today).
• #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified landed, and was blocking #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching..

That only leaves #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user, which we still need to get started on.

This should bring it down to 21 failures, assuming #148 still has 22 failures, like #145.

#149 has one less failure than #148 as expected. The fact that it's 11/10 failures and not 22/21, is solely because we have a new test runner as of yesterday, which reports things differently.

The 5 trivial failures in SmartCacheIntegrationTest are due to #2505989-58: Controllers render caching at the top level and setting a custom page title lose the title on render cache hits. That is being fixed in #2506581: Remove SafeMarkup::set() from Renderer::doRender, so those 5 failures should disappear again :)

And #2500013: Add cacheability metadata information to translation overview has also landed in the mean time! (This is why Berdir re-tested it in #153.)

Updating issue accordingly :) Keeping at PP-4 because of #156.

#2493033: Make 'user.permissions' a required cache context already was RTBC (and has been for quite a while now), #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching. is now RTBC (EDIT: while writing this, it actually got committed! So reducing from PP-4 to PP3.), #2506581: Remove SafeMarkup::set() from Renderer::doRender is getting close to RTBC.

That leaves just #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user, which I've now rerolled and is now blocked on reviews!

This is a rebase of #149, with also #143 / #144 addressed.

#159: Indeed, technically we need to fix it, but it doesn't cause any test failures (because Forum's test coverage is lacking). That's indeed why it's omitted from the IS.

Thanks to #2407195: Move attachment processing to services and per-type response subclasses, we can slightly simplify SmartCache's logic.