Proposed commit message:

Issue #2429617 by Wim Leers, Fabianx, Berdir, yched, dawehner, effulgentsia, catch, borisson_, jhodgdon, martin107, torgosPizza: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)

TL;DR

Dynamic Page Cache (formerly known as SmartCache, until comment ~380) in a nutshell:

  • cache the CacheableResponseInterface object (which in case of a HtmlResponse still has the #attached[placeholders; those are replaced at a later time, by HtmlResponseAttachmentsProcessor)
  • return that cached response immediately after routing has taken place, hence avoiding the controller service (and its dependencies) being initialized, along with everything else for building the response.

The resulting performance improvement as user 1 on the frontpage (APCu off, OpCache on, PHP 5.5, XDebug off, XHProf on):

Run #frontpage-before Run #frontpage-after Diff Diff%
Number of Function Calls 31,794 16,749 -15,045 -47.3%
Incl. Wall Time (microsec) 89,291 57,922 -31,369 -35.1%
Incl. MemUse (bytes) 17,050,176 13,120,368 -3,929,808 -23.0%
Incl. PeakMemUse (bytes) 17,101,080 13,153,056 -3,948,024 -23.1%

(See #205 for details.)

Problem/Motivation

  1. We want D8's authenticated user page loads to be fast.
  2. Some parts of the page are cacheable across users. To actually cache that across users, we have #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).
  3. Other parts need to be dynamically calculated per user, or are simply uncacheable.
  4. Those dynamic parts should not prevent us from showing the rest of the page first.

(Drupal 8's anonymous user page loads already are fast since #606840: Enable internal page cache by default.)

We're render caching bits and pieces (blocks & entities), but we're still running expensive controllers to build the main content array. And we're initializing dozens and dozens of services that we barely use. It's getting more difficult to see what to optimize next.

Drupal has historically always generated the majority of the response dynamically for every request.

But we've been doing massive amounts of work to make things more cacheable. Could we make use that work to break the trend, and make Drupal 8 the first release to generate a minority of the response dynamically for every request?

Proposed resolution: the simplified version

Assumption: everything that depends on some (request) context, specifies a cache context. #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance has made sure that this is implemented consistently (and tested) for the 99% use cases.

On a cache miss
  1. A KernelEvents::RESPONSE event subscriber detects whether it's a CacheableResponseInterface object, i.e. a response with cacheability metadata that SmartCache can therefore safely cache. This an object representing the entire response. In case of a HtmlResponse, it still has the placeholders (that need to be replaced on every request, i.e. dynamically, uncacheable).
  2. The result is that we therefore have the response that is cacheable across all users, because we know which cache contexts are associated. We use the RenderCache class that is capable of handling cache redirects (which is in fact based on early versions of the SmartCache patch), and ask it to cache the response per route, and if necessary, perform cache redirection.
  3. The event subscriber ends. Then the response is finished and sent as usual. (The other event subscribers fire, including in case of a HtmlResponse the HtmlResponseSubscriber, which calls HtmlResponseAttachmentsProcessor, which does all the final rendering (just like for any other request): it renders the attached placeholders, bubbles the bubbleable metadata from the placeholders, and given that final set of attachments, it is able to render the CSS/JS assets, plus HTML <head> tags + HTTP headers.)
On a cache hit
  1. We've already done the above, and now we're hitting the same route again.
  2. Immediately after routing, before even calling the controller (the thing that would otherwise do DB queries and whatnot to build a render array), SmartCache's event subscriber checks if we have a cache item in the smart_cache cache bin for the current route, following any redirects if necessary.
  3. If such a cache item (including potential cache redirect) exists, then the response for this route is already cached. Hence the controller never needs to be executed, and all that still needs to happen (in case of a response other than HtmlResponse, otherwise we're done already), is processing the attachments! (Which includes rendering the placeholders.) This is handled automatically; just like for any HtmlResponse.

How is this related to the BigPipe issue?

The BigPipe issue: #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.

Basically, SmartCache doesn't care about things that are dynamic (max-age = 0 or cache contexts that we consider "too dynamic to cache", such as "per user"). It's very simple, dumb and stupid. It simply varies by all cache contexts on the page.

So, if e.g.:

  • /foo varies only by interface language, then SmartCache's entry for that will also vary by interface language
  • /bar varies by interface language, route, permissions, query parameter, user and whatnot, then the SmartCache entry will also vary by all those things

That's where the intersection with BigPipe begins.

BigPipe allows us to NOT have the bits that are "too dynamic" to affect the overall page: it allows us to NOT bubble up the "per user" cache context, for example. Because we replace it with a placeholder, and render it separately.

Proposed resolution

  1. See the simplified proposed resolution above.
  2. Read the issue summary at #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance.

Given what you've just read in #2429287, you may now realize that once:

  1. Cache contexts are defined by everything, as they should be (i.e. once that meta is completed since #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance is 99% done, which it now is)
  2. Cache context bubbling is implemented (child of the meta: #2429257: Bubble cache contexts)
  3. All uncacheable things use placeholders (just like: node links, comment links, the comment form on entities, and more)

… then we can start to put all of that cache metadata to the originally intended powerful use: cache the entire HtmlResponse, varied by the contexts associated with that route!

(And once we have that, we can go even further! We could make it configurable which cache contexts (high-frequency ones, e.g. "per user") we don't actually want to vary by, which we could then automatically replace with placeholders. We could then run replace those placeholders either when rendering a response (like today), replace them using something like Facebook's BigPipe, or replace them using ESI. It would be configurable. We have an issue now for auto-placeholdering: #2499157: [meta] Auto-placeholdering.)

A final note: SmartCache works post-routing and caches per-route. You may wonder: why not pre-routing and caching per-URL? — see #219 for a detailed answer. But in short: because the SmartCache architecture is based on cache contexts, and several cache contexts are not available pre-routing/pre-bootstrap (user, user.permissions, user.*, route, and likely more). Which means SmartCache cannot work pre-routing.

SmartCache reuses the logic in RenderCache, at the cost of some (isolated!) ugliness because RenderCache only knows how to deal with render arrays. The benefit of this reuse is that we don't duplicate the logic, and can just depend on the existing comprehensive test coverage.

Algorithm, proofs & next steps — comments wanted!

This issue corresponds to step 1 in the Google Doc: https://docs.google.com/document/d/1Gw7ohBOUKu38t4kMbN9zj6cX-4-_2ZNXGotv...

(We will move this onto Drupal.org once it's 100% fleshed out.)

Remaining tasks

None!

When this gets committed, please also credit Fabianx.

User interface changes

None.

API changes

None. This is a pure addition; that doesn't change any APIs.

But, notable changes:

  • Forms are now marked as uncacheable (max-age = 0) by default.
  • The REQUEST event subscriber ContentControllerSubscriber::onRequestDeriveFormWrapper() has a different priority, because it was impossible to inject an event subscriber at the desired place (i.e. to inject a new event subscriber in between).

And a few small bugfixes, mostly small remnants of things that were fixed in blocking child issues, but don't really make sense to fix separately because of process overhead:

  • TestAccessBlock::blockAccess() specifies max-age=0 because it depends on state.
  • PageCacheTest invalidates the rendered cache tag rather than deleting everything in the render cache bin.
  • PathAliasTest invalidates the rendered cache tag because it is expecting path alias changes to show up immediately, despite there still being an open issue (#2480077: Using the URL alias UI to change aliases doesn't do necessary invalidations: path aliases don't have cache tags) to make path aliases handle cache invalidation correctly. So, this is a temporary fix, with a @todo added.
  • The ConfigCacheTag config save event subscriber was only invalidating the rendered cache tag for system.theme.global, it also needs to do it for system.theme.
  • TestControllers::testEntityLanguage()'s render array was failing to pass on cacheability metadata.

Credit

HUGE HUGE HUGE thanks to Fabianx, who's helped me enormously in verifying that this actually works! Without him, this issue would be much vaguer. Plus, once this is in, he has amazing ideas for further improvements, he even sees a way to make it work without executing a request at all — resulting in 10–20 ms response times! See the aforementioned Google Doc, steps 2 & 3, for details.

Files: 
CommentFileSizeAuthor
#416 dynamic_page_cache-2429617-416.patch50.55 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,739 pass(es). View

Comments

Fabianx’s picture

Issue summary: View changes
Fabianx’s picture

Just a quick note:

The idea that Wim implements at the page level can be applied to all sub levels pretty easily - once the bubbling is in:

https://gist.github.com/LionsAd/fef2568413569c13e952

is a quick PoC / pseudo-code for that.

Reason:

- Currently the bubbling leads to no longer receive cache hits (e.g. an entity_view() adds a 'user' cache context, then the block its part of has a cache ID of block:name:%user).

But when that block is next checked it will check for just block:%user.

The idea is now to store the final used cache contexts (the sum of it), at that location.

So block:name returns a render array that redirects to block:name:%user.

Yes, much more cache entries, but perfect granularity - similar to smart cache.

While it might not make sense to enable that for all renderables, it might make sense to complement the strategy of the smart cache working on the route level.

e.g. a block where an entity is displayed per user can be gotten from cache on other routes.

Wim Leers’s picture

FileSize
2.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch smartcache-2429617-poc0.patch. Unable to apply patch. See the log in the details link for more information. View
4.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch smartcache-2429617-poc1.patch. Unable to apply patch. See the log in the details link for more information. View
1.07 MB

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!

Wim Leers’s picture

Title: SmartCache: context-dependent page caching (for *all* users!) » Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Status: Active » Needs review
FileSize
1.04 MB
16.52 KB
62.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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%
dawehner’s picture

Not bad results.

+++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
@@ -0,0 +1,134 @@
+        // Unfortunately, we cannot call $event->setResponse() with our render
+        // array, because it only accepts Response objects. For unknown reasons,
+        // Symfony only allows Responses to be returned during the REQUEST event
+        // but allows any data to be returned during the CONTROLLER event. (i.e.
+        // what we want is to return our data — a render array — and let Symfony
+        // handle that further using its VIEW event.)
+        // This sadly leaves us with only one option: mimic the VIEW event
+        // directly.
+        // The only alternative is to let run this at the CONTROLLER event,
+        // at which point the controller service will already have been
+        // initialized (and all its dependencies), and then override the already
+        // loaded controller with a closure like function () { return $html; }.
+        $view_event = new GetResponseForControllerResultEvent($event->getKernel(), $event->getRequest(), $event->getRequestType(), $html);
+        $this->mainContentViewSubscriber->onViewRenderArray($view_event);
+        $event->setResponse($view_event->getResponse());
+        $event->stopPropagation();

Just an idea, you can at any moment replace _controller with a new one, which does, what you want

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.24 KB
60.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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

Fabianx’s picture

Here are my initial thoughts:

  1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -91,6 +92,12 @@ public function __construct(TitleResolverInterface $title_resolver, PluginManage
    +    // If the _controller result already is #type => html, we can skip
    +    // immediately to the final rendering (only html.html.twig).
    +    if (isset($main_content['#type']) && $main_content['#type'] === 'html') {
    

    I think we should explicitly used an internal key, like #cache_rendered_already or something like that.

    Checking for html could have side effects.

  2. +++ b/core/lib/Drupal/Core/Render/MainContent/SmartCacheHtmlRenderer.php
    @@ -0,0 +1,139 @@
    +    // Smart cache caches per route.
    +    $cid_part_route_match = $this->routeMatch->getRouteName() . serialize($this->routeMatch->getRawParameters()->all());
    +
    

    While this is workable, I prefer to use a hash e.g.:

    $this->routeMatch->getRouteName() . ':' . hash256(serialize($this->routeMatch->getRawParameters()->all()));

  3. +++ b/core/lib/Drupal/Core/Render/MainContent/SmartCacheHtmlRenderer.php
    @@ -0,0 +1,139 @@
    +    // "Soft-render" the HTML regions (don't execute #post_render_cache yet,
    +    // since we must cache the placeholders, not the replaced placeholders).
    +    foreach (Element::children($cacheable_html) as $child) {
    +      $this->renderer->render($cacheable_html[$child]);
    +    }
    

    All this work needed strengthens my belief that this belongs in the Renderer instead and this should just set a #cache flag.

  4. +++ b/core/lib/Drupal/Core/Render/MainContent/SmartCacheHtmlRenderer.php
    @@ -0,0 +1,139 @@
    +    // If the set of cache contexts is different, store the union of the already
    +    // stored cache contexts and the contexts for this request.
    +    if ($cache_contexts !== $stored_cache_contexts) {
    +      if (is_array($stored_cache_contexts)) {
    +        $cache_contexts = array_merge($cache_contexts, $stored_cache_contexts);
    +        sort($cache_contexts);
    +      }
    +      $this->smartContextsCache->set($contexts_cid, $cache_contexts);
    +    }
    

    I think all code dealing with the cache contexts and prefixes belongs into a helper class / helper service.

    The route itself could then be a cache_context.

    That would generalize the two cache tier approach.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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.

Wim Leers’s picture

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.

martin107’s picture

Issue tags: +Needs reroll
epari.siva’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
14.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 54,177 pass(es), 21,867 fail(s), and 7,838 exception(s). View

Patch rerolled.

martin107’s picture

This is a comment that apples to #10 - before the reroll

SmartCacheHtmlRenderer::__construct()

There are two parameter variables name - which are too similar and for me cause confusion

The difference between $contexts_cache and $cache_contexts is not immediately obvious!

Regarding the reroll - all the error have a common factor.
It looks like the order of the parameter in SmartCacheHtmlRenderer::__construct needs some attention.

epari.siva’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
14.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Thanks for pointing out. Changed the order of parameters. Also, added the missing 6th parameter in parent::__construct()

Berdir’s picture

This issue is very experimental right now *and* it almost takes a testbot down when it runs, see the earlier broken test results. I'd recommend to postpone it and avoid rerolling it for now.

epari.siva’s picture

Sorry, that the reroll caused trouble. Agree, that rerolling should be avoided when it is obvious that the tesbot takes 3 hours for testing a patch at this stage of this issue.

Fabianx’s picture

Title: Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-1] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Status: Needs work » Postponed

Postponing on the meta issue for now per #18

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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.

Wim Leers’s picture

Issue summary: View changes
Status: Postponed » Active

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

Status: Active » Needs review
FileSize
15.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
3.99 KB

This is #23.1.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
16.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
7.1 KB

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

Wim Leers’s picture

Status: Needs review » Postponed

This is #23.3.


#23.4 is for tomorrow.

yched’s picture

Sorry, I'm aware it's probably not the right time for a pedantic review at this point, but I got curious, and then I got remarks, so I wrote them down :-)

  1. +++ b/core/core.services.yml
    @@ -855,8 +855,8 @@ services:
       main_content_renderer.html:
    -    class: Drupal\Core\Render\MainContent\HtmlRenderer
    -    arguments: ['@title_resolver', '@plugin.manager.display_variant', '@event_dispatcher', '@module_handler', '@renderer', '@cache_contexts']
    +    class: Drupal\Core\Render\MainContent\SmartCacheHtmlRenderer
    

    Just wondering : this means the "old" HtmlRenderer is never used anymore ? Do we still want to keep it around ?

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,121 @@
    +use Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent;
    

    unused :-)

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,121 @@
    +   * @param \Drupal\Core\Cache\CacheContexts $cache_contexts
    +   *   The cache contexts service.
    

    Feels a bit misleading/confusing that a $cache_contexts / CacheContext object is in fact a service that does stuff about contexts, but not "some actual cache contexts" in itself.

    onRouteMatch() just a couple lines below has a $cache_context var, which, this time actually holds "cache contexts for some piece of rendered html"

    Better if we name things for what they are ?

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,121 @@
    +    $cid_part_route_match = $this->routeMatch->getRouteName() . serialize($this->routeMatch->getRawParameters()->all());
    

    minor : no separator between the route name and the serialized array ?

  5. +++ b/core/lib/Drupal/Core/Render/MainContent/SmartCacheHtmlRenderer.php
    @@ -0,0 +1,153 @@
    +    // Smart cache caches per route.
    +    $cid_part_route_match = $this->routeMatch->getRouteName() . serialize($this->routeMatch->getRawParameters()->all());
    +
    +    // Get the contexts by which the current route's response must be varied.
    +    $contexts_cid = 'smartcache:contexts:' . $cid_part_route_match;
    +    $stored_cache_contexts = $this->smartContextsCache->get($contexts_cid);
    

    I might very well be getting this wrong, but if we have a match in SmartCacheSubscriber::onRouteMatch(), then we'll still run through main_content_renderer.html 's renderResponse(), right ?

    So aren't we re-doing here what SmartCacheSubscriber already did ?

    More generally, if SmartCacheHtmlRenderer::finish() runs for both cache hits and cache misses, then isn't some of the logic extraneous for cache hits (including re-caching the data) ?

Wim Leers’s picture

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

Wim Leers’s picture

Issue summary: View changes
FileSize
16.8 KB
7.14 KB

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
17.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,480 pass(es), 644 fail(s), and 77 exception(s). View
1.9 KB

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

Fabianx’s picture

Nice work!

Wim Leers’s picture

Status: Needs work » Postponed

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.

Wim Leers’s picture

FileSize
230.96 KB
1.8 MB

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.

effulgentsia’s picture

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.

From what I remember, there's similar overhead in 7.x, and a big source of it is the fetch and unserialization of the theme registry. The fetch could maybe be sped up by storing it in a bin that uses the chainedfast backend. But quite likely it's the unserialization that's most of the cost, so that might not help all that much. With the smartcache though, we ought to be calling way fewer theme hooks per request, so another possibility is to split up the registry into a cache item per theme hook (or more likely to be better, per base theme hook). This should probably all go into a separate issue, but I'm too lazy to file that at the moment.

dawehner’s picture

From what I remember, there's similar overhead in 7.x, and a big source of it is the fetch and unserialization of the theme registry.

I'm not 100% sure, but last time I had a look at that code initTheme and getActiveTheme have been separated from the registry entirely. They are only to negotiate the active theme, which is probably quite costly, if you have multiple possible ones.

mikeytown2’s picture

In terms of template_preprocess_html() calling AssetResolver::getCssAssets() and AssetResolver::getJsAssets() causing the slowdown due to rendering the scripts; could we use a render cache here? That's exactly what I do in AdvAgg, see _advagg_process_html().

Couple of tricky things to look out for if you do decide to use a render cache for css/js:
- Use getMultiple to get both css & js at the same time to speed things up.
- Drupal.settings will need to be post processed so the current pages Drupal.settings will be replaced with the cached version. Also means the Drupal.settings will need to be excluded from the hash used for the cache. If you don't do this you're cache hit ratio will be very bad on a semi complicated site.
- Caching the alters can cut the time in half on a simple site and even more so on a complicated one. Digging into what the alter does and using that as apart of the hash is key. Make sure 3rd party modules can add code in to allow the altering of the hash. This is sorta tricky as if you do it wrong this is a wash. Looking at system_js_settings_alter() and at this point I would avoid caching the calls to the alter functions; too much going on to easily cache alter calls.
- If using js to render the css (https://github.com/filamentgroup/loadCSS) then if you get a hit on the css but a miss on the js or the other way around you'll need to render both css & js. Also means you'll need to include the setting that turns it on/off in the hash if using functionality like this.

Fabianx’s picture

#39: Excellent point!

We removed all runtime library altering, so we can cache all except settings, which we get on runtime via hook_js_settings_alter().

And yes, indeed caching that result is easily possible in the same smartcache bin as long as we synchronize it with the files on the system. (e.g. store both the filenames and the rendered data, then re-verify that the cache is current).

/me excited!

Go Wim!

moshe weitzman’s picture

#36 - I don't see any flame graphs in the .zip. Would love to see. Last time I did flame graphs I exported to PNG somehow and embedded in the issue so folks could get a taste. I also hyperlinked that to the SVG hosted on dropbox. Send me SVG and I will do it for you.

Wim Leers’s picture

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

Wim Leers’s picture

FileSize
18.3 KB

Chasing HEAD.

Wim Leers’s picture

FileSize
17.59 KB

Some rebase cruft snuck into #43.

xjm’s picture

Issue tags: +D8 Accelerate Dev Days
MiSc’s picture

Tested it out, and it seems to work very well, great job on this one. My profiler is not working for the moment, so I could not do proper testing.

MiSc’s picture

Noticed one thing - I guess it is by design - but before the patch I got the cache-tag config:user.role.anonymous, after the patch I dont get it in the header any more.

Wim Leers’s picture

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

Wim Leers’s picture

FileSize
17.61 KB

Just chasing HEAD.

Wim Leers’s picture

FileSize
17.76 KB
8.95 KB

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

Wim Leers’s picture

Issue summary: View changes

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

Wim Leers’s picture

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

Wim Leers’s picture

Issue summary: View changes

Forgot a few bits.

Wim Leers’s picture

Issue summary: View changes
FileSize
18 KB
4.74 KB

Chasing HEAD.

Wim Leers’s picture

Issue summary: View changes
Status: Postponed » Needs review
FileSize
18.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,048 pass(es), 199 fail(s), and 17 exception(s). View
834 bytes

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:

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
18.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,162 pass(es), 124 fail(s), and 17 exception(s). View
1.03 KB

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

Wim Leers’s picture

Title: [PP-1] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-3] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes
Status: Needs work » Active

Nice, it fixed 75 failures :)

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

Wim Leers’s picture

Title: [PP-3] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-5] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

Clearer "remaining tasks" section.

Wim Leers’s picture

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

Fabianx’s picture

#62:

In three ways we are then even more secure than Drupal 7:

a) Even if caches are not enabled by default, the first thing that most users do when a site is slow is to enable some kind of cache (views, panels,...). That means that the potential information disclosure happens on live - instead of during development or on a well-tested staging site.

b) By being able to enforce a cache context for everything on the page, we can partition everything by user permissions hash - but someone with the knowledge and 60 roles can still easily opt-out (after doing a security audit). That was not as generically possible in Drupal 7. (e.g. the filter cache would never take into account the user.roles / permissions; any custom filter that did not know there was a transparent never seen cache and that forgot to opt-out of caching, could have introduced a security issue there easily.)

c) In Drupal 8 you can set max-age=0 and you are "done" from a security standpoint [need to check in how this might affect forms], while in Drupal 7 you could opt-out of page caching (drupal_page_set_cacheable(FALSE)), but you would need to check your whole stack that not at some level somewhere you were using any caches that were not respecting that and because that is set by session for any authenticated user very early on, no one checks it in practice ... Being able to say, because I have this little critical information here that I never want to cache, but always re-create dynamically / or make the whole page uncacheable by bubbling that up is a HUGE advantage.

Fabianx’s picture

Btw. I would be happy that controllers need to opt-in to being cached by smart-cache [In the discussion I missed that the main concern have been custom controllers], where entity and other secure core controllers would opt-in automatically.

Could be as simple as allowing controllers to use CacheableDependencyInterface optionally and maybe providing a AlwaysCacheableTrait?

Also anyone who sends their own (non-cacheable) Response will / should be automatically opted out of all caching. (2nd step for CacheableResponseInterface)

That should also again reduce the test-failures significantly of smart-cache. Could even have a run-time / container setting for that (so that we can still fix uncacheable parts).

I believe custom controllers are the 10%, not the 90% case.

mikeytown2’s picture

What if we had some sort of access callback for complex cases (or whatever is the D8 equivalent)? I do agree with Fabianx that this is a lot better than what we have in D7.

hass’s picture

I need to ask and notify you about an I think complex example I need a solution for and if I set max-age = 0 and this bubbles to the page and makes all uncached - than I guess you may hate me for doing this as Google Analytics is installed on most Drupal installs.

Here is what we need to cover:

  • JS code may change per user e.g. userID is unique
  • JS code may change on every page as someone may add a dimension / metric that is based on a dynamic token.
  • JS code will change on every search result page as the page URL is dynamic.
  • JS code may need to be shown per user on some pages, but others not.
  • JS code need to change if Drupals page status code may be 403 or 404 ("file not found" / "access denied" tracking).
  • JS code need to be removed if a user unsubscribe from getting tracked (per user setting).
  • JS code need to be added to a page or not if path filters are active e.g. /admin
  • JS code may change on several
  • JS visibility on a page may be fully dynamic with PHP code snippet.
  • JS visibility on a page may be based on DNT http header a client may send.
  • JS visibility may be based on role membership
  • Plus many other dynamic parameters that just change the JS code

I hope you can give me an idea how I can handle this as I feal a bit lost currently.

Fabianx’s picture

Thanks for your concerns, hass. This is exactly why we are creating such frameworks as cache contexts for 'user' and 'url'.

It would be great if you could watch https://events.drupal.org/losangeles2015/sessions/making-drupal-fly-fast... however first as there we do explain a lot of the basics.

I also do give an example of a block that has kinda the dynamicness of what you describe above.

Another requirement is for the GA code to be in the html head right so that it is executed as soon as the page starts to load? (Just asking, I am not sure.)

I know it might be frustrating that APIs are not yet finished for the caching especially if you have such a highly dynamic module like GA or recaptcha, but I assure you we are working hard on it and then it will all be simpler and make much more sense. But that dynamicness is exactly the reason why we don't support inline JS anymore and yes - of course we need to support use cases like that.

I pledge to personally help your modules to be ready so that they are ready when D8 comes out - however I have to beg your pardon it might take a while as we are in the process of finishing the last big problems.

-----

edit: some examples:

- JS code may change per user e.g. userID is unique

=> 'user' cache context

- JS code may change on every page as someone may add a dimension / metric that is based on a dynamic token.

=> custom cache context or if its based on query parameters 'query_args:argument'

- JS code will change on every search result page as the page URL is dynamic.

=> 'url' cache context

- JS code may need to be shown per user on some pages, but others not.

=> 'user' cache context OR custom cache context to know what the per 'user' or not deviates on

- JS code need to change if Drupals page status code may be 403 or 404 ("file not found" / "access denied" tracking).

=> Re-use the request.access_result cacheability meta data, likely that is already correctly cached though, so nothing needed to be added

- JS code need to be removed if a user unsubscribe from getting tracked (per user setting).

=> Per 'user' cache context

- JS code need to be added to a page or not if path filters are active e.g. /admin

=> Per 'url' cache context

- JS code may change on several

=> custom cache context

- JS visibility on a page may be fully dynamic with PHP code snippet.

=> PHP snippet will need to define cacheability metadata or be treated as uncacheable

- JS visibility on a page may be based on DNT http header a client may send.

=> "headers:[some header]" cache context

- JS visibility may be based on role membership

=> 'user.roles' cache context

- Plus many other dynamic parameters that just change the JS code

=> Whatever the dynamicness varies by, needs to defined as a cache context

znerol’s picture

My gut feeling is that the GA module should use JS settings for the variable parts of the code (like user-id) such that the tracker code itself is cacheable without tons of cache contexts (especially without the user context).

Fabianx’s picture

#68: Yes, however even for the JS settings you should declare cache contexts, so that a site could potentially fetch those settings via ajax (for e.g. per role cached pages) if it does not need GA to run already in the head.

giorgio79’s picture

#66

You could also handle some of the logic via pure js, pregenerating the logic once the user saves the GA settings...
Eg, js could easily handle urls,

if url contains this and that
then run this and that

instead of letting the db decide what to output per page...

hass’s picture

@Fabian: Thanks for the video link. BigPipe looks really like a killer feature.

Another requirement is for the GA code to be in the html head right so that it is executed as soon as the page starts to load? (Just asking, I am not sure.)

Yep. It need to run async, defer and as soon as possible from head. Loading anything later with ajax is too late.

But that dynamicness is exactly the reason why we don't support inline JS anymore and yes - of course we need to support use cases like that.

I still think it is a pure lie that we cannot support inline JS. Inline JS and CSS is a critical feature for mobile first. I noted this in the other case and never received any feedback from Wim who pointed earlier to this. But this is out of scope of this issue here.

We need to get GA up and running. A lot of people are asking for it far before core reaches RTM.

I have also been told be bedir that we need to change a lot of code for smartcache and that "headers:[DNT]" cache context will not work and we can only solve this with JS code that ask the browser for DNT status. You said it will work with the header context. I should give it a try what is really true now.

What is a custom cache context and how will this implemented?

Thanks for the examples. If I combine them all together I will end in a very large list of contexts.

- Plus many other dynamic parameters that just change the JS code
=> Whatever the dynamicness varies by, needs to defined as a cache context

Does this mean that every token need to define it's own cache level? This sounds like highly complicated coding in some cases. Ok a "user" token is user context, but how should I add this if I have no real clue what the users are entering. I leave this to token for now, but I'm sure a lot of things becomes extreme difficult to solve with tokens.

About recaptcha I found a solution by using attached and html_head, see http://cgit.drupalcode.org/recaptcha/tree/recaptcha.module?h=8.x-2.x#n98 . This works stable and since I added addCacheableDependency() all looks good for now. However I still hope to be able to move this into a libraries file just to make it cleaner.

Thanks fabian for your feedback and help and commitment.

hass’s picture

@znerol: DrupalSettings are also cached, aren't they? No idea how this could change anything if I need the userID. It is static for one user, but it is different for all your users :-)

hass’s picture

You could also handle some of the logic via pure js, pregenerating the logic once the user saves the GA settings...

Sure that is config context. That is really the easiest part.

Fabianx’s picture

#73: As GA really only needs the User ID in JS, we could store it in a cookie - readable by JS. GA could then do _one_ ajax request at the very first page load, to populate the cookie / user specific settings - if its not there already. We use this technique a lot.

Just to give some food for thought:

It might be that the client _never_ hits Drupal in the first place, because all pages (yes also authenticated user pages) are cached in e.g. Varnish or Akamai or another CDN. But GA should then still work ... [ granted usually someone logs in if they are authenticated, so just setting the cookie on the first uncached request usually works fine. ]

Wim Leers’s picture

Status: Active » Needs review
FileSize
18.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,491 pass(es), 96 fail(s), and 17 exception(s). View

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.

Wim Leers’s picture

FileSize
43.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,409 pass(es), 128 fail(s), and 20 exception(s). View
26.26 KB

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

Spinning off "Implemented #62 (made user.permissions another required cache context). This required a whole bunch of test changes." sounds like a good idea to me.

It also seems like a good idea to spin off a dedicated support request about using cache contexts. There are some good bits in this issue that are going to get totally lost because no one would ever think about looking at the issue that implemented SmartCache to find more information on how to use cache contexts et al correctly.

Fabianx’s picture

#79: Absolutely agree, lets spin off user.permissions to make that another required cache context to another issue.

#79: Yes, we really should move that thread over to there ... Is there any technical way to move things? I guess not so we should just paste a summary? Or maybe add the discussion to a wiki page?

Wim Leers’s picture

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.

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2493033: Make 'user.permissions' a required cache context
FileSize
29.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,562 pass(es), 98 fail(s), and 17 exception(s). View
14.06 KB

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

Wim Leers’s picture

Wim Leers’s picture

FileSize
30.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,573 pass(es), 83 fail(s), and 17 exception(s). View
1.01 KB

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

Removes another >=15 test failures.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
30.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,584 pass(es), 77 fail(s), and 17 exception(s). View
1 KB

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
31.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,590 pass(es), 68 fail(s), and 17 exception(s). View
959 bytes

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

Berdir’s picture

Still haven't reviewed this. I was quite scared by this, but with the limitations that were put in place for a first phase (like excluding all post forms and admin pages), it looks like this actually works, the tests are certainly starting to agree with that :)

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

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?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
36.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,593 pass(es), 58 fail(s), and 16 exception(s). View
10.57 KB

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

Wim Leers’s picture

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

Wim Leers’s picture

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

Sigh. Re-testing.

Wim Leers’s picture

Title: [PP-5] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

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.

Fabianx’s picture

#98: Lets add the easy fix to this issue:

- Use max-age=0 for any added conditions for now with a @todo on the other issue?

Fabianx’s picture

FileSize
2.17 KB

Attaching just a quick interdiff for #99.

Probably will need to use the same trick of InaccessibleBlock wrapping the real block for security reasons and BC ... (e.g. to not have page manager expose inaccessible blocks then) ...

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2495171: Block access results' cacheability metadata is not applied to the render arrays
FileSize
35.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,204 pass(es), 55 fail(s), and 16 exception(s). View

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

Wim Leers’s picture

Fabianx’s picture

Title: [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-7] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes
Fabianx’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

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

Wim Leers’s picture

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

Wim Leers’s picture

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

hass’s picture

Berdir’s picture

Berdir’s picture

Wim Leers’s picture

Issue summary: View changes

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

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
35.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,800 pass(es), 362 fail(s), and 127,593 exception(s). View

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
35.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,854 pass(es), 64 fail(s), and 9 exception(s). View
824 bytes

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

martin107’s picture

Status: Needs work » Needs review
FileSize
34.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,848 pass(es), 64 fail(s), and 9 exception(s). View
4.78 KB

Found only minor nits while looking this patch over.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
36.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,960 pass(es), 38 fail(s), and 8 exception(s). View
3.28 KB
+++ 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!


Both #2481453: Implement query parameter based content negotiation as alternative to extensions and #2273925: Ensure #markup is XSS escaped in Renderer::doRender() broke this in tricky ways. The former requires SmartCache's event subscriber to ignore any requests that specify a wrapper format (one-line change), the latter requires us to mark all of SmartCache's cached markup as safe.

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.

Wim Leers’s picture

Title: [PP-7] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)

Yay, down to 39 failures! :)

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

Wim Leers’s picture

Title: [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-5] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

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

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
37.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,356 pass(es), 33 fail(s), and 8 exception(s). View
12.77 KB
Wim Leers’s picture

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
38.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,379 pass(es), 15 fail(s), and 0 exception(s). View
762 bytes

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.

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity
FileSize
37.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,352 pass(es), 33 fail(s), and 8 exception(s). View

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.

Wim Leers’s picture

Title: [PP-5] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-8] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

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.

Test Fails Exceptions Issue
Drupal\block\Tests\BlockTest 4 0 #2495171: Block access results' cacheability metadata is not applied to the render arrays(critical, because security + potential API changes)
Drupal\comment\Tests\CommentTranslationUITest 5 4 #2500013: Add cacheability metadata information to translation overview
Drupal\content_translation\Tests\ContentTestTranslationUITest 5 4
Drupal\content_translation\Tests\ContentTranslationWorkflowsTest 4 0
Drupal\node\Tests\NodeEntityViewModeAlterTes 2 0 #2512580: NodeEntityViewModeAlterTest uses State to dynamically affect hook_entity_view_mode_alter()
Drupal\system\Tests\ParamConverter\UpcastingTes 1 0 #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity
Drupal\system\Tests\Session\SessionTest 6 0 #2512734: session_test routes/controllers don't specify the appropriate cacheability metadata
Drupal\toolbar\Tests\ToolbarAdminMenuTest 3 0 #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching. + #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified
Drupal\tracker\Tests\TrackerTest 3 0 #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user

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

Wim Leers’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
35.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,655 pass(es), 40 fail(s), and 32,927 exception(s). View

Reroll, the block test fails should be gone now.

Berdir’s picture

FileSize
37.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,364 pass(es), 28 fail(s), and 8 exception(s). View
2.93 KB

Ah, need to generate a proxy class

Berdir’s picture

Still one test fail in BlockTest, see #2375695-56: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed:

I'm also attaching a patch that combined this with the smartcache patch. This fixes 3 of the 4 test fails in BlockTest. The last one is a test-only scenario I think, since it's an access check that just depends on state. So I'm adding a cache tag invalidation there for the block, which I think is actually a useful test on it's own. With that, BlockTest is green. Let's see if it helps with other test fails as well.

I think that should just be added to this issue. Doesn't make sense to me to do that in a separate one.

Wim Leers’s picture

Title: [PP-8] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

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

Wim Leers’s picture

Title: [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-5] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes
Status: Needs work » Needs review
FileSize
37.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,381 pass(es), 22 fail(s), and 8 exception(s). View

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

Wim Leers’s picture

Title: [PP-5] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
39.52 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,181 pass(es), 21 fail(s), and 8 exception(s). View
4.73 KB
Wim Leers’s picture

Title: [PP-6] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-4] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes
FileSize
40.29 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,297 pass(es), 11 fail(s), and 0 exception(s). View
854 bytes

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.

Wim Leers’s picture

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

Berdir’s picture

5 new test fails in SmartCacheIntegrationTest but those are trivial to fix.

Wim Leers’s picture

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

Wim Leers’s picture

Issue summary: View changes

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.

Wim Leers’s picture

Title: [PP-4] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-3] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

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

serg2’s picture

#2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user is still listed as a child of this. I thought best to mention just in case you have overlooked it. It doesn't appear to be causing test failures which is what I think you are focussing on.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
41.17 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,288 pass(es), 10 fail(s), and 0 exception(s). View
976 bytes

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

Wim Leers’s picture

FileSize
40.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,290 pass(es), 10 fail(s), and 0 exception(s). View
1.26 KB

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

lauriii’s picture

Title: [PP-3] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-2] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Wim Leers’s picture

I investigated the unexpected Toolbar failures. (After #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching., we expected zero failures.)

I found the root cause, and I posted it as a comment on the relevant issue: #2512866-132: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified. Awaiting feedback there to figure out the best way to fix this.

Wim Leers’s picture

Title: [PP-2] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-3] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

For #165, we now have a new major issue, that also blocks SmartCache: #2533768: Add the user entity cache tag to user.* cache contexts that need it.

Wim Leers’s picture

Title: [PP-3] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-2] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes
Status: Needs work » Needs review
FileSize
40.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,622 pass(es), 15 fail(s), and 0 exception(s). View

#2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user just landed. Again one less blocker! :)

Straight reroll of #161, to track how many remaining failures we have.

Wim Leers’s picture

Issue summary: View changes

As @lauriii noted in #164, #2493033: Make 'user.permissions' a required cache context has landed too. I forgot to update the IS — done now.

I will start updating the IS in the next few days to get this issue + patch ready for the final steps.

Wim Leers’s picture

Title: [PP-2] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » [PP-1] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
Issue summary: View changes

And now #2506581: Remove SafeMarkup::set() from Renderer::doRender landed too! Down to one blocker :)

cosmicdreams’s picture

It appears that even though #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user landed the tracker related fails in this issue are not resolved. How should we follow up on this issue?

Berdir’s picture

Maybe it's an invalidation problem again? So not that it's different per user, but that something changes and the caches aren't invalidated by it.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
40.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,640 pass(es), 10 fail(s), and 0 exception(s). View
926 bytes

Indeed. Investigating.

First, here's a fix for the 5 remaining failures (1 on the new testbot) in SmartCacheIntegrationTest. Those are caused by the fact that the simplification in #161 causes no more 'page' key to be cached. I simply had not yet updated the test accordingly.

Wim Leers’s picture

FileSize
43.81 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,648 pass(es), 2 fail(s), and 0 exception(s). View
3.18 KB

Found the root cause why #167 (i.e. after #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user landed), we still have failures in TrackerTest (also raised by #173 + #174).

Given that that is going to be the only small bit that technically belongs in a child issue, I think it's fine to do that last bit as part of this patch.


Now rerolling #2533768: Add the user entity cache tag to user.* cache contexts that need it, which is the last child issue, and will fix the last 2 failures.

Wim Leers’s picture

Issue summary: View changes

IS updated.

Note that I'm working on #2499157: [meta] Auto-placeholdering, which would make this issue effectively cache the majority (i.e. minus the too-dynamic parts, which are automatically placeholdered) of most HTML pages (we don't cache /admin HTML pages for example)!
That, in turn, makes BigPipe (#2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts) significantly more effective, because it means BigPipe can just send the initial HTML directly, because SmartCache already has it cached. BigPipe simply chooses to stream the content for the placeholders after the majority of the page is already sent to the browser (i.e. the user doesn't have to wait for the highly dynamic content in the placeholders to be rendered), instead of first replacing all placeholders, and then sending a completely finished response (i.e. the user DOES have to wait for the highly dynamic content in the placeholders to be rendered).

Fabianx’s picture

  1. +++ b/core/modules/tracker/tracker.pages.inc
    @@ -123,8 +123,9 @@ function tracker_page($account = NULL) {
       $cache_tags = Cache::mergeTags($cache_tags, \Drupal::entityManager()->getDefinition('node')->getListCacheTags());
    +  $cache_tags = Cache::mergeTags($cache_tags, \Drupal::entityManager()->getDefinition('comment')->getListCacheTags());
    

    So tracker shows both nodes and comments?

    Makes sense ...

  2. +++ b/core/modules/tracker/tracker.pages.inc
    @@ -142,6 +143,7 @@ function tracker_page($account = NULL) {
       if (Drupal::moduleHandler()->moduleExists('history') && \Drupal::currentUser()->isAuthenticated()) {
         $page['#attached']['library'][] = 'tracker/history';
    +    $page['#cache']['contexts'][] = 'user.roles:authenticated';
       }
    

    uhm, the cache context should be added outside of the if - unless I am missing something.

Berdir’s picture

On top of that #2082315: Tracker history markers ("new" and "updated" markers, "x new replies" links) forces render caching to be per user didn't add the user.roles:authenticated cache context (for only adding the tracker/history asset library for authenticated users). Fixing that removes the remaining 2 of 8 failures. (Without that cache context, SmartCache thinks the same HTML is valid for both anonymous and authenticated users.)

Shouldn't #2493033: Make 'user.permissions' a required cache context essentially result in the same because the permissions is going always different between anon and authenticated?

Is it possible that smartcache doesn't respect the required cache contexts yet and only gets the implicitly if there's something being render cached on a page that bubbles up?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
44.04 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,645 pass(es), 2 fail(s), and 0 exception(s). View
944 bytes

#179:

  1. Indeed.
  2. OOPS! Too quick a reroll. Fixed.

#181: Well, I think it's theoretically possible that they have the same sets of permissions. So we should fix that bit in the Tracker module anyway. But, nevertheless, you raise an excellent point!

Is it possible that smartcache doesn't respect the required cache contexts yet and only gets the implicitly if there's something being render cached on a page that bubbles up?

Cha-ching! We've got a winner! Fixing that next.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
45.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,658 pass(es), 2 fail(s), and 0 exception(s). View
9.16 KB

Here it is, required cache contexts are now handled by SmartCache. Includes updated test coverage.

(I've manually tested it locally, and can confirm that with this fixed, we technically could remove the 'user.roles:authenticated' cache context that is being added in tracker.pages since #176, but as explained in #182, I think it's better to still specify it anyway. Especially now that it's cleaned up/documented well since #182.)

Fabianx’s picture

#184: Now that we handle required cache contexts ourselves and cache whatever we get as cacheable render array, we can probably use render cache service now and avoid the logic duplication, right?

( e.g. inject it, which would still work even if it was private. )

Wim Leers’s picture

Also, I just realized that since #2407195: Move attachment processing to services and per-type response subclasses, SmartCache can be simplified even further, and to do even less work on cache hits. Instead of caching the #type => 'html' render array, we can now simply cache the entire HtmlResponse object AFAICT.

It's getting late here, but I'll look into that tomorrow. I very well may be forgetting something right now!

Wim Leers’s picture

#184: no, RenderCache is not aware of required cache contexts. See #2483887: Mark RenderCache as internal for that. So, for now, that's a simplification we cannot yet make.

Berdir’s picture

Sure, we should definitely keep explicit cache contexts.

That said, even if anon and authenticated would have the same permissions, our implementations ensures that the hash is still different because we also include the role names. And I think we could actually optimize user.roles* into user.permissions with #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents().

While that does not itself result in different variations as they will always be consistent with each other, it would likely result in fewer cache redirects, since we have the complete list of necessary cache contexts already available upfront. @catch made a similar point about user.permissions being a required context already in the issue that made it so.

Fabianx’s picture

#187 Yes, that is what I meant.

Because we now handle required cache contexts in smartcache ourselves, we can use the RenderCache service to load / store the data instead of duplicating the logic from there.

Wim Leers’s picture

#188:

That said, even if anon and authenticated would have the same permissions, our implementations ensures that the hash is still different because we also include the role names.

True. Since #2417895: AccountPermissionsCacheContext/PermissionsHashGenerator must special case user 1, since permissions don't apply to it, not before, but true.

And I think we could actually optimize user.roles* into user.permissions with #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents().

Hah, great thinking! (But again, only since #2417895.) Can you comment on #2453835 so we don't forget it there? @Berdir++
EDIT: Berdir already did: #2453835-12: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() :)

While that does not itself result in different variations as they will always be consistent with each other, it would likely result in fewer cache redirects

Indeed :)

Wim Leers’s picture

Before embarking on the idea I had in #186 to make SmartCache even faster, I figured I'd do some profiling so we have an idea about the impact of the SmartCache patch after all these months. Is it still worth it? As you'll see below, the answer is Yes.


I did a round of profiling using the patch in #184, with D8 HEAD (commit cc34748c31e393e25f595f472c1a40bedd9acf12). The sole @todo is uncommented — again: that won't be necessary anymore once #2499157: [meta] Auto-placeholdering lands. But in HEAD, e.g. the breadcrumb block is present on all pages, and has max-age = 0, hence every page is made uncacheable by it. Auto-placeholdering will fix that. But for now, uncommenting that @todo fixes it, so we can have meaningful profiling results.

The last time was in #36. But in #36, I only profiled the frontpage, to compare @yched's suggestions for improvements with the original patch, that was profiled in #4.

This round of profiling tests more scenarios than before. And I also did benchmarking.

So, without further ado…

Profiling

Frontpage, with one node created
Run #frontpage-before Run #frontpage-after Diff Diff%
Number of Function Calls 37,273 22,573 -14,700 -39.4%
Incl. Wall Time (microsec) 95,100 65,190 -29,910 -31.5%
Incl. MemUse (bytes) 17,083,448 13,332,912 -3,750,536 -22.0%
Incl. PeakMemUse (bytes) 17,336,144 13,550,016 -3,786,128 -21.8%
/node/1
Run #node1-before Run #node1-after Diff Diff%
Number of Function Calls 62,491 43,918 -18,573 -29.7%
Incl. Wall Time (microsec) 146,704 103,273 -43,431 -29.6%
Incl. MemUse (bytes) 21,952,296 17,874,752 -4,077,544 -18.6%
Incl. PeakMemUse (bytes) 22,312,336 18,214,280 -4,098,056 -18.4%
/contact
Run #contact-before Run #contact-after Diff Diff%
Number of Function Calls 43,223 17,166 -26,057 -60.3%
Incl. Wall Time (microsec) 104,518 45,320 -59,198 -56.6%
Incl. MemUse (bytes) 17,811,576 8,984,952 -8,826,624 -49.6%
Incl. PeakMemUse (bytes) 18,179,096 9,145,000 -9,034,096 -49.7%

What's interesting here is that we see that neither /node/1 nor the frontpage show the 55-60% improvement anymore that we saw in earlier profiling. This is simply because we made Drupal 8's bottom line performance better. But, nevertheless, we still see 30-40% improvements for both of those.
Note that both will become faster once we no longer placeholder node and comment links no longer explicitly use placeholders, but just bubble cacheability metadata (I'd swear Berdir filed an issue for this, but having gone through his last 50 (!) pages of nodes he participated in, I did not find it). Because that is the main reason the /node/1 and frontpage routes are still relatively slow even with SmartCache: because they have quite a few placeholders to replace, and to render those placeholders, a whole bunch of services need to be initialized.

Contrast that with the results for the /contact route, which has only a single placeholder: that for rendering messages. It's sped up by 60% by this issue.

Profiling evolution

Nevertheless, if we take a look at the evolution of the profiling over the course of this issue, i.e. if we compare the frontpage's results in #4 and #36 with that in #184:

#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%
#184, with the @todo uncommented
Run #frontpage-before Run #frontpage-after Diff Diff%
Number of Function Calls 37,273 22,573 -14,700 -39.4%
Incl. Wall Time (microsec) 95,100 65,190 -29,910 -31.5%
Incl. MemUse (bytes) 17,083,448 13,332,912 -3,750,536 -22.0%
Incl. PeakMemUse (bytes) 17,336,144 13,550,016 -3,786,128 -21.8%

We went from 66K to 75K to 37K function calls in the before ("without patch") situation. I don't know the reason for the 66K to 75K increase in HEAD at the time, but it's likely a regression. I do know the reason for going from 75K to 37K function calls: Views are render cached now.
But note we also went from 26K to 28K to 22K function calls in the after ("with patch") situation. We went up from 26K to 28K because the original patch in #4 was yielding incorrect results. But we did go down from 28K to 22K function calls in recent time. In part thanks to the SmartCache implementation becoming better, but mostly thanks to bootstrapping/routing/route access checking becoming more efficient.

Benchmarking

Frontpage, with one node created

/node/1

/contact


Berdir’s picture

The issue you were looking for is #2530846: Fix and improve comment cache tag usage, I tested removing it there, but I'll open a separate issue just for that, so we can get the comment cache tags stuff done.

Wim Leers’s picture

Note that with #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, every HTML page will basically see the ~30 ms response time of /contact, and everything on top of that (the ~50 and ~70 ms response times for the frontpage and /node/1, so +20 and +40 ms, respectively) will happen after the initial page is sent, those contents would be streamed to the browser by BigPipe!

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
44.65 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,649 pass(es), 72 fail(s), and 15 exception(s). View
32.23 KB
7.64 KB

I now did what I described in #186:

Also, I just realized that since #2407195: Move attachment processing to services and per-type response subclasses, SmartCache can be simplified even further, and to do even less work on cache hits. Instead of caching the #type => 'html' render array, we can now simply cache the entire HtmlResponse object AFAICT.

I'm happy to report that this shaves off another 300 function calls per page load, but much more importantly: it makes the SmartCache implementation trivial!

No more careful overriding of the HtmlRenderer… just an event subscriber. Because this patch defers rendering of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor (to make it easier to review the changes in that area separately, I've attached smartcache_placeholder_replacement_in_htmlresponse-2429617-do-not-test.patch), SmartCache can simply be implemented as a KernelEvents::RESPONSE subscriber. Note that this change is not an API change (it doesn't affect anybody). On top of that, it also simplifies the implementation of #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, because BigPipe will want to render the placeholders into BigPipe placeholders and not actually render the placeholders yet; the actual rendering of the placeholders happens later in BigPipe, as it wants to stream the HTML for those placeholders (as also explained in #194).

In terms of performance, the difference will be negligible because it only saves 300 function calls (for the /contact example, we go from 17,166 function calls to 16,838, or 328 fewer function calls). So a new round of profiling is not useful. But what matters most, is that this new implementation is vastly simpler.


If everybody is on board with this, I'll update the IS to describe the new implementation.

Wim Leers’s picture

FileSize
44.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,662 pass(es), 62 fail(s), and 0 exception(s). View
2.31 KB

I saw a way to slightly simplify things further.

Also: I forgot to update a small bit in SmartCacheIntegrationTest, that will cause its tests to fail.

Wim Leers’s picture

Ugh, all of the above profiling was done with this in my settings.local.php:

$config['system.performance']['css']['preprocess'] = FALSE;
$config['system.performance']['js']['preprocess'] = FALSE;

Looks like that has significantly skewed the results. So, I'll have to redo it all. For example, /contact would then not take 16838 functionc alls as described in #195, but only 11,604 function calls and significantly fewer milliseconds.

Will do that next, after a very, very late lunch.

yched’s picture

I've fallen a bit behind the last couple weeks, but :

+++ b/core/core.services.yml
@@ -906,6 +915,35 @@ services:
+  smart_cache_subscriber:
+    class: Drupal\Core\EventSubscriber\SmartCacheSubscriber
+    arguments: ['@current_route_match', '@cache_contexts_manager', '@cache.smart_cache_contexts', '@cache.smart_cache_html', '@smart_cache_request_policy', '@smart_cache_response_policy', '%renderer.config%']

+++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
@@ -0,0 +1,241 @@
+  public function __construct(RouteMatchInterface $route_match, CacheContextsManager $cache_contexts_manager, CacheBackendInterface $contexts_cache, CacheBackendInterface $html_cache, RequestPolicyInterface $request_policy, ResponsePolicyInterface $response_policy, array $renderer_config) {
...
+    $this->requestPolicy = \Drupal::service('smart_cache_request_policy');
+    $this->responsePolicy = \Drupal::service('smart_cache_response_policy');

So are the policies injected or pulled ? ;-)

yched’s picture

Also, I might very well get that wrong, but : so far HtmlRenderer used to be in charge of rendering all the placeholders in the HTML, and now it's the job of HtmlResponseAttachmentsProcessor ? Doesn't this seem out of scope for a class that is primarily supposed to take care of attached assets ?

If BigPipe wants to swap a different processing for the placeholders, does it really want to swap the "process assets" logic as well ?

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,241 @@
    +    if (!$response instanceof HtmlResponse) {
    +      return;
    +    }
    

    I think we definitely need an HtmlResponseInterface now ...

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -128,6 +134,33 @@ public function processAttachments(AttachmentsInterface $response) {
    +  protected function renderPlaceholders(HtmlResponse $response) {
    

    I thought we had a HtmlResponseInterface?

    If not maybe we could use AttachmentsInterface as that should be all we need?

  3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -128,6 +134,33 @@ public function processAttachments(AttachmentsInterface $response) {
    +    $build = [
    +      '#markup' => SafeString::create($response->getContent()),
    +      '#attached' => $response->getAttachments(),
    +    ];
    +    $this->renderer->renderRoot($build);
    

    Overall this works for me, but it won't make BigPipe easier, but more difficult overall.

    The reason is 2-fold:

    a) BigPipe needs arbitrary data in #attached, but between renderPlaceholders() and drupal_process_attached() there is no way to hook in.

    So probably the best would be to lift the Exception from drupal_process_attached ...
    (I never liked it anyway).

    Or at least make the list configurable somehow via a container parameter or such.

    b) We need to clone the full response to use the BigPipeResponse class, but that is very error prone at this point as someone could have already replaced the response with a custom class.

    So likely a decorator with magic __call, while slow, is probably the only option left here and even then it is a little problematic as it could possibly not define the right interface.

    So it would be better to return a BigPipeResponse from the start from HtmlRenderer, but that is only possible if placeholders are replaced (or rather: render strategies run), because only then we know that we want to BigPipe or not.

    But I agree that for SmartCache this is way simpler.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
49.24 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,846 pass(es), 2 fail(s), and 0 exception(s). View
6.77 KB

This should bring us back to 2 failures.

Also fixed #200 — thanks @yched — that's a PoC-hack leftover that I failed to clean up :)

Replying to #201 and #202 in a next comment.

Wim Leers’s picture

Here's a new round of profiling, this time profiling #203. (Because, as I said in #197, the previous round of profiling had CSS & JS aggregation disabled, which skewed the results.)

Profiling

Frontpage, with one node created
Run #frontpage-before Run #frontpage-after Diff Diff%
Number of Function Calls 31,794 16,749 -15,045 -47.3%
Incl. Wall Time (microsec) 89,291 57,922 -31,369 -35.1%
Incl. MemUse (bytes) 17,050,176 13,120,368 -3,929,808 -23.0%
Incl. PeakMemUse (bytes) 17,101,080 13,153,056 -3,948,024 -23.1%
/node/1
Run #node1-before Run #node1-after Diff Diff%
Number of Function Calls 56,953 38,041 -18,912 -33.2%
Incl. Wall Time (microsec) 140,211 99,686 -40,525 -28.9%
Incl. MemUse (bytes) 21,884,320 17,616,736 -4,267,584 -19.5%
Incl. PeakMemUse (bytes) 21,983,560 17,695,024 -4,288,536 -19.5%
/contact
Run #contact-before Run #contact-after Diff Diff%
Number of Function Calls 37,994 11,605 -26,389 -69.5%
Incl. Wall Time (microsec) 101,501 39,037 -62,464 -61.5%
Incl. MemUse (bytes) 17,845,912 8,682,792 -9,163,120 -51.3%
Incl. PeakMemUse (bytes) 17,980,048 8,708,800 -9,271,248 -51.6%

Profiling evolution

#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%
#203, with the @todo uncommented
Run #frontpage-before Run #frontpage-after Diff Diff%
Number of Function Calls 31,794 16,749 -15,045 -47.3%
Incl. Wall Time (microsec) 89,291 57,922 -31,369 -35.1%
Incl. MemUse (bytes) 17,050,176 13,120,368 -3,929,808 -23.0%
Incl. PeakMemUse (bytes) 17,101,080 13,153,056 -3,948,024 -23.1%

Benchmarking

Frontpage, with one node created

/node/1

/contact


Wim Leers’s picture

So, an additional 10% gain, that was masked in the previous profiling.

EDIT: Moshe Weitzman asked why exactly. Well, CSS/JS aggregation being disabled means many more <link rel=stylesheet … and <script>… tags need to be generated. And those are generated using the Render system. And that is apparently fairly expensive. We can dig into why that is expensive, but that's out of scope here. What matters is that we're profiling the actual situation that people will be using Drupal in the most: with CSS/JS aggregation turned on. Those numbers look great. But even with CSS/JS aggregation turned off, those numbers look great. So: either way: yay :)

Wim Leers’s picture

#201: But as of #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache, placeholders are attachments. Attachments != only assets, it's also things like HTML in <head>, HTTP headers… and … placeholders.
BigPipe will not want to swap (override) asset handling, but they also won't have to. See below.

#202

  1. Why?
  2. We don't have that interface (see point 1).
    We could use AttachmentsInterface, but I think it's semantically wrong. This is only for HTML responses.

    Given that Symfony only has Response and not ResponseInterface. Similarly, we can just have HtmlResponse, and BigPipe could provide BigPipeResponse extends HtmlResponse.

    Introducing HtmlResponseInterface can be done in a separate issue.

  3. Hm…
    a) Right, but that's just stupidity/brokenness/legacy cruft/clean-up we never got to in drupal_process_attached() that needs to be fixed anyway.
    b) That's how the current BigPipe patch works. But I think it could just as easily be changed so that HtmlRenderer always returns a HtmlResponse object. That also makes sense conceptually, since it's called the HTML renderer. The current BigPipe patch (#2469431-81: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts) is kind of hacky in that respect: it requires changes to HtmlRenderer so that it is able to return a BigPipeResponse. That's not really acceptable IMO, because it means contrib cannot add more render strategies. So I think render strategies need to be layered on top of (i.e. run after) HtmlRenderer.
    Given that, I think this is a very simple, elegant and complete solution (interdiff relative to latest patch):
    diff --git a/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    index ab8db4d..16c17af 100644
    --- a/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -103,7 +103,18 @@ public function processAttachments(AttachmentsInterface $response) {
         // attachments to be added to the response, which the attachment
         // placeholders rendered by renderHtmlResponseAttachmentPlaceholders() will
         // need to include.
    -    $this->renderPlaceholders($response);
    +    // This takes HtmlResponse and returns:
    +    // - in case of the SingleFlush render strategy: it returns the same
    +    //   HtmlResponse object, but with #attached[placeholders] rendered and
    +    //   replaced in the Response's content
    +    // - in case of the BigPipe render strategy: it returns a BigPipeResponse
    +    //   object, which contains exactly the same data as the HtmlResponse object
    +    //   but with the placeholders in #attached[placeholders] replaced with
    +    //   BigPipe's own placeholders (if it needs that) plus
    +    //   #attached[bigpipe_placeholders], which are handled by
    +    //   BigPipe::sendContent(), like the current BigPipe patch does at
    +    //   #2469431-81: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.
    +    $response = $this->renderStrategyManager->replacePlaceholders($response);
     
         $attached = $response->getAttachments();
    
andypost’s picture

yched’s picture

as of #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache, placeholders are attachments

Doh, right. I even yayed about it in that very issue back then.

Sorry for the noise - awesome simplifications in the latest rounds !

[edit : and the proposed code at the end of #207 makes a ton of sense]

beejeebus’s picture

another drive by, just reading the code to see if any of it makes sense to someone way out of the loop.

+    $this->routeMatch->getRouteName();

typo, or some side-effect-causing-thing that should have a comment?

+    $cache_contexts = $this->smartContextsCache->get($this->cacheContextsManager->convertTokensToKeys(['route'])->getKeys()[0]);

OMG my eyes. maybe not for this issue, but at least one object is missing an API method.

+    // If we already know the contexts by which the current route's response
+    // must be varied, check if a response already is cached for the current
+    // request's values for those contexts, and if so, return early.

i think that comment can go away. it doesn't say anything interesting beyond what we see in the code.

+    // "Soft-render" the HTML regions: don't replace placeholders yet, because
+    // that happens in \Drupal\Core\Render\HtmlResponseAttachmentsProcessor.
+    $render_context = new RenderContext();
+    $this->renderer->executeInRenderContext($render_context, function() use (&$html) {
+      $this->renderer->render($html);
+    });
+    if (!$render_context->isEmpty()) {
+      $bubbleable_metadata = $render_context->pop();
+      BubbleableMetadata::createFromRenderArray($html)
+        ->merge($bubbleable_metadata)
+        ->applyTo($html);
+    }
     $content = $this->renderCache->getCacheableRenderArray($html);
 
+    // Also associate the required cache contexts. Since we only "soft-render"
+    // the HTML above, and don't use a ::renderRoot() call which would add the
+    // required cache contexts automatically, we have to make sure manually that
+    // the HTML response varies by the required cache contexts.
+    $content['#cache']['contexts'] = Cache::mergeContexts($content['#cache']['contexts'], $this->rendererConfig['required_cache_contexts']);

this block seems like it could use some work. i don't think "soft-render" as a concept adds anything. i think it means "don't replace placeholders", which is already stated. also, i guess this is an early / late thing? or something? there's kind of a missing bit of 'why' here. here's a possibly wrong attempt:

// Don't replace placeholders yet, because that happens later in the render pipeline. Instead, make sure to
// provide the cache contexts so that later rendering stages can vary the output appropriately.
// @see \Drupal\Core\Render\HtmlResponseAttachmentsProcessor
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
49.03 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,817 pass(es), 2 fail(s), and 0 exception(s). View
3.65 KB

#210

  • RE: typo/side-effect: just dead code from an earlier iteration, thanks!
  • RE: OMG my eyes: Agreed on confusingness. I think this is better.
  • RE: pointless comment: Agreed; rewrote the comment to be more useful. If you feel it still can be removed, I will do so.
  • RE: soft-rendering comment: Agreed; there was also some leftover cruft in that initial comment that didn't make sense anymore, which obviously did not help.

I'm hopeful that if those are your only remarks, that the patch was mostly clear? :)

Wim Leers’s picture

#189 (@Fabianx):

Because we now handle required cache contexts in smartcache ourselves, we can use the RenderCache service to load / store the data instead of duplicating the logic from there.

In recent patches, that's not possible without hacks that impede legibility, because SmartCache now deals with HtmlResponse objects instead of render arrays.
Thoughts?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
49.25 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,811 pass(es), 2 fail(s), and 0 exception(s). View
6.64 KB

Further clean-up/simplification:

  • #210's first point made me realize $this->routeMatch is unused; removed it.
  • Having done that, I realized that 99% of the logic for returning early is already in SmartCache's request policy. So, moved the last bits of logic in there too.
  • I also realized that the work in #195 through #203 (which makes SmartCache cache HtmlResponse objects and ensures HtmlRenderer doesn't yet render placeholders) makes it so that it is guaranteed that the HtmlResponse that SmartCache receives always has the required cache contexts. Which means we can simplify SmartCache further, by reverting #184's non-test coverage bits.

(Not a smaller patch, but a simpler patch. Although it would have been smaller if we would have less boilerplate.)

Fabianx’s picture

#207: I think the best is then to just add the necessary send() method to HtmlResponse itself, and check for $this->getSendService() or something like that and call the service if that exists.

e.g. make it possible to plug the send method in HtmlResponse() directly.

Then we don't need to deal with decorators or any of that, which are error prone and get direct integration and other strategies could use something similar.

Exchanging the response mid-event chain is not the right solution.

--

Anyway: Could we split off the renderPlaceholders() move to subscriber to another task, the both RenderStrategies and BigPipe, etc. can work off of that?

And lets have a dedicated subscriber for the renderPlaceholders() so that someone can hook in between.

--

#212: The logic is easy enough, lets keep as is.

Fabianx’s picture

#214 looks fantastic overall! :)

I think once we remove those last 2 test failures we can get this in!

borisson_’s picture

Since this is almost ready, I read trough this patch and found a few small nitpicks as well.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,208 @@
    +    // SmartCache only cares about HTML responses.
    +    if (!$response instanceof HtmlResponse) {
    +      return;
    +    }
    

    shouldn't the DenyNonHtmlRequests Request policy already make sure that smartcache only cares about HTML responses?

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,208 @@
    +    // SmartCache only caches cacheable HTML responses.
    

    I didn't really understand this comment until I read the contents of the else block.
    I think something like: // SmartCache can only cache HTML responses that have a max age would more descriptive.

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,208 @@
    +      // Anonymous function to optimize the cache contexts of CacheableMetadata.
    +      $optimize_cache_contexts = function (CacheableMetadata $cacheability) {
    +        $cacheability->setCacheContexts($this->cacheContextsManager->optimizeTokens($cacheability->getCacheContexts()));
    +      };
    

    I don't think you need to describe in the comment that this is an anonymous function.

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,208 @@
    +      // If the set of cache contexts is different, store the union of the already
    +      // stored cache contexts and the contexts for this request.
    

    80 cols

  5. +++ b/core/lib/Drupal/Core/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,208 @@
    +      // Now that the HTML response is cached, mark the response as a cache miss.
    

    The period makes this go over 80cols, do we care about that?

  6. +++ b/core/lib/Drupal/Core/ProxyClass/SmartCache/DefaultRequestPolicy.php
    @@ -0,0 +1,92 @@
    +/**
    + * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\SmartCache\DefaultRequestPolicy' "core/lib/Drupal/Core".
    + */
    

    Should this comment be in there?

  7. +++ b/core/lib/Drupal/Core/ProxyClass/SmartCache/DefaultRequestPolicy.php
    @@ -0,0 +1,92 @@
    +namespace Drupal\Core\ProxyClass\SmartCache {
    

    Not sure why this class is in this namespace construct, can this be a normal namespace declaration? namespace Drupal\Core\ProxyClass\SmartCache; I haven't seen any other files in this patch use this type of namespace declaration.
    That way this class doesn't need to use FQDNs all over.

  8. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -128,6 +134,33 @@ public function processAttachments(AttachmentsInterface $response) {
       /**
    +   * Renders placeholders.
    +   *
    +   * Renders #attached[placeholders].
    +   *
    

    Can you merge these two lines?

  9. +++ b/core/modules/system/src/Tests/Cache/SmartCacheIntegrationTest.php
    @@ -0,0 +1,147 @@
    +      $this->assertSmartCache($url, ['url.query_args:animal' => $animal]);
    +      $this->drupalGet($url);
    +      $this->assertEqual('HIT', $this->drupalGetHeader('X-Drupal-SmartCache'), 'Render array returned, rendered as HTML response: SmartCache is active, SmartCache HIT.');
    

    Should this also check in the response html that the correct $animal is in the response?

Wim Leers’s picture

So, by now I feel pretty good about the SmartCache patch :)

But! There was one more thing that was nagging me. SmartCache still requires routing and routing access to run. I.e. SmartCache caches per route cache context. But why not make it cache per url cache context? That'd be similar to the PageCache module & middleware.

Reasons I could think of not to do that:

  1. Caching per-route makes it so that /node/1, /node/1?foo=bar, /node/1?foo=llama and so on all end up using the same cached response, if /node/1 doesn't actually care about the foo query arg (if it did, it'd have specified the url.query_args:foo cache context), thanks to SmartCache relying on cache contexts to tell it which variants it should cache. In other words: it avoids cache poisoning (not exactly the right term, because I mean it here as in "filling the cache with garbage"), and ensures faster response times for all HTML requests.
  2. But, surely, that could not be the only reason, right? Well, then I remembered that back when SmartCache was started, the selected route (and therefore response) depended upon both the URL and the Accept header. That changed only weeks ago: see https://www.drupal.org/node/2501221. I.e. content negotiation happened. That's no longer the case.

So, given those facts, I was thinking last night: Why not make SmartCache use the url cache context instead, and then add the same cache poisoning protection?. That protection could be quite simple — roughly speaking: for every URL sans its query string, cache whether the associated response every specified the url.query_args cache context or not. This could then be used by both PageCache and SmartCache to automatically use the cached response for /node/1 when the actual URL is /node/1?foo=bar! This would also help with the scalability of Drupal: it'd be harder to DDoS Drupal.
This is a solution that can be implemented in a separate issue, and would definitely benefit PageCache too.


Given the above, I set out to do a quick experiment: modifying the SmartCache patch as minimally as possible to get an idea of how this would work. Ideally, it'd be converted to a middleware. But as a first step, just making the existing event subscriber run earlier would be sufficient.
In doing so, I ran into my first problem: a response that varies by the user cache context never results in a cache hit, because… SmartCache now runs so early that we haven't created a User object yet!
So, I changed the SmartCache event subscriber to run later (after authentication_subscriber). This is the earliest possible time that could possibly work — note that I am quite likely to encounter further problems, but let's ignore that for a second. Also note this means it can NOT be converted to a middleware.
Let's look at the performance of this incomplete solution. This brings /contact down from 11,605 function calls (in #205) to 10,894. 711 fewer function alls, and ~2 ms faster. Maybe worth it, but only a small incremental gain compared to what the current SmartCache patch already offers.
A next problem is the route cache context: that won't work, since this now runs before routing. But we're assuming here that the route cache context is implied by the url cache context, because we're assuming we don't have Accept header negotiation. So, once we do #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents(), the route cache context would be optimized away automatically (since the url cache context is always present for SmartCache). But that means that any contrib module doing Accept header negotiation on routes that also happen to serve HTML responses (i.e. the ones cached by SmartCache) would need to override SmartCache to a quite large extent, to … bring it back to the behavior that the current patch (#214) implements.

In conclusion:

  • Making SmartCache URL-based instead of route-based is theoretically possible.
  • But the additional performance gains are minor.
  • And it cannot be implemented elegantly; it requires hacks.
  • It would make it very hard for contrib (or a future D8 core release) to bring back Accept header-based negotiation.
  • Therefore I recommend keeping SmartCache route-based (like #214).
  • I do think it is valuable opening an issue for PageCache to implement the strategy described above to normalize URLs like /node/1?foo=bar to /node/1, to increase cache hit ratios and avoid DDoSes by filling the cache with random query strings.
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
49.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,842 pass(es), 2 fail(s), and 0 exception(s). View
5.7 KB

#218

  1. No, DenyNonHtmlRequests is, as its name says, about requests, not responses. I clarified the reasons for this if-statement in the comment though — great question!
  2. Clarified the comment.
  3. Moved it into a protected helper with proper docs.
  4. Fixed.
  5. Yes. Improved.
  6. Yes, because it's automatically generated by that script. You can run that script again, and you'll see the file remains exactly the same.
  7. Again, automatically generated by that script.
  8. Done.
  9. Sure, done.

Thanks for the review! :)

ivanjaros’s picture

This would also help with the scalability of Drupal: it'd be harder to DDoS Drupal.

This sounds really good. But if you have to sacrifice this

And it cannot be implemented elegantly; it requires hacks.
It would make it very hard for contrib (or a future D8 core release) to bring back Accept header-based negotiation.

then I guess routes are much better approach.

On the other hand

bring back Accept header-based negotiation

I think changing it to query argument is now a Drupal way and so contrib modules should follow that.

Fabianx’s picture

TL;DR: I am perfectly fine with smart cache to run after routing, but it should probably use the url cache context.


So there is a misunderstanding:

SmartCache fully uses:

a) a two-tier caching strategy
b) allows cache contexts to bubble up

If smart cache is url based, then the request_format cache context could still bubble up.

That only means that request format needs to be negotiated by a middleware before smart cache ($request->setFormat()).

OR if request_format is determined during routing that smart cache is moved by said contrib module after routing.

I don't think neither is a problem.


To have smart cache fully run in a middleware we need to "cache the cache contexts" similar to the ESI use case based on the user session and only if we can't fulfill them fall back to the subscriber after routing / authentication.

However that middleware could be added at any point in the future, so should not block that.

However for forward compatibility it would be good if smartcache used ['url'] and relied on request_format to bubble up - it just needs to make sure to take into account cache contexts set on $request->data('_access_cacheable_metadata') [or what we called that] if it runs earlier, but before the normal FinishResponseSubscriber.

We can always harden DDOS in the future.

Wim Leers’s picture

#223: I don't think it's valuable to make SmartCache use the url cache context because:
if we run SmartCache AFTER routing already, there's no point in doing so
if we change SmartCache to run BEFORE routing, then the problem that arises is described in #219: the user (and user.*) cache contexts don't work anymore. And who knows what else.

Therefore, I think making SmartCache run before routing should be follow-up material; it can happen at any time in the future (think SmartCache2, which will only be enabled on new sites).


Which bring me to an important point: how do we let users turn off SmartCache? My concern with making it a separate module is that it is too difficult to explain to users what it does. "Page Cache" is simple": "caches HTML for anon users".
But SmartCache works for all users, in all situations, and only for HTML. So… "HTMLResponseCache" may be a better name. But exposing a module with that name is super confusing too.
IMHO, we should ship with SmartCache enabled and not exposed in the UI. Making it configurable using either a container parameter or setting or config, and having settings.local.php disable SmartCache by default (cfr. https://www.drupal.org/node/2259531) seems like a good solution to me.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Revamped IS.

Wim Leers’s picture

Can somebody please add Fabianx to the issue credit? Thanks.

cosmicdreams’s picture

Wim: I think calling it "Smart cache" is fine. Because it is applying a good deal of intelligence in how it handles the cache. I can't think of another kind of cache we would throw at this that would be smarter than this smart cache (that we wouldn't roll into smart cache).

To the site admin / site builder that is curious about what this new fancy thing is and only looks at that name, I would imagine they would eventually learn that turning this on is like supercharging their site.

Fabianx’s picture

#224: Lets keep it focused and go with route for smartcache 1.

A container parameter is fine and enough, this is like block cache, just for a different part.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
60.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,974 pass(es), 2 fail(s), and 0 exception(s). View
21.38 KB

Having discussed this with @Fabianx, we felt like the clearest way to add SmartCache to D8 core, is by putting it in a smart_cache module.

So, this patch:

  • Moves SmartCache into the smart_cache module (all code)
  • Slightly updates the page_cache module's description + hook_help() to make the difference with smart_cache clearer
  • Adds a hook_help() for SmartCache
  • Renames the NoAdminRoutes policy to DenyAdminRoutes, for consistency
  • Adds smart_cache to be enabled by default in the minimal, standard and testing install profiles
  • Updates example.settings.local.php to disable SmartCache by default

Thoughts? :)

Berdir’s picture

Yes, clear +1 on making it a module. Was discussed before and was suggested by me as well before, but back then the coupling way too tight for that to really work IIRC. Yay that that's no longer the case :)

Berdir’s picture

Discussed example.settings.local.php with wim.

I expressed my worries about developers disabling smart, page and render caching locally and then deploy stuff without testing when it's enabled.

We agreed on adding both smart and render cache disabling but commented out by default with a clear comment about the pitfalls.

webflo’s picture

Please add smart_cache module to the replace section in core/composer.json

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
61.65 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,952 pass(es), 2 fail(s), and 0 exception(s). View
1.84 KB

#232: Done.

#233: Done. I had no idea about that, so thanks for pointing that out! Ideally that'd have caused a test failure, so created an issue for adding a test so that happens in the future: #2539682: Add test to ensure composer.json is listing all core modules.

Wim Leers’s picture

+++ b/core/composer.json
@@ -105,6 +105,7 @@
+    "drupal/smartcache": "self.version",

OMG, missing underscore. Wim--

Will fix that in the next reroll.

In the mean time: more feedback/reviews? :)

yched’s picture

A couple thoughts on the overall organisation around request / response policies between page_cache and smart_cache :

  1. Now that smart_cache is a non-required module, isn't it weird that core.services.yml uses the 'smart_cache_response_policy' tag on some of its services ? Shouldn't those be altered in by smart_cache.module ?
    (although, the same could be said about the pre-existing 'page_cache_response_policy' tags I guess...)
  2. Also, I notice that the 'page_cache_request_policy' and 'page_cache_response_policy' tag collector services still live in core.services.yml, while the corresponding services for smart cache live in smart_cache.services.yml. Not sure which way, but seems like we'd want to unify ?
  3. Likewise, only page_cache_response_policy is lazy, page_cache_request_policy is not. On the smart_cache side, both are lazy ? Why is it worth making the request_policy lazy for smart cache and not for page cache ?
  4. So the concepts of request/response policies were tied to the page cache so far, and are now also used by a new, separate kind of cache (smart cache). Yet they are still presented as tied to "PageCache" :
    - the concepts are defined in Core/PageCache,
    - policy implementations are shipped in PageCache folders / namespaces (modules/smart_cache/src/PageCache/RequestPolicy/DefaultRequestPolicy is "The default SmartCache request policy" - feels confusing to have "smart cache" policies shipped in a PageCache folder ?)

    Thus, should we rename the concept and folders to ResponseCachePolicy ? (the current Core/PageCache folder is only about Policy stuff)

  5. Similarly to what smart_cache.module does, should the page_cache-specific Policies move from Core to page_cache.module ?
yched’s picture

Also : In case of a cache miss, the request policy is checked twice (once before checking the cache in onRouteMatch(), once after buidling the uncached response in onResponse()).

- that is a different behavior from page_cache, which only checks the request policy once (before handling the request, and not after computing an uncached response), the inconsistency could be confusing ?
- if we do need to re-check the request policy before caching the computed response, it's possibly not a biggie perf-wise since this won't happen on the next cache hit, but then maybe let's skip it if the request policy failed the first time ? Because it seems we'll be checking it twice on each request to an admin route ?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
58.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,008 pass(es), 2 fail(s), and 0 exception(s). View
4.37 KB
15.81 KB
5.48 KB

Thanks, great review! :)

  1. :) I knew somebody would ask that :) I think it's fine because these tags are still for a *core* module, so it's fine for those service tags to be there: they don't hurt one bit.
  2. The confusing thing about "page cache request/response policies" is that they are NOT only for the page_cache module; they're actually for all responses (pages)! They determine a response's Cache-Control headers, see FinishResponseSubscriber::onRespond(). It is simply the case that the page_cache module respects precisely the same request/response policies!
  3. What a great question! I hadn't even noticed that! It was #2348679: Move the remaining procedural page cache code to the page cache stack middleware that made the response policy in HEAD lazy. AFAICT we only made that one lazy, because we always evaluate the request policy, so there's no savings (in fact only a cost!) to making the request policy lazy. The same applies here, so made the request policy non-lazy.
  4. See point 2. IMO the PageCache part in the namespace is wrong; we should rename it to ResponseCache or Response or ResponseCacheControl. Which is similar to what you are suggesting.
    But, IMO that can be done in a separate issue (before or after this one), because it is an independent, pre-existing issue. (Again, see point 2.)
  5. There aren't any page_cache-specific policies; page_cache simply uses those that policies that apply to all responses. (Again, see point 2.)

So, with point 3 addressed, this should in theory be faster. In practice, it's *marginally* faster, but only within the margin of error. The distribution of the histogram just looks a bit more favorable. (See attached file.)

Wim Leers’s picture

#238: Another good point! :) The only way to avoid doing that though, is by storing the conclusion of the request event subscriber somewhere. Where? On the response itself? IDK what the best practices are around that, except for that it is generally looked down upon. Happy to do whatever people think is the best way!

yched’s picture

Thanks for thé answers in #239, makes sense. Agreed we should rename the PageCache namespace in a separate issue.

#240 :

The only way to avoid doing that though, is
by storing the conclusion of the request event subscriber somewhere. Where?

Can't we put a "smartcache : not cacheable" header or attribute on the request (and then carry over to the Response) ?

Related : might be a good idea to briefly document why smartcache can't be implemented with a middleware like page cache ?

benjy’s picture

  1. +++ b/core/lib/Drupal/Core/ProxyClass/SmartCache/DefaultRequestPolicy.php
    @@ -0,0 +1,92 @@
    +
    +    /**
    +     * Provides a proxy class for \Drupal\Core\SmartCache\DefaultRequestPolicy.
    +     *
    +     * @see \Drupal\Component\ProxyBuilder
    +     */
    +    class DefaultRequestPolicy implements \Drupal\Core\PageCache\ChainRequestPolicyInterface
    +    {
    

    Unrelated i guess but how come this proxy class is not generated with Drupal's coding standards?

  2. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -125,11 +137,26 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +    $this->renderer->executeInRenderContext($render_context, function() use (&$html) {
    +      $this->renderer->render($html);
    +    });
    

    We're not actually using the result of the render() call, could we add a comment. It isn't obvious what's happening.

  3. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,219 @@
    +    // @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_cacheability->setCacheMaxAge(Cache::PERMANENT);
    

    Just pointing this out.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
60.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,962 pass(es), 2 fail(s), and 0 exception(s). View
4.21 KB

Thank you both for your reviews!

#242

  • RE: request attribute: I though that was considered a bad practice now, but I also don't see any other way. Done.
  • RE: why not middleware: done. Is it clear?

#243:

  1. IDK, #2408371: Proxies of module interfaces don't work introduced that; you can create a follow-up issue to address that if you want :)
  2. This is why we need to fix #2495001: [meta] Add a replacement for renderRoot() that returns a cacheable render array instead of a string, because that is indeed unclear/confusing. Documented, and added a @todo to that issue.
  3. Yes… I'm keeping that in the patch for now to simplify testing. I've explicitly made it screamy/ugly so that it won't get committed by accident :) Once this patch is RTBC or almost RTBC, I'll remove that hunk (unless everybody wants me to remove it *now* of course).
plach’s picture

I just tried to add @Fabianx to the credit list. Hopefully he will stick :)

yched’s picture

Thanks @wim !

+ $event->getRequest()->attributes->set(self::SMART_CACHE_REQUEST_POLICY_RESULT, $request_policy_result);
Minor : should use the $request var that was extracted two lines above :-)

Also, nitpick, not sure how we do this elsewhere, but the const name, being a class const, doesn't really need to be prefixed with the SMART_CACHE_ business domain ? (just like the class member is requestPolicy rather than smartCacheRequestPolicy)

Other than that, I'm amazed at how straightforward the patch has become :-) Amazing work.

yched’s picture

  1. --- /dev/null
    +++ b/core/lib/Drupal/Core/ProxyClass/SmartCache/DefaultRequestPolicy.php
    

    Do we really still need that proxy ? Can't seem to find a service that uses it ?

  2. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,245 @@
    +    $stored_cache_contexts = $this->smartContextsCache->get($contexts_cid);
    +    if ($stored_cache_contexts !== FALSE) {
    +      $stored_cache_contexts = $stored_cache_contexts->data;
    +    }
    

    Nitpick : reusing the same var for "cache item" and "content of that cache item" is a bit confusing. Plus, this means in the rest of the code, $stored_cache_contexts is either "the cached contexts" or FALSE, rather than NULL or [] as you would more likely expect on a cache miss ?. That's not really explicit and might lead to surprises in later adjustments / refactorings ?

    Also, isn't that something we already fetched in onRouteMatch() ? Maybe not, since, although the comment is the same ("Get the contexts by which the current route's response must be varied"), the actual code is slightly different :
    - onRouteMatch() fetches $cid = implode(':', cacheContextsManager->convertTokensToKeys(['route'])->getKeys());
    - onResponse() fetches $cid = cacheContextsManager->convertTokensToKeys(['route'])->getKeys()[0]

    If both actually fetch a different context_cache entry, maybe we could clarify/differentiate the code comments ? ATM it's not clear to me why onResponse() only looks at the first key (especially since, according to CacheContextsManager::convertTokensToKeys(), the order seems arbitrary/alphabetical ?)

    If both actually should be fetching the same entry, is that something we want to save in a request attribute as well ? (well, I guess this one only affects real cache misses, and not uncacheable pages, so maybe that's not a big deal)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
57.75 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,872 pass(es), 27 fail(s), and 161 exception(s). View
7.83 KB

#247

  1. Minor : should use the $request var that was extracted two lines above :-)
    /facepalm — fixed :D
  2. RE: const name: fixed.

#248:

  1. Oops, forgot to remove it! Yay :) Fixed.
  2. RE: variable reuse: agreed, fixed.
  3. RE: already fetched: yes, they were the same, so moved that into a request attribute too. Consistency++
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
58.63 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,045 pass(es), 12 fail(s), and 15 exception(s). View
1.86 KB

Fixing those new failures.

Wim Leers’s picture

Title: [PP-1] Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!) » Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)

#2533768: Add the user entity cache tag to user.* cache contexts that need it just landed! This means SmartCache is now unblocked!

yched’s picture

Painful nitpicker returns :-p :
Rather than putting the cache_item object in the CACHE_CONTEXTS_FOR_ROUTE attribute, it might be a bit cleaner to first resolve "$stored_contexts = ($cache_item !== FALSE) ? $cache_item->data : NULL" once and for all and store that resolved value in the attribute, so that onRepsonse() doesn't need to duplicate the unwrapping logic ? We probably don't really need to leak to consumers that this came from a cache bin and is still wrapped by the cache API ?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
58.67 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,042 pass(es). View
2.29 KB

#2430397: When mapping cache contexts to cache keys, include the cache context ID for easier debugging caused those ten failures in the integration test. This interdiff updates the integration test accordingly.

This … should be green! :) :O :)

lauriii’s picture

WOW! Good job Wim & Fabian & all!

Fabianx’s picture

Issue summary: View changes

Last changes look awesome! It is fantastic to see this finally being green!


As we are nearing the RTBC stage.

Proposed commit message:

Issue #2429617 by Wim Leers, Berdir, epari.siva, martin107, Fabianx, yched, MiSc, dawehner: Make D8 2x as fast: SmartCache: context-dependent page caching (for *all* users!)
lauriii’s picture

Not much to say but few comments:

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -621,6 +621,11 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    +    // Mark every non-GET form as uncacheable.
    

    I was wondering if it would be a good idea to change this comment to include why because it is possible to override this

  2. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,276 @@
    +    // SmartCache only works with HTML responses that are actual  HTMLResponse
    

    Double space before HTMLResponse

  3. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,276 @@
    +    // @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_cacheability->setCacheMaxAge(Cache::PERMANENT);
    

    Hmm?

  4. +++ b/core/modules/smart_cache/tests/smart_cache_test/src/SmartCacheTestController.php
    @@ -0,0 +1,51 @@
    +class SmartCacheTestController {
    

    This class is missing class documentation and all the methods are also undocumented

Wim Leers’s picture

Issue summary: View changes
FileSize
58.93 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,049 pass(es). View
2.92 KB

#254: hah :P Done! yched-nitpicking++


Also tried getting rid of the REQUEST subscriber priority stuff, just to double-check that it's still necessary. It still is. So, documented why exactly.

borisson_’s picture

Read trough the entire patch again, found a couple of really small nitpicks.

  1. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,276 @@
    +    // SmartCache only works with HTML responses that are actual  HTMLResponse
    

    Nitpick: there's a double space right before HTMLResponse.

  2. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,276 @@
    +    // There's no work left to be done if this is a SmartCache cache hit.
    +    if ($response->headers->get('X-Drupal-SmartCache') === 'HIT') {
    

    Should we return this early when the X-Drupal-SmartCache Header is 'UNCACHEABLE' as well?

  3. +++ b/core/modules/smart_cache/src/PageCache/RequestPolicy/DenyNonHtmlRequests.php
    @@ -0,0 +1,33 @@
    + * The policy denies caching if the request has a request format other than HTML
    + * or when that is HTML, but additionally a wrapper format is specified.
    

    Can we specify an example of the wrapper format? (ie, drupal_ajax, drupal_dialog, ...)

Wim Leers’s picture

FileSize
59.24 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,046 pass(es). View
4.06 KB

#259:

  1. That's out of scope for this issue, we have #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable for that. Now linking to that in the code comment.
  2. Fixed.
  3. This just is in the patch for profiling purposes. SmartCache only caches responses that are cacheable; but some blocks "infect" the overall page (because e.g. max-age=0 gets bubbled from the block to the overall page). SmartCache won't cache those pages. Think e.g. the breadcrumb/local tasks/local actions blocks; those all still specify max-age=0. We are working on auto-placeholdering in #2499157: [meta] Auto-placeholdering, which avoids that infection and puts a placeholder in the place of the block, so that the block can be rendered separately. Which means that once that lands, SmartCache will be able to cache even those pages.

    So far, I've argued to keep this in for profiling purposes. But, since this patch is now green, I'm removing it now, so that it can really undergo final review :)

  4. Fixed.
Wim Leers’s picture

FileSize
59.23 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,060 pass(es). View
5.64 KB

#260:

  1. Duplicate of #258.2, already fixed in my previous (cross-posted) reroll :)
  2. Hrm, good point. Done! It seems this allows for some nice further code simplification :)
  3. Sure, done.

(Whew, it's getting hard to keep up with all the reviews :D)

borisson_’s picture

#262.2: This makes SmartCacheSubscriber a lot more readable by removing that extra layer of indentation! Awesome.

Wim Leers’s picture

#263: agreed :)


Fixed a bunch of nitpicks/confusing language after a self-review.

yched’s picture

I was wondering whether we should also set the 'X-Drupal-SmartCache: UNCACHEABLE' header on request/response policy deny.
It seems PageCache doesn't - it in fact never sets its X-Drupal-Cache header to UNCACHEABLE, whatever the reason (max age 0, policy deny...). So maybe we should look into unifying that in a followup ?

Wim Leers’s picture

But the request/response policies simply exclude some things; some of them technically are cacheable (e.g. admin routes/responses). X-Drupal-SmartCache: UNCACHEABLE really means that the response itself is uncacheable.

(In the future, admin routes/responses may become cacheable; e.g. /admin/content would make a lot of sense to cache.)

yched’s picture

Sure, maybe UNCACHEABLE is not the right info there, but "some" explicit info might help ? Or we consider that "SmartCache enabled and X-Drupal-SmartCache header absent" implicitly means "excluded because of policy" ?

(again, totally fine for a followup across page_cache and smart_cache - if at all)

Wim Leers’s picture

Or we consider that "SmartCache enabled and X-Drupal-SmartCache header absent" implicitly means "excluded because of policy" ?

Yes; just like PageCache only adds X-Drupal-Cache: MISS if the response didn't already get excluded by the response policy.

yched’s picture

Ok - but then do we want to have X-Drupal-Cache output 'UNCACHEABLE' too on relevant cases ? page_cache currently doesn't distinguish between "excluded by policy" or "response says it's not cacheable".

Side note : opened #2540684: Rename Drupal\Core\PageCache namespace for #237.4 / #239.4

Wim Leers’s picture

But page_cache doesn't care about uncacheable. It assumes — like it always has — that every page is cacheable. To fix that, we have #2352009: Bubbling of elements' max-age to the page's headers and the page cache.

Thanks for opening that other issue!

yched’s picture

Oooh, right, that's why :-) Then it would be for #2352009: Bubbling of elements' max-age to the page's headers and the page cache to spit X-Drupal-Cache UNCACHEABLE when applicable, to be consistent with what smartCache does with its X-Drupal-SmartCache here, right ?

Wim Leers’s picture

Yes!

jhodgdon’s picture

Status: Needs review » Needs work

I was asked to take a look at the hook_help and I have a few comments:

a) In page_cache.module -- The standard way for one module to say "check out this other module" is to link to that other module's help page, not directly to drupal.org. So please change this sentence:

For speeding up your site for authenticated users, see the <a href="!smartcache-documentation">online documentation for the Smart Cache module</a>.

to say something like "To speed up your site for authenticated users, see the Smart Cache module" [also note wording change, "for speeding up" is not great], and then turn "Smart Cache module" into a link to that module's help page. You'll need to have a module exists check in there too. For an example of how to do this... well they are in other places in hook_help(), like there's a link in Menu about Block, and in Field about Field UI, Views for Views UI, etc.

b) In smart_cache.module help: Although I found the "Nothing needs to be configured — that is why it is smart!" text amusing, I don't think it is great for documentation. Instead of having a "Configuring" heading and then saying "you don't have to configure it", I think it would be better to just add a sentence to the previous paragraph under "Speeding up your site" to say simply "The module requires no configuration.".

c) I also think that the help is confusing and contradictory. The first line of the help says it caches "parts of pages", but then in the Speeding part, it is talking about whole pages. So this needs some attention. It would be nice if by reading this help I could understand what this module does, why it's better than Internal Page Cache alone, etc.

d) In system_help(), there is a link to the Internal Page Cache module. Probably something should be added there to link to the Smart Cache module too.

Wim Leers’s picture

a) Done.
b) Done.
c) Good point. I refrained from explaining that in the documentation, because i) it's advanced, ii) it links the d.o handbooks precisely for that reason :)
d) Done.

You'll probably have more nitpicks, but this should be a step in the right direction :)

jhodgdon’s picture

MUCH better, thanks!

A few small suggestions:

a) "The Smart Cache module caches pages minus the personalized parts in the database" ... I found that a bit hard to parse. How about:

The Smart Cache module caches pages in the database, minus the personalized parts.

b)

+      $output .= '<dd>' . t('The module requires no configuration.') . '</dd>';
       $output .= '<dd>' . t('(Every part of the page contains metadata that allows Smart Cache to figure this out on its own.)') . '</dd>';

I think that these two sentences could be put into the same paragraph, and I don't think the second one needs to be in parentheses.

The rest looks great, thanks!

dawehner’s picture

I really like how it is not required to change a lot of existing code! Great work wim.

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -621,6 +621,12 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    +      $form['#cache']['max-age'] = 0;
    

    We still don't have any constant for 0?

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponse.php
    @@ -36,12 +36,15 @@ public function setContent($content) {
         parent::setContent($content);
    +
    +    return $this;
    

    Can we do a return parent::setContent($content);?

  3. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -75,6 +77,13 @@ class HtmlRenderer implements MainContentRendererInterface {
       /**
    +   * The renderer configuration array.
    +   *
    +   * @var array
    +   */
    +  protected $rendererConfig;
    +
    
    @@ -89,14 +98,17 @@ class HtmlRenderer implements MainContentRendererInterface {
    +   * @param array $renderer_config
    +   *   The renderer configuration array.
    

    It would be nie to document what is in there or at least a pointer to the same information in the render cache service

  4. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,272 @@
    + * evaluated after routing. (Examples: 'user', 'user.permissions', 'route' …)
    

    It would be interesting what happens if you don't vary by user. Could you then say route is actually less detailed then URL decide to cache by URL, which then is available at middleware level ...

  5. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,272 @@
    +  const ATTRIBUTE_REQUEST_POLICY_RESULT = '_smart_cache_request_policy_result';
    ...
    +  const ATTRIBUTE_CACHE_CONTEXTS_FOR_ROUTE_CID = '_smart_cache_cache_contexts_for_route_cid';
    ...
    +  const ATTRIBUTE_CACHE_CONTEXTS_FOR_ROUTE = '_smart_cache_cache_contexts_for_route';
    

    +1 for documenting the attributes. I still think attributes aren't that bad as other people considered them to be, at least for internal value.

  6. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,272 @@
    +    $cid = implode(':', $this->cacheContextsManager->convertTokensToKeys(['route'])->getKeys());
    +    $cached = $this->smartContextsCache->get($cid);
    

    Do we have some test coverage, maybe a test event subscriber which ensures everywhere that the cache contexts don't vary on the same route for different parts of a test function? This could maybe let us find a couple of problematic places.

  7. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,272 @@
    +    // SmartCache only works with HTML responses that are actual HTMLResponse
    +    // objects, it does not work with plain Response objects that happen to
    +    // return HTML. (SmartCache needs to be able to access and modify the
    +    // cacheability metadata associated with the HTML response.)
    +    if (!$response instanceof HtmlResponse) {
    

    So I'm curious why would it not work with rest responses with cacheability metadata?

  8. +++ b/core/modules/smart_cache/src/PageCache/RequestPolicy/DenyNonHtmlRequests.php
    @@ -0,0 +1,34 @@
    + * The policy denies caching if the request has a request format other than HTML
    + * or when that is HTML, but additionally a wrapper format is specified. For
    + * example, 'drupal_ajax', 'drupal_modal' are wrapper formats.
    + *
    

    Somewhere it would be great to document why

  9. +++ b/core/modules/smart_cache/src/PageCache/ResponsePolicy/DenyAdminRoutes.php
    @@ -0,0 +1,48 @@
    + *
    + * This policy rule denies caching of responses generated for admin routes.
    + */
    

    Let's answer why, sorry.

  10. +++ b/core/modules/system/system.routing.yml
    @@ -454,6 +454,8 @@ system.db_update:
    +  options:
    +    _admin_route: TRUE
    

    That sounds like a workaround for me ... its not necessary the admin theme, right? This is just using the bare html renderer. I would rather mark it as not cacheable directly

Wim Leers’s picture

FileSize
66.99 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,046 pass(es), 47 fail(s), and 5 exception(s). View
9.53 KB

#275:
a) Thanks, applied your suggestion, that is so much better! :) Updated the module description in smart_cache.info.yml similarly.
b) Oh, yes, of course!

@jhodgdon++

#276:
:)

  1. No, we indeed still don't have a constant for zero.
  2. LOL, of course! Silly me.
  3. Done; pointed to sites/default/default.services.yml.
  4. Not varying by user if the user cache context is present means cache poisoning: the cache will simply not be per user, which means I can see things intended only for you and vice versa, depending on who accessed it first.
  5. :) I'm fine with this; it makes sense here. I don't see a better alternative in any case.
  6. This needs a separate comment/reroll; but I'm first doing the next point. So, this will be addressed in the second comment after this one (#279).
  7. This needs a separate comment/reroll; addressing in the next comment (#278).
  8. The patch does this since #123, i.e. since #2481453: Implement query parameter based content negotiation as alternative to extensions, because before that it only supported the html request format, but since #2481453, caring just about the request format isn't enough anymore; we also need to care about the wrapper format. However, you're right; this is not necessary. We can fix it by having the logic that determines which wrapper format to use (MainContentViewSubscriber::onViewRenderArray() add the url.query_args:_wrapper_format cache context.
    Note this also means it's in fact quite easy to do a DDoS attack: just specify random _wrapper_format query arguments. This is a consequence of having "ajax" and "dialog" and "modal" not be actual request formats, but just wrapper formats. But that's out of scope to fix here.
    What is in scope here is to allow ajax/dialog/modal (i.e. HTML + wrapper format) responses to be cached by SmartCache. Except AJAX responses aren't cacheable yet. But, once they are made cacheable — or at least dialog/modal at first — this will be perfectly possible.
  9. Documented.
  10. Changed to no_cache: true instead.
Wim Leers’s picture

Issue summary: View changes
FileSize
67.74 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,074 pass(es), 39 fail(s), and 0 exception(s). View
22.17 KB

This is addressing #276.7:

+++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php

@@ -0,0 +1,272 @@
+    // SmartCache only works with HTML responses that are actual HTMLResponse
+    // objects, it does not work with plain Response objects that happen to
+    // return HTML. (SmartCache needs to be able to access and modify the
+    // cacheability metadata associated with the HTML response.)
+    if (!$response instanceof HtmlResponse) {

So I'm curious why would it not work with rest responses with cacheability metadata?

HAH! This is an EXCELLENT EXCELLENT question! You're absolutely right! Since #195, SmartCache is no longer deeply intertwined with HtmlRenderer. Which is why it is now indeed completely possible to do this in a generic way!

Fixed!

Wim Leers’s picture

Issue summary: View changes
FileSize
63.93 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,043 pass(es), 39 fail(s), and 0 exception(s). View
18.41 KB

This is addressing #276.6:

+++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
@@ -0,0 +1,272 @@
+    $cid = implode(':', $this->cacheContextsManager->convertTokensToKeys(['route'])->getKeys());
+    $cached = $this->smartContextsCache->get($cid);

Do we have some test coverage, maybe a test event subscriber which ensures everywhere that the cache contexts don't vary on the same route for different parts of a test function? This could maybe let us find a couple of problematic places.

First: It is okay for a response to return different cache contexts: it's possible that only if the user does have a certain permission (i.e. for a certain variant of the user.permissions) cache context, that something else is shown that itself has cache contexts that aren't otherwise present (e.g. render something that depends on a certain URL query argument, so it adds url.query_args:foo).

Second: this is the most important missing part in the current patch IMHO: test coverage of this logic.

Third: This is basically a second implementation of cache context bubbling for this particular use case. See RenderCache for the existing implementation and \Drupal\Tests\Core\Render\RendererBubblingTest::testConditionalCacheContextBubblingSelfHealing() for the extensive existing test coverage.

Fourth: I tried getting rid of SmartCache's re-implementation in #189/#212, but failed, because SmartCache deals with Response objects and RenderCache deals with render arrays. It seemed like a bad idea there, but… really, it is not. Duplicating the exact same logic and all of its test coverage, that's just a recipe for disaster.

So, in this reroll, SmartCache now uses RenderCache, so we automatically have all the needed test coverage :)

yched’s picture

Wow. Expanding to all CacheableResponses is huge !
Still wrapping my head around the use of RenderCache :-). Meanwhile, a couple more remarks :

  1. +++ b/core/modules/smart_cache/smart_cache.services.yml
    @@ -0,0 +1,32 @@
    +  cache.smart_cache:
    +    class: Drupal\Core\Cache\CacheBackendInterface
    +    tags:
    +      - { name: cache.bin }
    +    factory: cache_factory:get
    +    arguments: [smart_cache]
    

    I don't see where that bin is used now ?
    [edit : never mind, found out, it's specified in SmartCacheSubscriber::$smartCacheRedirectRenderArray]

  2. +++ b/core/modules/smart_cache/tests/smart_cache_test/src/SmartCacheTestController.php
    @@ -0,0 +1,97 @@
    +  /**
    +   * A route returning a Response object.
    +   *
    +   * @return \Drupal\Core\Cache\CacheableResponseInterface
    +   *   A CacheableResponseInterface object.
    +   */
    +  public function cacheableResponse() {
    

    minor : the phpdoc description is copied from the previous method, that one should be "... returning a CacheableResponse object" ?

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
    @@ -90,7 +92,14 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
    +      if ($response instanceof CacheableResponseInterface) {
    +        $main_content_view_subscriber_cacheability = (new CacheableMetadata())->setCacheContexts(['url.query_args:' . static::WRAPPER_FORMAT]);
    +        $response->addCacheableDependency($main_content_view_subscriber_cacheability);
    +      }
    

    Side note, not for the patch here, but adding a cache context to a response is kind of convoluted.
    Can't we have CacheableResponseInterface extend RefinableCacheableDependencyInterface and gain direct setters ?

yched’s picture

+++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
@@ -0,0 +1,258 @@
+    // Embed the response object in a render array so that SmartCache is able to
+    // cache it, handling cache redirection for us.

shouldn't that be "... so that *RenderCache* is able to cache it" ?

dawehner’s picture

Not varying by user if the user cache context is present means cache poisoning: the cache will simply not be per user, which means I can see things intended only for you and vice versa, depending on who accessed it first.

Well for sure ... but just assume we don't have something per user. Once you have that, you might be able to have all the other cache tokens available on middleware level.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
75.7 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,002 pass(es), 2 fail(s), and 0 exception(s). View
15.38 KB

#277: The addition of the 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT cache context in #277 caused a whole bunch of test failures. Easy fixes though: we need to update the expectations of some tests.
Because it was incorrectly split up from the 2 other patches (278/279), it also fails the SmartCacheIntegrationTest.

(This causes the patch size to increase significantly, even though the changes themselves are tiny.)

#278: Here, the SmartCacheIntegrationTest passes again. But it has failures in Drupal\rest\Tests\ReadTest. Because some routes' controllers return different responses based on the request format (rather than having a separate route for each request format.) Solved by making SmartCache always vary by the request_format cache context too.


#282:

  1. Done.
  2. +1 to having CacheableResponseInterface extends Response implements RefinableCacheableDependencyInterface, and using RefinableCacheableDependencyTrait. Opened #2541272: Improve CacheableResponseInterface for that.

#284: Fixed.

Wim Leers’s picture

Well for sure ... but just assume we don't have something per user. Once you have that, you might be able to have all the other cache tokens available on middleware level.

True — to some extent. How are you going to get the route cache context in a middleware? And all the cache contexts that depend on the current user?

But, yes, this is super interesting/potentially powerful stuff to investigate, because it may yield additional performance benefits. Let's do that in a follow-up :) Created #2541284: Investigate moving Dynamic Page Cache into a middleware at the cost of doing authentication manually if needed for that.

Fabianx’s picture

#287: Yes, lets explore in a follow-up, ESI and other middlewares have the same problems that cache contexts can only be fulfilled once http kernel processing has started and not lazily.

However we can cache the cache contexts per session potentially and then we can move it pretty early in the chain.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
76.43 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,036 pass(es). View
813 bytes

#288: interesting! Can you post that as a comment on #2541284: Investigate moving Dynamic Page Cache into a middleware at the cost of doing authentication manually if needed, or update the IS?


Forgot one bit; should be green again now.

P.S.: #285 had 100,002 assertions, of which 2 failed. I wonder if it's the first time we have exactly 100K assertions passing? :)

Wim Leers’s picture

yannisc’s picture

I tried #290 through simplytest.me and I get the UNCACHEABLE header. I'm sure I'm missing something here, which would make it much better to understand if it was mentioned in the documentation.

borisson_’s picture

Reviewed the patch again after the recent changes, found 2 more nitpicks:

  1. +++ b/core/modules/smart_cache/smart_cache.module
    @@ -0,0 +1,29 @@
    +      $output .= '<dd>' . t('Pages are stored the first time they are requested if they are safe to cache, and then are reused. Personalized parts are excluded automatically. Depending on your site configuration and the complexity of particular pages, Smart Cache may significantly increase the speed of your site, even for authenticated users.') . '</dd>';
    

    can you change and then are reused into and then they are reused. That makes this a little bit more fluent to read.

  2. +++ b/core/modules/smart_cache/smart_cache.module
    @@ -0,0 +1,29 @@
    +      $output .= '<dd>' . t('The module requires no configuration. Every part of the page contains metadata that allows Smart Cache to figure this out on its own.') . '</dd>';
    

    While this is true for all the code that core (and hopefully contrib) provides, should we mention something about adding cache contexts / cache tags for custom code here, or is that something that should go in the docs on d.o?

jhodgdon’s picture

Thanks for taking a look at the patch @borrison_!

I actually disagree with both of your comments though... I think "then are reused" is clear, and regarding item 2, hook_help() is documentation for end users, not for developers. The documentation for developers should be put into (a) topics using @defgroup and (b) pages on drupal.org. I believe that we already have sufficient developer documentation about cache tags and cache contexts in both places.

larowlan’s picture

jhodgdon’s picture

I thought there were also some blockers to page cacheing in the Search module? But maybe they're not the blockers you are talking about.

lauriii’s picture

@yannisc: if any element has max-age set to 0 it will not cache the whole page. I think now there is multiple places setting it to 0 but it can be worked on after this issue has been fixed

yannisc’s picture

@laurii: Thank you for the clarification. Should we add this in the documentation, along with how we check which elements have max-age=0?

Wim Leers’s picture

#292 + #298 + #299: See #258.3 + #261.3.

But basically:, as @lauriii already said: max-age=0 causes SmartCache not to cache. As the IS says: It's very simple, dumb and stupid. It simply varies by all cache contexts on the page.

What we still need, is the tools to make it easy to see — while developing — what the cacheability of things are. See http://wimleers.com/blog/renderviz-prototype for that. In fact, I just made https://www.drupal.org/project/renderviz compatible (commit d99477103e80dce3baa0159801ae6f6abb7b4995) with D8 beta 14. Apply the core patch, install the module, and you can see something like the attached screenshot. As you can see, it is the breadcrumb block that is causing every page to become uncacheable. #2483183: Make breadcrumb block cacheable (currently RTBC!) will make that block cacheable and will this effectively make SmartCache work on most pages.

Cacheability of HEAD + SmartCache patch, visualized by renderviz
Note the red outline around the entire page; this means the overall page has max-age=0, which means SmartCache won't cache the page.
Cacheability of HEAD + SmartCache patch + cacheable breadcrumb block patch, visualized by renderviz
Note that the red outline around the entire page is gone; this means SmartCache can cache it.
The reason there is no red outline around the entire page despite the comment form still being all red, is that the comment form is in fact a placeholder, that is only rendered at the very last moment, i.e. after SmartCache.

(Yes, the visualization — and accessibility — of renderviz is still lacking. If you want to help out with that, ping me, or file issues/patches at https://www.drupal.org/project/renderviz.)


#293 + #294: Nothing to do for me then I guess :)

#296: nope, that's not blocking SmartCache (though it should be fixed for sure; there simply isn't any test coverage for that one, which is why SmartCache is green without that issue having been fixed). The last blocker was #2533768: Add the user entity cache tag to user.* cache contexts that need it, which has since been fixed.

#297: Those can be fixed independently of this issue; they're a pre-existing problem, also for Page Cache. (They were only discovered after Page Cache landed. And yes, I still have that issue open in a tab — just haven't gotten around to it yet.)

#298: Indeed, thanks!


Straight reroll, with one bugfix: creating the SmartCache vs. SmartCache + cacheable breadcrumb block screenshots above using renderviz made me discover a bug in HEAD: FormBuilder does not pass on the method specified in $form['#method'] to FormState. So, FormState still thought that the search block's form was a POST form, hence it was still setting max-age=0 on that.

(The screenshots include this fix.)

Wim Leers’s picture

And of course I forgot to mention the most important thing in #300: we're working on auto-placeholdering of things that are detected to be uncacheable, over at #2543332: Auto-placeholdering for #lazy_builder without bubbling. That would've automatically placeholdered the uncacheable breadcrumbs block, and thus allowed SmartCache to still work despite the breadcrumbs block! (Just like SmartCache still works despite the comment form, because the comment form is manually placeholdered.) Of course, it's even better to have the breadcrumbs block just be cacheable, then no placeholdering is necessary.

#2543332: Auto-placeholdering for #lazy_builder without bubbling has several child issues that are ready for final review: #2543332: Auto-placeholdering for #lazy_builder without bubbling + #2543340: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered).

Wim Leers’s picture

Quick status update!

#2543332: Auto-placeholdering for #lazy_builder without bubbling landed! If #2543340: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) now also lands (a soft blocker for that was discovered and fixed already: #2543554: Clean up Help & Statistics blocks), then we won't have uncacheable blocks preventing SmartCache from caching pages anymore.

Alternatively, if #2483183: Make breadcrumb block cacheable lands (it was RTBC, but committer Alex Pott found some areas that needed more work), then the breadcrumbs block — which is the main reason why most pages are not cacheable by SmartCache — will become cacheable and hence SmartCache will also be effective on most pages.

Holding off on rerolling this, since there seems little point at the moment — if you want to review this, #300 is the one :)

P.S.: finally, because #2543332: Auto-placeholdering for #lazy_builder without bubbling was committed, #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags is now unblocked, and has a green patch too. This will ensure that blocks whose contents contain poorly cacheable things will also be automatically placeholdered.

Wim Leers’s picture

FileSize
76.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,631 pass(es), 7 fail(s), and 0 exception(s). View

#2483183: Make breadcrumb block cacheable just landed. And as said before, this is the last block preventing most pages from being cached by SmartCache.

So, as of right now, if you apply the SmartCache patch, you can actually see it in action.


Patch rerolled. Just one small conflict to resolve: the changes in NodeTranslationUiTest are no longer necessary, so the patch even became slightly smaller.

ivanjaros’s picture

So what is the ETA for core?

dawehner’s picture

We will have fun one #144538: User logout is vulnerable to CSRF lands, don't we? Or will we autoplaceholder that entire menu in that case?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
91.52 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,524 pass(es), 24 fail(s), and 2 exception(s). View
16.6 KB

#304: That depends completely on the reviews this issue gets — it needs to be thoroughly questioned, reviewed before it can be committed, even more so than most patches.

I personally hope that the ETA is soon :) I've been working towards this for almost exactly 6 months (6 days short of 6 months!) — with the help of many others obviously.


So, turns out the uncacheable breadcrumb block was masking several problems. Hence the 7 failures. I also found one small bug/optimization/simplification in the patch.

Changes:

  • HtmlRenderer was slightly simplified/optimized.
  • the failures in CommentNonNodeTest were because entity_test/structure is actually an admin route, but wasn't marked as such because it doesn't have the admin/ prefix. So, changed the system path for that route. This was introduced several years ago, in "the early new routing system days". I checked with Berdir, and he confirms/thinks that was simply an oversight.
  • the failures in ForumTest were due to a missing cache tag. Test coverage added.

Will fix the remaining test failures tomorrow.

dawehner’s picture

+++ b/core/modules/comment/src/Tests/CommentNonNodeTest.php
@@ -258,15 +258,15 @@ function testCommentFunctionality() {
-    $this->drupalGet('entity_test/structure/entity_test/fields');
+    $this->drupalGet('admin/structure/entity_test/entity_test/fields');

It feels we are loosing some test coverage with that we might want to have actually

Wim Leers’s picture

FileSize
95.66 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,538 pass(es), 1 fail(s), and 0 exception(s). View
4.58 KB

Hrm, not only is that route change (system path rename) quite large already in #307, I clearly also missed several places. This fixes that. But it makes the patch even bigger. I'm going to split it off into a separate issue so we can make this patch smaller again.

#308: Can you be more specific about what we lose?

larowlan’s picture

What is great here is how little code there is in this patch - looks great Wim - just a few questions and comments

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php
    @@ -40,7 +40,7 @@ public function onRequestDeriveFormWrapper(GetResponseEvent $event) {
    -    $events[KernelEvents::REQUEST][] = array('onRequestDeriveFormWrapper', 29);
    

    side note: I wonder if its worth creating constants somewhere in the routing system - so we could do stuff like SomeRouteInterface::FORM_WRAPPER_DERIVATION - 1 which would make it clear that the subscriber went just before that particular instance.

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -128,6 +134,31 @@ public function processAttachments(AttachmentsInterface $response) {
    +   * Renders placeholders (#attached[placeholders]).
    

    nit: should we single quote placeholders?

  3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -128,6 +134,31 @@ public function processAttachments(AttachmentsInterface $response) {
    +   *   The HTML response whose placeholders to replace.
    

    %s/to replace/are being replaced

  4. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -128,6 +134,31 @@ public function processAttachments(AttachmentsInterface $response) {
    +    $this->renderer->renderRoot($build);
    

    Can we get a comment as to why we render here but don't use the output/return? We have one added later on in the patch in HtmlRenderer::renderResponse, happy for it to be similar.

  5. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
    @@ -16,6 +16,9 @@
    + *   redirects in a non-render array-specific way.
    

    Can we get a follow up/issue number here?

  6. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -115,6 +115,7 @@ function createBook() {
    +    $this->dumpHeaders = TRUE;
    
    @@ -151,7 +152,7 @@ public function testBookNavigationCacheContext() {
    +  function atestEmptyBook() {
    
    @@ -166,7 +167,7 @@ function testEmptyBook() {
    +  function atestBook() {
    
    @@ -370,7 +371,7 @@ function createBookNode($book_nid, $parent = NULL) {
    +  function atestBookExport() {
    
    @@ -415,7 +416,7 @@ function testBookExport() {
    +  function atestBookNavigationBlock() {
    
    @@ -438,7 +439,7 @@ function testBookNavigationBlock() {
    +  function atestNavigationBlockOnAccessModuleInstalled() {
    
    @@ -469,7 +470,7 @@ function testNavigationBlockOnAccessModuleInstalled() {
    +   function atestBookDelete() {
    
    @@ -497,7 +498,7 @@ function testBookDelete() {
    +  function atestBookNodeTypeChange() {
    
    @@ -590,7 +591,7 @@ function testBookNodeTypeChange() {
    +  public function atestBookOrdering() {
    
    @@ -618,7 +619,7 @@ public function testBookOrdering() {
    +  public function atestBookOutline() {
    
    @@ -672,7 +673,7 @@ public function testBookOutline() {
    +  public function atestSaveBookLink() {
    
    @@ -692,7 +693,7 @@ public function testSaveBookLink() {
    +  public function atestBookListing() {
    
    @@ -708,7 +709,7 @@ public function testBookListing() {
    +  public function atestAdminBookListing() {
    
    @@ -721,7 +722,7 @@ public function testAdminBookListing() {
    +  public function atestAdminBookNodeListing() {
    
    @@ -738,7 +739,7 @@ public function testAdminBookNodeListing() {
    +  public function atestHookNodeLoadAccess() {
    

    debug code still?

  7. +++ b/core/modules/smart_cache/smart_cache.info.yml
    @@ -0,0 +1,6 @@
    +description: 'Caches pages, minus the personalized parts. Works well for websites of all sizes.'
    

    The 'works well for websites of all size' is a bit bizarre.

  8. +++ b/core/modules/smart_cache/smart_cache.module
    @@ -0,0 +1,29 @@
    + * Caches HTML responses, request and response policies allowing.
    

    sentence needs work

  9. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,265 @@
    + * Returns cached responses, as early and avoiding as much work as possible.
    

    no need for the comma here

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
92.58 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,665 pass(es). View
5.82 KB

Fixing the last fail. And removing debugging changes that I accidentally included in #307.

Wim Leers’s picture

FileSize
92.82 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,673 pass(es). View
3.61 KB

#310: Thanks! Glad you like it :)

  1. That's an interesting idea for sure :) We already have #2510738: Audit (and reorganize?) Kernel event listener priorities for improving the organization of kernel event subscribers and their priorities. I've repeated your idea there: #2510738-11: Audit (and reorganize?) Kernel event listener priorities.
  2. Done.
  3. Done.
  4. Great point, done.
  5. Done, and here's the link: #2551419: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way.
  6. Yes :P Fixed in previous comment.
  7. This was written to contrast with page_cache.info.yml's Caches entire pages for anonymous users. Works well for small to medium-sized websites. — can you make a suggestion on how to phrase it differently?
  8. Heh. In looking at this, found two unused use statements; fixed that. I haven't changed the wording here though, it's again mirrored after page_cache.module's wording: Caches responses for anonymous users, request and response policies allowing. — suggestions?
  9. Fixed.

Comment 100π :)

Next: moving out the route change as mentioned in #309.

Wim Leers’s picture

And here's that part moved out into a separate issue: #2551429: FieldUI should accommodate route options in RouteSubscriber. Will make this patch 14 KB smaller again.

Not postponing this issue on that one; it's just to make this patch easier to review.

Wim Leers’s picture

FileSize
83.7 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,252 pass(es). View
16.84 KB

Changes:

  1. Thanks to #2540416: Move update.php back to a front controller, this became slightly simpler again, the changes in system.routing.yml can be removed.
  2. Integration test coverage added to ensure certain responses in the Standard install profile are in fact cacheable by SmartCache, to prevent regressions.
  3. #2551429: FieldUI should accommodate route options in RouteSubscriber was RTBC, but was considered too disruptive. Instead, it was decided to just mark that route as an admin route, without changing the system path. This now uses the patch in #2551429-11: FieldUI should accommodate route options in RouteSubscriber, which is an alternative solution with absolutely zero disruption. And makes this patch ~10 KB smaller
Wim Leers’s picture

Wim Leers’s picture

FileSize
71.3 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,284 pass(es). View
779 bytes

#2551429: FieldUI should accommodate route options in RouteSubscriber and #2551989: Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor landed! That already helps make this patch considerably smaller.

Because #2551989 has landed, #2349011: Support placeholder render strategies so contrib can support BigPipe, ESI… is now also unblocked!

This is a straight reroll/rebase, with only a single change: an early return statement for debugging purposes that I accidentally included.


Next: get #2552013: Follow-up for #2481453: ensure the 'url.query_args': MainContentViewSubscriber::WRAPPER_FORMAT cache context is set to RTBC (it's close), and #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable. I'll try to open additional issues later.

andypost’s picture

It's really close! only one nit

  1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
    @@ -16,6 +16,9 @@
    + * @todo Refactor this out into a generic service capable of cache redirects,
    + *   let RenderCache use that. https://www.drupal.org/node/2551419
    
    +++ b/core/modules/path/src/Tests/PathAliasTest.php
    @@ -57,6 +59,8 @@ function testPathCache() {
    +    // @todo Remove this once https://www.drupal.org/node/2480077 lands.
    

    only 2 todos! yay!

  2. +++ b/sites/example.settings.local.php
    @@ -51,12 +51,25 @@
    -$settings['cache']['bins']['render'] = 'cache.backend.null';
    +# $settings['cache']['bins']['render'] = 'cache.backend.null';
    ...
    +# $settings['cache']['bins']['smart_cache'] = 'cache.backend.null';
    

    Suppose they should be uncommented

Wim Leers’s picture

Related issues: +#2554579: Forum index response is missing the vocabulary cache tag, +#2554585: Tracker responses are missing a cache tag and a cache context
FileSize
70.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,705 pass(es). View
1.42 KB

Since #318, #2552013: Follow-up for #2481453: ensure the 'url.query_args': MainContentViewSubscriber::WRAPPER_FORMAT cache context is set got RTBC'd and #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable saw some discussion.

The next step was to try to move as many of the final small bits of cacheability fixes out into separate issues. Only two of those make sense to go in independently of this patch, because it makes sense to have explicit test coverage for them. The others are very small cacheability fixes that really only make sense in combination with SmartCache.
So, I opened #2554579: Forum index response is missing the vocabulary cache tag & #2554585: Tracker responses are missing a cache tag and a cache context for those two fixes.


#319:

  1. :) — and those are actually pre-existing problems independent of this issue!
  2. This was intentional, see #232

Removed two small leftovers in this reroll. Also rebased on HEAD.

Wim Leers’s picture

Issue summary: View changes
FileSize
53.4 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch smartcache-2429617-321.patch. Unable to apply patch. See the log in the details link for more information. View

Woot, #2552013: Follow-up for #2481453: ensure the 'url.query_args': MainContentViewSubscriber::WRAPPER_FORMAT cache context is set landed, and #2554579: Forum index response is missing the vocabulary cache tag landed amazingly fast (it was super trivial after all)!

That means this patch can now shed about 25% of its weight! Even if the other patches land in the mean time, they won't make this patch significantly smaller.

IS updated to reflect that the scope of the patch is now quite a bit smaller, now that the recent problems that this patch uncovered have already been fixed in other issues :)

I'll keep this patch up-to-date, applying to HEAD. It's ready. It's just a matter of deciding when/if this lands.

tstoeckler’s picture

Read through the entire patch and it is really a very good sign, when someone like me - who has no real in-depth knowledge of the new caching world order - can understand the entire patch not only in broad terms but in its completeness. The code is very nicely architected and thoroughly documented, otherwise I would have had no chance at all to grok this. Awesome work!!!

I have some minor comments, probably not worth a re-roll just for that, though.

  1. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestAccessBlock.php
    @@ -63,7 +63,7 @@ public static function create(ContainerInterface $container, array $configuratio
    -    return $this->state->get('test_block_access', FALSE) ? AccessResult::allowed() : AccessResult::forbidden();
    +    return $this->state->get('test_block_access', FALSE) ? AccessResult::allowed()->setCacheMaxAge(0) : AccessResult::forbidden()->setCacheMaxAge(0);
    

    Super minor, but I think would be more readable to do:

    $access_result = ... ? ... : ... ;
    $access_result->setCacheMaxAge(0);
    return $access_result;
    
  2. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,265 @@
    +use Drupal\Core\Render\Element;
    

    I might be missing something, but I think this is unused.

  3. +++ b/core/modules/smart_cache/src/EventSubscriber/SmartCacheSubscriber.php
    @@ -0,0 +1,265 @@
    +      '#attached' => '',
    

    Also super minor, but it looks kind of strange to have #attached be a string. Shouldn't/Couldn't it be an empty array?

  4. +++ b/core/modules/smart_cache/src/PageCache/RequestPolicy/DefaultRequestPolicy.php
    @@ -0,0 +1,29 @@
    +class DefaultRequestPolicy extends ChainRequestPolicy {
    +
    +  /**
    +   * Constructs the default SmartCache request policy.
    +   */
    +  public function __construct() {
    +    $this->addPolicy(new CommandLineOrUnsafeMethod());
    +  }
    

    As far as I can tell, this "chains" only a single policy, so why not use the one policy in the first place? The chaining seems overkill here, no?

  5. +++ b/core/modules/smart_cache/src/PageCache/ResponsePolicy/DenyAdminRoutes.php
    @@ -0,0 +1,51 @@
    + * Cache policy for routes with the '_admin_route' option set.
    

    I think we need a separate change notice for this. I realized this when reading the discussion above about adding _admin_route to the entity test routes. So far _admin_route was pretty much cosmetic, i.e. it determined which theme is used and not (much) more. This makes it so that if you provide a non-admin page which is i.e. an entity list, you will have to take great care about caching metadata.

  6. +++ b/core/modules/smart_cache/src/Tests/SmartCacheIntegrationTest.php
    @@ -0,0 +1,119 @@
    +    $this->assertFalse($this->drupalGetHeader('X-Drupal-SmartCache'), 'Response returned, cacheable response, admin route: SmartCache is ignoring');
    +
    +
    +    // Max-age = 0 responses are ignored by SmartCache.
    

    Double newline

effulgentsia’s picture

Wim and I had a long conversation about the concerns that committing this patch (once it's ready) would be disruptive to contrib, so I opened a separate policy issue for all of us to discuss that further. See #2556889: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation. Feedback there welcome and appreciated.

effulgentsia’s picture

Priority: Major » Critical

#2470679: [meta] Identify necessary performance optimizations for common profiling scenarios says that a performance issue should be critical if:

Over ~10ms savings with warm caches and would have to be deferred to 9.x

This issue has not been prioritized as critical up until now because we thought that even if it didn't make it into 8.0, it could still be eligible for 8.1, and thus not meet that second criterion. That may still be true, but per #2556889-14: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation, I'm less confident of that now, so raising this to critical until we sort that out.

xjm’s picture

Discussed with @alexpott, @effulgentsia, @catch, and @webchick. We were mostly agreed that it seemed safer overall to do this issue pre-RC than in a minor. (See extended discussion in #2556889: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation.) We also discussed strategies to mitigate the risk of delaying the RC, including:

  • Addressing security-sensitive functionality (like node access, users, etc.)
  • Enabling it sooner rather than later to begin flushing out potential bugs.
  • Disabling caching on routes that show cache invalidation bugs.
  • As a fallback, disabling the module in core profiles if there are an unmanageable number of cache invalidation bugs.
  • Not postponing RCs on account of specific/manageable cache invalidation bugs being discovered.

We also plan to discuss the issue with Dries tomorrow given its potential impacts and disruptions for both the release timeline and the product.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
51.82 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,680 pass(es), 1 fail(s), and 0 exception(s). View

#2554585: Tracker responses are missing a cache tag and a cache context landed, this needed a reroll for that. The patch again became a bit smaller.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
50.02 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,689 pass(es). View
1.84 KB

Oops, I rebased incorrectly. There's still Tracker stuff in #328. Fixed. Should be green again now. Smaller again too.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -615,11 +615,22 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    +    // If the form method is specified in the form, pass it on to FormState.
    +    if (isset($form['#method'])) {
    +      $form_state->setMethod($form['#method']);
    +    }
    

    Do we have a unit test for this particular change?

  2. +++ b/core/modules/page_cache/page_cache.module
    @@ -21,7 +20,8 @@ function page_cache_help($route_name, RouteMatchInterface $route_match) {
    +      $output .= '<dd>' . t('To speed up your site for authenticated users, see the <a href="!smart_cache-help">Smart Cache module</a>.', ['!smart_cache-help' => (\Drupal::moduleHandler()->moduleExists('smart_cache')) ? Url::fromRoute('help.page', ['name' => 'smart_cache'])->toString()  : '#']) . '</p>';
    
    +++ b/core/modules/smart_cache/smart_cache.module
    @@ -0,0 +1,27 @@
    +      $output .= '<p>' . t('The Smart Cache module caches pages in the database, minus the personalized parts. For more information, see the <a href="!smartcache-documentation">online documentation for the Smart Cache module</a>.', ['!smartcache-documentation' => 'https://www.drupal.org/documentation/modules/smart_cache']) . '</p>';
    

    Why do we use ! as placeholder? @ should work the same way. Interesting we kinda use both in core all over the place, so nevermind.

  3. +++ b/core/modules/smart_cache/src/PageCache/ResponsePolicy/DenyAdminRoutes.php
    @@ -0,0 +1,51 @@
    + * This policy rule denies caching of responses generated for admin routes,
    + * because the cacheability metadata of most admin route responses is lacking,
    + * which would lead to stale content being shown and the site being perceived as
    + * broken.
    

    I mean ideally we would fix all those cases as well, right? The other argumentation we might want to follow here is that its not worth to cache admin pages, right?

  4. +++ b/core/modules/smart_cache/src/Tests/SmartCacheIntegrationTest.php
    @@ -0,0 +1,119 @@
    +
    +
    

    You can use one of the new lines below :P

  5. +++ b/core/modules/system/tests/modules/paramconverter_test/src/TestControllers.php
    @@ -25,6 +25,8 @@ public function testNodeSetParent(NodeInterface $node, NodeInterface $parent) {
    -    return ['#markup' => $node->label()];
    +    $build = ['#markup' => $node->label()];
    +    \Drupal::service('renderer')->addCacheableDependency($build, $node);
    +    return $build;
       }
    

    Out of scope of this issue, but just a thought: Could we introduce something like $build['#cache']['dependencies'][], which takes care of those kind of merging?

  6. +++ b/core/profiles/minimal/minimal.info.yml
    @@ -8,5 +8,6 @@ dependencies:
       - dblog
       - page_cache
    +  - smart_cache
     themes:
    
    +++ b/core/profiles/standard/standard.info.yml
    @@ -26,6 +26,7 @@ dependencies:
       - path
       - page_cache
    +  - smart_cache
       - taxonomy
       - dblog
    

    So do we plan to defer the enabling by default, until #2556889: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation has settled but get this in without it? This could have the huge advantage, that contrib/custom code can already deal with it, which is kinda what the other discussion was about, contrib/custom code need awareness.

  7. +++ b/core/profiles/standard/src/Tests/StandardTest.php
    @@ -160,6 +162,30 @@ function testStandard() {
    +    $this->assertEqual('UNCACHEABLE', $this->drupalGetHeader('X-Drupal-SmartCache'), 'Site-wide contact page cannot be cached by SmartCache.');
    +    $url = Url::fromRoute('<front>');
    ...
    +    $this->assertEqual('HIT', $this->drupalGetHeader('X-Drupal-SmartCache'), 'Full node page is cached by SmartCache.');
    +    $url = Url::fromRoute('entity.user.canonical', ['user' => 1]);
    ...
    +    $this->assertFalse($this->drupalGetHeader('X-Drupal-SmartCache'), 'Admin pages cannot be cached by SmartCache.');
    +    $url = Url::fromRoute('system.db_update');
    

    There is always time for an ubernitpick: Do you mind adding new lines after each assert... call for better readability?

Fabianx’s picture

I agree we can definitely get this in and then agree on when its enabled - if its opt-in (smart cache response policy) or opt-out (smart-cache response policy).

Even the safeguards module could have a (configurabl