Move internal page caching to a module to avoid relying on config get on runtime

I'm profiling this from time to time, didn't do so in a while.

Option 1 sounds interesting, but it's one more setting change that requires a container rebuild, which is a heavy operation and still a problem on multi-head environments.

This seems to actually work ;)

Might need to move the container rebuild into a config save event...

Default internal page caching to on, to disable internal page caching there'd still be the following options:

+1, this is what I've been working towards for more than a year, regardless of this particular issue/problem.

This is why we have cache tags and why we have many integration tests verifying that cache tags are being invalidated in alle expected scenarios. All of that is to provide us with a sufficient degree of certainty that we're invalidating all the things that must be invalidated when changing content/config in Drupal, so that we can have a reverse proxy work with Drupal 8 out of the box without pain. And the page cache is the built-in reverse proxy.

Having talked to catch in IRC, he says we also do this for multilingual sites (related to negotiation), and he's ambivalent on whether we should add more container rebuilds (i.e. option 1 in the issue summary, and the one implemented by the patch in #2). He thinks we could also fix the problem attacked by this issue with option 2 from the IS.

And as option 2 as described in the IS already indicates, there are a few consequences to that:

1. Page caching (Drupal's built-in reverse proxy) would be enabled by default.
2. It would be impossible to disable in the UI.
3. It is only possible to disable in code.
4. This is <blink>awesome</blink> from a "Fast by Default" POV.
5. Basically, only complex and high-perf sites with their own Varnish instances will want to disable the page cache, and they'll have to do it in code.

catch and I think that is a fine trade-off.

Now that we have the Page cache as a properly separated out middleware, wouldn't it be possible to simply stick the respective service definition into a module?

That's basically option #2 .3 except reverse: Instead of having a module to turn off page caching, have on to turn it on (and have that module on by default in Standard).

Or are there any downsides to that?

@catch just suggested the same, yes that would be possible, that would avoid the config check and dynamic service, but enabling/disabling page cache would still require to enable/disable a module, which would require a container rebuild. But it is something that might make you a bit more aware of the impact?

tstoeckler++++++++++++++++++++++

Simple, elegant, everybody happy AFAICT.

Not checking this variable at all on runtime, by remove the service dynamically (for example via a module or custom code), has several advantages:

• It makes requests with page cache faster
• It makes requests without page cache faster, Note: you have to initializes a bunch of classes less (I would guess at least 5 in HEAD)

Not sure whether I have an opinion of using a pass vs. a module. The advantage of using a pass is that you can still override the value in your settings.php,
which is afaik less simple in case of a module, as you would have to extend the existing list of modules.

Taking a pass at implementing #5. Berdir, catch and I are all +1 on that idea, let's find out if we find any problems with that.

While working on this, uncovered a security problem: #2421503: SA-CORE-2014-002 forward port only checks internal cache.

Re #8: I would suggest leaving the actual Middleware class in Core and not moving it to the module. That way you can just go crazy in your environment-specific services.yml (or even a custom ServiceProvider for e.g. unregistering the service)

This does probably 90% of the work necessary to move the page cache into a module. All page cache, form cache and cache tags related tests are green, as well as anything that explicitly used the page cache. Hopefully the rest is too.

1. +++ b/core/modules/file/src/Tests/DownloadTest.php
   @@ -56,10 +56,9 @@ public function testPrivateFileTransferWithoutPageCache() {
   +    $this->assertTrue($this->container->get('module_installer')->install(['page_cache']), 'Page cache module installed.');
   

   Why assertTrue()?

2. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
   @@ -247,10 +247,9 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
      protected function enablePageCache() {
        // Turn on page caching and rerun the test.
   -    $config = $this->config('system.performance');
   -    $config->set('cache.page.use_internal', 1);
   -    $config->set('cache.page.max_age', 300);
   -    $config->save();
   +    $this->assertTrue($this->container->get('module_installer')->install(['page_cache']), 'Page cache module installed.');
   +    $this->rebuildContainer();
   +    $this->config('system.performance')->set('cache.page.max_age', 300)->save();
   

   Many diffs are a lot bigger because you change the max_age or response setting to an inline call chain. AFAIK, that is not really correct with our coding standard (when chaining calls, then it should be on separate lines) and it makes the patch bigger.

3. +++ b/core/modules/page_cache/page_cache.module
   @@ -0,0 +1,19 @@
   +/**
   + * Implements hook_form_alter().
   + */
   +function page_cache_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
   +  // If the page that's being built is cacheable, set the 'immutable' flag, to
   

   Should we really move this into an alter hook?

4. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -226,10 +204,7 @@ public function testPageCacheWithoutVaryCookie() {
   +    $config->set('response.gzip', 1)->save();
   

   Should we move this setting to the new module?

5. +++ b/core/profiles/standard/standard.info.yml
   @@ -36,6 +36,7 @@ dependencies:
      - views_ui
      - tour
   +  - page_cache
   

   standard doesn't have the internal cache enabled by default, so we shouldn't enable it here, not yet?

The idea to keep the class in core but enable it in a module is interesting but there are a few questions then. like, where should the gzip setting live then..

standard doesn't have the internal cache enabled by default, so we shouldn't enable it here, not yet?

So I thought that parts of Drupal is still incompatible with page caching ... does that mean we have "missing" test coverage for those parts?

First addressing #15.2 only, to make the patch smaller and make subsequent interdiffs more reviewable.

#15

1. Because:
   

      * @return bool
      *   TRUE if the modules were successfully installed.
   

2. See #18.
3. That's where it belongs, see https://www.drupal.org/node/2242659: Whenever a form is going to be saved to the page cache, it must be updated with the "immutable" flag. This can be done from within hook_form_alter().
   (The code setting the 'immutable' flag originally lived in the first removed hunk of FormCache. But FormCache hardcoded it for the page cache; since it now no longer can do that, hook_form_alter() is the logical new place. At the same time, this serves as a clear example of what other page cache implementations should do.)
4. I'd be fine with that, but IIRC I discussed it with catch and he said we should keep it, because other page cache implementations can then reuse this. It's more of an environment configuration than it is specific to the page cache (even though that's the only consumer). I could be misremembering that though. I'd be fine with moving it if people felt that was better.
5. Right, disabled — I just implemented #5 excitedly :)

#17: Yes, we do have missing test coverage for those parts. Note that all Drupal building blocks generally are now fully compatible with page caching and are varied as appropriate (blocks, all entities, contextual links, "active" links, et cetera). The only remaining problem: Views. Working on that at #2381217: Views should set cache tags on its render arrays, and bubble the output's cache tags to the cache items written to the Views output cache.

EDIT: Do you know of other problems other than Views? Looks like UserPasswordResetTest is the only one being broken by having the page cache enabled.

Tested with PHP 5.5.11, no XDebug, no XHProf, with APC class loader, response.gzip at its default (disabled), best out of 5 ab -c 1 -n 1000 http://tres/ runs. Before doing those 5 test runs, I restarted both Apache and MySQL, to ensure apples-to-apples comparisons.

HEAD

Requests per second:    130.26 [#/sec] (mean)
Time per request:       7.677 [ms] (mean)
Time per request:       7.677 [ms] (mean, across all concurrent requests)
Transfer rate:          1525.09 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     6    8   0.5      8      10
Waiting:        6    7   0.4      7       9
Total:          6    8   0.5      8      10

Patch #19

Requests per second:    166.52 [#/sec] (mean)
Time per request:       6.005 [ms] (mean)
Time per request:       6.005 [ms] (mean, across all concurrent requests)
Transfer rate:          1943.42 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     5    6   0.4      6       8
Waiting:        4    5   0.4      5       8
Total:          5    6   0.4      6       9

… so there is a significant difference: consistently 1.5 to 2 milliseconds faster, or 20% faster.

Yes, should be the same difference as enabling it through $settings, essentially, except that we now don't need that anymore.

It is great to see that the stack architecture pays off now, its good that we fought that fight.

I find really similar numbers.

HEAD

Time taken for tests:   4.933 seconds
Complete requests:      1000
Failed requests:        0
Total transferred:      9392000 bytes
HTML transferred:       8148000 bytes
Requests per second:    202.73 [#/sec] (mean)
Time per request:       4.933 [ms] (mean)
Time per request:       4.933 [ms] (mean, across all concurrent requests)
Transfer rate:          1859.44 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       1
Processing:     4    5   0.5      5      12
Waiting:        4    4   0.4      4      11
Total:          4    5   0.5      5      12

Percentage of the requests served within a certain time (ms)
  50%      5
  66%      5
  75%      5
  80%      5
  90%      5
  95%      5
  98%      6
  99%      7
 100%     12 (longest request)

With patch number 19

Time taken for tests:   3.940 seconds
Complete requests:      1000
Failed requests:        0
Total transferred:      9392000 bytes
HTML transferred:       8148000 bytes
Requests per second:    253.83 [#/sec] (mean)
Time per request:       3.940 [ms] (mean)
Time per request:       3.940 [ms] (mean, across all concurrent requests)
Transfer rate:          2328.07 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       2
Processing:     3    4   0.3      4       7
Waiting:        3    3   0.3      3       6
Total:          3    4   0.3      4       7

Percentage of the requests served within a certain time (ms)
  50%      4
  66%      4
  75%      4
  80%      4
  90%      4
  95%      4
  98%      5
  99%      5
 100%      7 (longest request)

... Some quick run with blackfire shows where the actual win is (https://blackfire.io/profiles/compare/82b668a4-2071-4afb-94bf-5f7f8ebbda...) ...
We initialize much less classes, so its not mainly about the config system being slow but rather the initialization cost. Yet another sign that potentially
we need to work on the lazy issue.

1. +++ b/core/modules/file/src/Tests/DownloadTest.php
   @@ -56,8 +56,9 @@ public function testPrivateFileTransferWithoutPageCache() {
   +    $this->rebuildContainer();
   
   +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
   @@ -247,8 +247,9 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
   +    $this->rebuildContainer();
   

   Are you sure you need this rebuildContainer() calls here? ... doesn't module_installer->install triggers DrupalKernel->updateModules which should also trigger a rebuild?

2. +++ b/core/modules/page_cache/page_cache.module
   @@ -0,0 +1,19 @@
   +// @todo
   +//function page_cache_help() {}
   +
   

   ha

3. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -85,20 +85,8 @@ public function __construct(HttpKernelInterface $http_kernel, CacheBackendInterf
   -    elseif (Settings::get('page_cache_without_database')) {
   -      // Check for a cache mode force from settings.php.
   -      $cache_enabled = TRUE;
   -    }
   
   +++ b/core/modules/system/config/schema/system.schema.yml
   index 9049c78..e4a2de0 100644
   --- a/core/modules/system/src/Form/PerformanceForm.php
   
   --- a/core/modules/system/src/Form/PerformanceForm.php
   +++ b/core/modules/system/src/Form/PerformanceForm.php
   
   +++ b/core/modules/system/src/Form/PerformanceForm.php
   +++ b/core/modules/system/src/Form/PerformanceForm.php
   @@ -135,13 +135,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   
   @@ -135,13 +135,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
          '#description' => t('The maximum time a page can be cached. This is used as the value for max-age in Cache-Control headers.'),
        );
    
   -    $form['caching']['cache'] = array(
   -      '#type' => 'checkbox',
   -      '#title' => t('Use internal page cache'),
   -      '#description' => t("If a reverse proxy cache isn't available, use Drupal's internal cache system to store cached pages."),
   -      '#default_value' => $config->get('cache.page.use_internal'),
   -    );
   -
   

   IMHO it would be great to add some dedicated text onto the form in order to explain how to enable caching. I think having a module won't be obvious for people.

4. +++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
   @@ -488,8 +488,10 @@ public function testSetCacheBuildIdMismatch() {
      /**
       * @covers ::setCache
   +   *
   +   * @todo Move this test to page_cache module.
       */
   -  public function testSetCacheImmutableForm() {
   +  public function atestSetCacheImmutableForm() {
   

   Well that, but don't ignore it ;)

Fixed the failures and addressed #23:

1. Hah, you're right, that's unnecessary. Wonderful! :)
2. Now done :)
3. …hence the importance of enabling it by default, but that's out of scope here. Done.
4. Still keeping that @todo for now, it might be solved by #2421503: SA-CORE-2014-002 forward port only checks internal cache before this issue gets to RTBC.

So that still leaves as TBD:

• #15.4 (see my reply in #19.4)
• #23.4 (see my reply in #25.4)

Re 19.1: Yes, but I think that is a left-over. we throw exceptions now if something goes wrong, we never return FALSE, so asserting that is pointless and just makes the line longer ;)

Discussed #15.4 with catch in IRC, particularly because it prevents performance from being even better, since the page cache even with this patch still requires a config get (for response.gzip).

#8509: store gzipped cache pages added the "Compress cached pages" setting… 11 years ago! There, Dries asked What happens when Apache itself does gzip compression as well? Wouldn't that be a waste of resources?, to which killes answered in #8509-4: store gzipped cache pages: 1) Apache should detect that the output has been compressed already (we set the right header). If it doesn't it is broken.

… that answer was not quite right… and caused #121820: Cache with zlib results in double compression and #13224: apache and drupal both compressing pages, which fixed it by only conditionally doing the gzipping.

An alternative way to support this setting without using the config system, is by storing the value in the container and rebuilding the container whenever the value changes. But this would mean that it's very easy to cause container rebuilds on production sites, which catch strongly dislikes (enabling/disabling modules requires this, but config changes shouldn't, because it removes predictability: catch: it's easy to lock down module installs/uninstalls, less easy to lock down config in general.).

catch and I agreed that the best way forward was to simply remove the "Compress cached pages"/response.gzip, because that avoids all problems, it always works. No need for a confusing setting anymore (yay, enhanced usability!). If you want better performance, install the advanced_page_cache module from contrib or enable gzipping in your web server.

Also fixed #27.

catch and I agreed that the best way forward was to simply remove the "Compress cached pages"/response.gzip, because that avoids all problems, it always works. No need for a confusing setting anymore (yay, enhanced usability!). If you want better performance, install the advanced_page_cache module from contrib or enable gzipping in your web server.

Damn! That should have been done years ago! Yay.

+1 for removal of the gzip option.

Just to be clear, deploying new language entities / change the default language already requires a container rebuilt, but I agree
we should avoid them, if possible.

Especially for things like page cache, people might just turn it on, without thinking about the consequences. Having a
dedicated module communicates it really good, that its not just a boolean flag you check in the UI.

I think this is a good change, as a big WTF is why some pages are uncompressed (those not in the page cache), but others are ...

So if you want compressed pages, you end up with mod_deflate or something like that anyway ...

Alright, seems we're all in agreement. We'll need some reviews to get this to RTBC :)

I noticed in @dawehner's blackfire.io link that the query count went from 5 to 4. That's the cache get for the system.performance config.

The cache get here should only require two queries (the second for cache tags), so that made me wonder what the other two were. I'm fairly sure that counts the two "SET queries" that we do in mysql\Connnection::open(). So that is fine, in case someone else notices that as well.

(The only problem with that is that they are already executed when the connection services is requested, in Database::openConnection(), which violates the principle to not do anything costly in your constructor/service init, but the connection/database service is a dark, dark corner in our fancy services configuration ;)

Revamped the IS, because the issue went a different route than the original options indicated.

To fix the LanguageNegotiator todo, which is the trickiest one to resolve, I had to do some serious archeology work to find out how exactly its integration with the page cache was intended to work, because there's no test coverage nor documentation explaining it. Having talked to @Gábor Hojtsy, he agrees it's confusing at best, wrong at worst.

This is the offending/difficult to resolve snippet of code:

      // Check for a cache mode force from settings.php.
      if ($this->settings->get('page_cache_without_database')) {
        $cache_enabled = TRUE;
      }
      else {
        $cache_enabled = $this->configFactory->get('system.performance')->get('cache.page.use_internal');
      }

      // If the language negotiation method has no cache preference or this is
      // satisfied we can execute the callback.
      if ($cache = !isset($method['cache']) || $this->currentUser->isAuthenticated() || $method['cache'] == $cache_enabled) {
        $langcode = $this->getNegotiationMethodInstance($method_id)->getLangcode($this->requestStack->getCurrentRequest());
      }

And then particularly, that if-test.

So, looking for its roots, we see it was introduced by #1862202: Objectify the language system (commit 0b55dcd8). But that was just modernizing existing code. So we dig further:

$ git blame -L474,+14 0b55dcd8^ -- core/includes/language.inc
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 474)     // Check for a cache mode force from settings.php.
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 475)     if (settings()->get('page_cache_without_database')) {
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 476)       $cache_enabled = TRUE;
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 477)     }
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 478)     else {
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 479)       drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES, FALSE);
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 480)       $config = \Drupal::config('system.performance');
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 481)       $cache_enabled = $config->get('cache.page.use_internal');
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 482)     }
661488af (Dries               2012-03-05 07:28:36 -0500 483)     // If the language negotiation method has no cache preference or this is
661488af (Dries               2012-03-05 07:28:36 -0500 484)     // satisfied we can execute the callback.
ea41ae25 (Nathaniel Catchpole 2013-10-07 12:09:58 +0100 485)     $cache = !isset($method['cache']) || $user->isAuthenticated() || $method['cache'] == $cache_enabled;
661488af (Dries               2012-03-05 07:28:36 -0500 486)     $callback = isset($method['callbacks']['negotiation']) ? $method['callbacks']['negotiation'] : FALSE;
b979e8aa (Katherine Bailey    2012-08-06 11:59:53 -0700 487)     $langcode = $cache && function_exists($callback) ? $callback($languages, $request) : FALSE;

ea41ae25 is just updating things from variable_get() to the config system. 661488af is the one we want, and corresponds to #1222106: Unify language negotiation APIs, declutter terminology. But there, we also find that it was just reorganizing code. Digging further:

$ git blame -L376,+6 661488af^ -- core/includes/language.inc
af3f94b3 includes/language.inc      (Dries Buytaert 2010-01-25 10:38:35 +0000 376)     // If the language provider has no cache preference or this is satisfied
1b9cde9d includes/language.inc      (Angie Byron    2009-10-09 16:33:14 +0000 377)     // we can execute the callback.
a3156a15 core/includes/language.inc (justinrandell  2012-02-11 23:01:58 -0500 378)     $config = config('system.performance');
a3156a15 core/includes/language.inc (justinrandell  2012-02-11 23:01:58 -0500 379)     $cache = !isset($provider['cache']) || $user->uid || $provider['cache'] == $config->get('cache');
1b9cde9d includes/language.inc      (Angie Byron    2009-10-09 16:33:14 +0000 380)     $callback = isset($provider['callbacks']['language']) ? $provider['callbacks']['language'] : FALSE;
1b9cde9d includes/language.inc      (Angie Byron    2009-10-09 16:33:14 +0000 381)     $langcode = $cache && function_exists($callback) ? $callback($languages) : FALSE;

Looks like a3156a15 is what we want, but alas, it's only converting variable_get()s to the config system. So… more time travel:

$ git blame -L344,+6 a3156a15^ -- core/includes/language.inc
af3f94b3 includes/language.inc (Dries Buytaert 2010-01-25 10:38:35 +0000 344)     // If the language provider has no cache preference or this is satisfied
1b9cde9d includes/language.inc (Angie Byron    2009-10-09 16:33:14 +0000 345)     // we can execute the callback.
47653eae includes/language.inc (Dries Buytaert 2010-05-12 08:26:15 +0000 346)     $cache = !isset($provider['cache']) || $user->uid || $provider['cache'] == variable_get('cache', 0);
1b9cde9d includes/language.inc (Angie Byron    2009-10-09 16:33:14 +0000 347)     $callback = isset($provider['callbacks']['language']) ? $provider['callbacks']['language'] : FALSE;
1b9cde9d includes/language.inc (Angie Byron    2009-10-09 16:33:14 +0000 348)     $langcode = $cache && function_exists($callback) ? $callback($languages) : FALSE;
1b9cde9d includes/language.inc (Angie Byron    2009-10-09 16:33:14 +0000 349)     $results[$provider_id] = isset($languages[$langcode]) ? $languages[$langcode] : FALSE;

Which leads to 1b9cde9d (#282191: TF #1: Allow different interface language for the same path)… which finally is the one that introduced this:

// If the language provider has no cache preference or this is satisified
// we can execute the callback.
$cache = !isset($provider['cache']) || $user->uid || $provider['cache'] == variable_get('cache', CACHE_DISABLED);
$callback = isset($provider['callbacks']['language']) ? $provider['callbacks']['language'] : FALSE;
$langcode = $cache && function_exists($callback) ? $callback($languages) : FALSE;
$results[$provider_id] = isset($languages[$langcode]) ? $languages[$langcode] : FALSE;

As you can see, the code is basically unchanged since then. Unfortunately, no discussion was had about this particular part of that patch whatsoever. Either it was overlooked, or everybody agreed. But at least it has a bit of docs — citing @plach in #1222106-29: Unify language negotiation APIs, declutter terminology:

 *   - "cache": The value Drupal's page cache should be set to for the current
 *     language negotiation method to be run.

And just like in HEAD, it's only LanguageNegotiationBrowser that has cache = 0 in its plugin annotation, that code had the equivalent locale_language_from_browser() function with these docs:

+/**
+ * Identify language from the Accept-language HTTP header we got.
+ *
+ * We perform browser accept-language parsing only if page cache is disabled,
+ * otherwise we would cache a user-specific preference.

So… the entire reason this was incompatible with the page cache, is:

1. a request header (Accept-Language) being used, which can very from request to request
2. the page cache not distinguishing between different languages, hence potentially serving the wrong response (language A) to a request (requesting language B or C)

In other words, we can remove the need to check for the page cache if the page cache's cache ID varied depending on the Accept-Language request header.

Trying to find issues related to that, I stumbled upon:

#2257843: Browser language detection is skipped for anonymous users when page caching is turned on, with no warning to the site administrator

The same code as above is being cited/blamed for a browser's preferred language not being detected by Drupal when the page cache is enabled, to which Damien Tournoud says this in #3:

This code prevents language negotiation when the page cache is enabled. It gives the language negotiation methods the opportunity to say:

• "I don't care about the page cache" ('cache' not set or NULL)
• "I can only work with page cache disabled" ('cache' => 0)
• or "I can only work with page cache enabled" ('cache' => 1)

Browser-based language negotiation is incompatible with the page cache, at least in Drupal 7 (because the page cache doesn't take the language into account when caching or serving pages). So this code is working as intended.

Hence further confirming that this (bug) is indeed the problem.

#1855260: Page caching broken by accept header-based routing

Where we added support for the Accept header to the page cache, by including the negotiated content type (HTML, JSON …) in the page cache ID

The code to support this

Looking at \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationBrowser(), we could add something like this to PageCache::getCacheId():

    $http_accept_language = $request->server->get('HTTP_ACCEPT_LANGUAGE');
    if ($http_accept_language) {
      $default_langcode = $this->languageManger->getDefaultLanguage();
      if ($this->languageManger->isMultilingual()) {
        $langcodes = array_keys($this->languageManager->getLanguages());
        $mappings = $this->config->get('language.mappings')->get();
        $langcode = UserAgent::getBestMatchingLangcode($http_accept_language, $langcodes, $mappings);
        if ($langcode !== FALSE) {
          $langcode = $default_langcode;
        }
      }
      else {
        $langcode = $default_langcode;
      }
      $cid_parts[] = $langcode;
    }

(Where $cid_parts are the parts that will make up the cache ID.)

The problem

The only problem with that: that uses a service that in turn uses many other services, including config. That's not an option.

The solution

Fortunately, DrupalKernel::compileContainer() (for single-language sites) and LanguageServiceProvider::alter() (for multilingual sites) already provide an example: they alter the container and set the language.default_values parameter. If we do exactly the same for the additional data that we need (whether the site is multilingual, the supported languages and the language mappings), then we don't introduce a new container rebuild, but we do get rid of all these dependencies: we'll be able to get exactly the data we need at no cost!

So, with that done:

Patch, single language (not multilingual), no Accept-Language header ($ ab -c 1 -n 1000 -H http://tres/)

Requests per second:    164.68 [#/sec] (mean)
Time per request:       6.072 [ms] (mean)
Time per request:       6.072 [ms] (mean, across all concurrent requests)
Transfer rate:          1498.98 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     5    6   0.4      6       9
Waiting:        4    5   0.4      5       8
Total:          5    6   0.4      6       9

Patch, two languages (multilingual), with a relatively complex Accept-Language header ($ ab -c 1 -n 1000 -H 'Accept-Language: hu, en-us;q=0.66, en;q=0.33' http://tres/

Requests per second:    163.21 [#/sec] (mean)
Time per request:       6.127 [ms] (mean)
Time per request:       6.127 [ms] (mean, across all concurrent requests)
Transfer rate:          1485.65 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     5    6   0.5      6       8
Waiting:        4    5   0.5      5       8
Total:          5    6   0.5      6       8

As you can tell, the cost of doing this, compared to the benchmarks I did in #20, is lower than the margin of error.

I'd go as far and remove support for any content negotiation (including language negotiation) from the internal page cache. We could leave that up to advanced page cache contrib module.

#41: the only reason I spent my entire day on #40 is that LanguageNegotiator currently hardwires logic to detect whether the page cache is enabled or not. So, to not break that, #40 is the only way out that I see. If we'd do #41, then we'd also have to remove \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationBrowser() from core.

Great work @Wim Leers. Some first level review comments.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -980,6 +980,24 @@ protected function compileContainer() {
   +    $container->register('class_loader')->setSynthetic(TRUE);
   +    $container->register('kernel', 'Symfony\Component\HttpKernel\KernelInterface')->setSynthetic(TRUE);
   +    $container->register('service_container', 'Symfony\Component\DependencyInjection\ContainerInterface')->setSynthetic(TRUE);
   

   Seeing it for the first time. Quick google brings http://symfony.com/doc/current/components/dependency_injection/advanced..... Would it worth updating some place that we are using this? (coding standard/pattern etc). Also adding a link to above URL would help?

   BTW, I can see it is just a code move. Worth adding doc, if it's OK.

2. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -35,6 +38,10 @@ protected function setUp() {
   +    $config = $this->config('system.performance');
   

   may be we have to move this config as well to page_cache

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
   @@ -488,8 +488,10 @@ public function testSetCacheBuildIdMismatch() {
   +  public function atestSetCacheImmutableForm() {
   

   typo

#43

1. I actually didn't move that code, that's just git being silly. I moved the bits that are setting the language.default_values container parameter.
2. No, see FinishResponseSubscriber — this is not used only for the page cache, it is used for all cacheable pages. It's just a setting for the Cache-Control header that we set on all cacheable pages. (I thought the same at first though, but it's definitely something that's not specific to the page cache.)
3. No, that's the other remaining @todo just above it :) It's commented so that we don't forget to actually move it. I'll do that next.

Consider the following scenario (also see attached screenshots):

• A site with three languages (English, Dutch, French in this order, first is default).
• Content translation enabled
• Browser language selection is enabled
• node/1 is available in English only
• node/2 is available in English and French
• node/3 is available in English and Dutch
• node/4 is available in English, Dutch and French
• Node titles are translated into the respective language, such that we can grep for the <title> tag

With the following shell script:

BASE_URL=http://localhost:3000/
HEADERS="en fr nl fr;q=1,en;q=0.5 nl;q=1,fr;q=0.5 fr;q=1,nl;q=0.5"

for H in $HEADERS; do
    echo "Testing: $H"
    wget -qO - --header "Accept-Language: $H" $BASE_URL/node/1 | grep '<title>'
    wget -qO - --header "Accept-Language: $H" $BASE_URL/node/2 | grep '<title>'
    wget -qO - --header "Accept-Language: $H" $BASE_URL/node/3 | grep '<title>'
    wget -qO - --header "Accept-Language: $H" $BASE_URL/node/4 | grep '<title>'
done

This produces the following output for me:

Testing: en
    <title>n1 english only | Site-Install</title>
    <title>n2 in english | Site-Install</title>
    <title>n3 in english | Site-Install</title>
    <title>n4 in english | Site-Install</title>
Testing: fr
    <title>n1 english only | Site-Install</title>
    <title>n2 in french | Site-Install</title>
    <title>n3 in english | Site-Install</title>
    <title>n4 in french | Site-Install</title>
Testing: nl
    <title>n1 english only | Site-Install</title>
    <title>n2 in english | Site-Install</title>
    <title>n3 in dutch | Site-Install</title>
    <title>n4 in dutch | Site-Install</title>
Testing: fr;q=1,en;q=0.5
    <title>n1 english only | Site-Install</title>
    <title>n2 in french | Site-Install</title>
    <title>n3 in english | Site-Install</title>
    <title>n4 in french | Site-Install</title>
Testing: nl;q=1,fr;q=0.5
    <title>n1 english only | Site-Install</title>
    <title>n2 in english | Site-Install</title>
    <title>n3 in dutch | Site-Install</title>
    <title>n4 in dutch | Site-Install</title>
Testing: fr;q=1,nl;q=0.5
    <title>n1 english only | Site-Install</title>
    <title>n2 in french | Site-Install</title>
    <title>n3 in english | Site-Install</title>
    <title>n4 in french | Site-Install</title>

When the client only supports a single language, the results are correct. However, if the browser is configured with two languages other than the the default language, things are subtly broken:

1. node/2 is served to a nl/fr client in english even though it is available in French
2. node/3 is served to a fr/nl client in english even though it is available in Dutch

This behavior is caused by the fact that negotiation of the language does not take into account the language versions which are available on the current route but simply uses the list of site-wide available languages. Maybe D8MI people may decide that this is a bug at some point, and that's when the page cache will break in the same manner as it is currently broken for Accept based content negotiation.

Note that it is also possible to use different negotiation methods for the content language versus the interface language. I do not yet understand the effects of that option on a language-aware page cache like #40, but I suspect there is room for even more cache poisoning/mix-up issues.

Regrettably we are still far away from a proper implementation of content-negotiation as well as language-negotiation. Therefore I think it might be better to simply declare that browser based language negotiation is incompatible with the internal page cache.

Also if I'm not completely wrong, using Accept-Language is also affected by #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use

For clarity: I cannot reproduce #46.1 when unit testing the logic powering it:

$ drush php
[1] self> \Drupal\Component\Utility\UserAgent::getBestMatchingLangcode('nl;q=1,fr;q=0.5', ['en', 'fr'], []);
// 'fr'

… but I realize that's precisely for the reason #46 cites: content translation does show the problem you describe.

But… that's actually because of a bug in content translation, that Customize Content language detection to differ from User interface text language detection settings checkbox at /admin/config/regional/language/detection, when left unchecked, says the content language will just be the interface language. But that doesn't actually work. (Reported to Gábor Hojtsy, and he helped me find what the problem was.)
If you just *check* that checkbox and configure it in the same way (i.e. also check the "Browser" checkbox for "Content language detection" at /admin/config/regional/language/detection), then it actually works as expected :)

Proof (with attached patch applied, that adds some more debug output that shows up when using @znerol's test script):

$ wget -qO - --header "Accept-Language: nl;q=1,fr;q=0.5" http://tres/node/1 | grep '<title>'
<title>language_interface nl</title>string(12) "language_url"
<title>language_url en</title>string(16) "language_content"
<title>language_content nl</title><!DOCTYPE html>
    <title>n2 in english | tres</title>

Except it still doesn't take into account the available translations for that specific entity. So it negotiated to Dutch, which is indeed an available language, but it's NOT an available translation. AFAIK there is no way to make that work efficiently.

(Gábor filed #2424171: Language module vs. content translation module interaction exposes content translation bug, which is for solving *that* bug.)

In IRC, catch said this:

catch: WimLeers: I think we should probably disable the browser negotiation like 7.x did though to be honest.
catch: WimLeers: because this won't work at all for reverse proxies and we still support both internal cache + proxies.
WimLeers: catch: by that, you mean that Accept-Language doesn't work well with reverse proxies in general, so we shouldn't do it anyway?
catch: WimLeers: yes.

Further confirming what #41 said.

IMHO, that leaves only one option: removing LanguageNegotiationBrowser altogether (see #42 for details), and catch agrees:

WimLeers: catch: but then we should imo remove browser negotiation
catch: WimLeers: that'd be fine with me too.
catch: It was added in 6.x, and was broken.
catch: And that never got fully resolved.

So… rerolling this patch, reverting almost everything from #40, and moving the container parameters work I did there into a separate issue, because that's going to be useful to contrib in general.

Before actually deleting LanguageNegotiationBrowser, I'm only deleting the truly problematic bits, so we can get feedback from Gábor Hojtsy before actually deleting it:

1. the cache = 0 annotation in the browser negotiation method
2. the support for that cache key on language negotiation methods, because its docs (missing from HEAD, see #40) say The value Drupal's page cache should be set to for the current language negotiation method to be run. — but it's impossible to know if 1) a non-Drupal core page cache is active, 2) whether Varnish is in front of this Drupal instance, which has exactly the same effect

The problem is that it then has the ability to poison the page cache, because the page cache doesn't key based on the Accept-Language header, which #46 shows can't actually work: it's currently broken because of a bug (the list of available translations isn't checked when doing browser-based content language negotiation), but when that is fixed, the page cache cannot possibly begin to .

Ironically, this means that the previous patches actually work fine with page cache/reverse proxies… but because the "content language" concept exists (instead of only interface language), we have a unique problem: we should check available translations for it to make sense (and hence also in the page cache when building the CID, which would be prohibitively expensive), but we don't, and hence it actually works fine with even reverse proxies like Varnish (Varnish could never ever check available translations). However, catch and znerol prefer this to be removed entirely, and I understand that POV too.

The LanguageNegotiationBrowser class, which allows for Accept-Language header negotiation is removed, because it is incompatible with page caching, has never supported alternative page caches or reverse proxies, and is impossible to make work with content translation. (We could keep it, but then with that long list of caveats.) In contrib, this problem space can be fully fleshed out; we need to ship D8.

I uploaded the right interdiff but wrong patch.

We also don't cache when in a session, why not disable caching when using the browser language negotiation method? Ideally a SEO friendly browser negotiator would sit on a fallback URL and redirect to a specific URL for the negotiated language to support caching (only the fallback URLs would be exempt from caching).

The browser language negotiation feature is what we use to detect your preferred language in the Drupal installer too, so you get the least interaction to get the desired site, so it would be painful to need to remove it now.

That /fr/node/1 may give you an English node is already true if using the language switcher block, so I don't think that's news. I would be fine with browser detection always redirecting, it would be a behavior change but its for the better for performance. Those who need it to not redirect will need to take the performance killing effect and use a contrib solution for it.

The patch looks RTBC to me.

I think we can re-factor / remove current language/content negotiation in a follow-up with more discussion.

Currently page_cache is just incompatible with language / content neg, but so is Varnish or any other cache, so I don't think this needs to be done as part of this task.

@catch: the interface translation switcher block certainly should not check for content language. That is the only switcher available until you configure the content language negotiation separately and it does switch the interface language, so it does not necessarily have a reason to deny linking to a specific language. ConfigurableLanguageManager::getLanguageSwitchLinks() does not seem to do anything to filter the languages (not LanguageNegotiationUrl::getLanguageSwitchLinks()). ConfigurableLanguageManager::getLanguageSwitchLinks() does support a language_switch_links_alter but that does not seem to be implemented. So there does not seem to be code to filter the language list on a quick look. Many pages like views, panels, etc. make it impossible to tell which components would be available in the target language anyway up front.

1. Replaced with something better.
2. Indeed.
3. :) Now fixed, which was the last remaining task here.

Testbots were crashing, that's now fixed. Now this is properly being tested. Updating status accordingly.

I cannot reproduce any of the test failures. Testbot wonkiness?

+++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
@@ -218,50 +197,36 @@ public function testPageCacheWithoutVaryCookie() {
+      // Mock a request.
+      $request = Request::create('/');
+      $request->setMethod($request_method);
+      $this->container->get('request_stack')->push($request);

My guess is this is because testbot runs in a subfolder, so it needs to be Request::create('/checkout') on testbot (something dynamically actually. obviously)

#66: thank you, that is super helpful!

Opened #2430335: Browser language detection is not cache aware based on @catch in #52.

So until #2430335: Browser language detection is not cache aware is resolved, looks like this issue needs to make the browser language detector add a 'no-cache' directive to the page when it detects a language (as opposed to passing over to the next negotiation method).

Which simply means that we need to call the kill switch page cache response policy thing I think?

I think this is not blocked on browser language detection.

If you want to use that, don't use the page_cache module. Just need to make clear that page_cache module is limited in core and most sites with more complex needs, need to use a different (contrib) one or varnish directly.

To quote myself:

"Currently page_cache is just incompatible with language / content neg, but so is Varnish or any other cache, so I don't think this needs to be done as part of this task."

I think it is perfectly fine to use, *as a fallback* to url/domain or some other reliable method that works with page cache, just like it already was in 7.x I think :)

So we just add the kill switch here, and the other issue is then about making that clear to the user, validating it and so on...

I don't think its a big deal to set no caching from the browser language negotiation. Tribal knowledge that the two modules will not work together vs. actual explicit code are two very different levels. If you are concerned that a slow site is worse than poisoned cache entries, we can also try to alert people to possibly disastrous combinations of configuration for performance implications. Before this path, if you enabled browser language negotiation, it would silently not execute at all if page caching was on. Now it executes and poisons your cache. I think it would be better if it would silently make your site slower (not cached) vs. silently poisoning the cache instead. Sounds like its a matter of a couple lines of code and a test, no?

#73: That sounds good and if we move browser language neg. to make a redirect, we can also cache it again.

But yes providing page cache with the information that this page is not cacheable is fine.

FWIW, it might be cacheable soon. :)

Negotiated language in reality is a cache_context and once that parametrized cache context is set (different from language), the page could be cached in different ways depending on the configured context.

Again a two-tier approach (where the page cache just works later after content neg was done) would work well for such cache entries - compare SmartCache and context bubbling in the other issues.

But that is up for future discussion, we can kill the cache for now in language neg.

I think the redirection will likely need to not be cached either, unless you want to cache that by the myriads of whatever possible accept language headers (or what we derive from them which is also probably infinite variability :) so I don't think the code to write to kill the cache would be in vain (as in not temporary)?

FYI, this is what I recommend to Authcache users struggling with multilingual sites: #2348155-12: Integrate with locale cookie

@znerol: it sounds good to fix Drupal to only URL based negotiation and let redirection happen in a different layer on the stack, depending on how you use Drupal, you may not have access to any of the other layers of the stack. That is why we include a page cache feature, a search module, etc. Obviously more serious sites will not use these and instead rely on other components in their stack.

#75:

Unless that has changed with response objects, I don't think the page cache ever cached a redirect, so we should be fine there.

We do cache more or less any response, (see also #2429671: "The website has encountered an error. Please try again later." page is cached).

If you have a distinct prefix/domain for each language, then it would be easy to write a page cache request policy rule that will prevent caching of unprefixed pages. However, I usually try to avoid a bootstrap for trivial redirects (example.com -> example.com/en). If they are slow, that will affect the first impression.

@znerol: right, thats a great policy as long as you have control of your stack other than Drupal or you don't need redirection or implement/hack it in JS on those unprefixed pages (which is then cacheable). I think Drupal 8 still wants to support people who don't have control over more than Drupal in their stack (eg. they are on a Drupal Gardens like hosted service).

Gábor Hojtsy: The language module could actually provide such a request policy rule. The tools are there. If the language module wants to provide a feature which does negotiate/redirect for new users on unprefixed pages, then this can be built right now - including automatic integration with internal and external page caching.

@znerol: where are similar request policy rules provided by core now?

For example in core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php, tests in core/tests/Drupal/Tests/Core/PageCache/CommandLineOrUnsafeMethodTest.php.

The important thing is to keep dependencies lightweight, otherwise this will kill page cache performance.

@znerol: not sure that would be very performant, given that policy rule would need to depend on the language manager (language list), the negotiators (figure out path settings, etc) and slowing down all cached pages to figure that out sounds like a problem to me. I'd rather tell the page after the fact that it should not be cached when it was an unprefixed / redirect path and assume it can be cached otherwise. Are there problems with that?

Are there problems with that?

That works too, then it simple needs to be implemented as a response rule. The only caveat is that it will always generate a cache round-trip (in fact a MISS) if you implement it as a response rule. But that's for sure the lesser of two evils.

#84: together with #2424309: Expose more language container parameters, to allow services to be language-aware efficiently, it could easily be performant — just inject the language.is_multingual container parameter to your new request policy and check that with a simple (cheap!) if-test.

Not sure why we are keeping talking about what would be in implemented in #2430335: Browser language detection is not cache aware on this issue, unless you want to skip offloading that to that issue and want to resolve it here? I thought you want to avoid dealing with that problem here, but given your interest, I am not sure anymore?

BTW #2424309: Expose more language container parameters, to allow services to be language-aware efficiently does not include data on how the negotiation is configured, which is essential to be able to tell if there is any language without a prefix, if there would be a redirection on certain pages, etc. so not sure how would that resolve it, unless we put all language module config on the container (which that issue did not propose so far) and if we reimplement part of the negotiation logic in the request policy. Neither of these were supposed to be in scope for this issue, no?

The only reason I wrote #86 is because you said this in #84:

[…] given that policy rule would need to depend on the language manager (language list) […]

I'm only refuting that by pointing to a solution around the expensiveness that you pointed at.

Sorry for any confusion.

first a reroll

here a super easy first version. It disables the internal page cache when browser negotiation is used.

Some todos and thoughts:

1. needs tests
2. if browser negotiation is used after url negotiation, then this is not necessary, but we need to redirect from a non path prefix to a path prefix, this will be handled in #2430335: Browser language detection is not cache aware

and here a try to fix the testbot issue.

Looks like the fails is because of the DefaultRequestPolicy is checking if !cli and testbot running in CLI? Other option is to add a requestPolicy as part of the page_cache_form_test module and override core's page_cache_request_policy service?

Thanks @dawehner for the help over IRC to write the test.

If it works, we can bit closer by adding another Policy without CLI (i.e. just UnsafeMethod from CommandLineOrUnsafeMethod policy).

hmm, no change in response. But the size of files in #94 and #98 are different (83k vs 38k) because of rename like core/{lib/Drupal/Core => modules/page_cache/src}/StackMiddleware/PageCache.php in #98 which is good.

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -40,6 +40,10 @@ public function getLangcode(Request $request = NULL) {
+      // could lead to wrong cached sites. Therefore disabling the internal
+      // page cache.
+      \Drupal::service('page_cache_kill_switch')->trigger();

Would be nice to add a @todo reference to #2430335: Browser language detection is not cache aware.

Just a reroll.

Fixing some tests, cleanup.

Ok, I don't see a way to make that test work like that. page cache doesn't work by design on CLI.

If we want to test the form alter there directly, then I think we need a real route that displays the form, and then compare the cached form.

If we want to test the form alter there directly, then I think we need a real route that displays the form, and then compare the cached form.

done in my patch

green!!! yay!!!

1 minor issue:

+++ b/core/modules/page_cache/tests/modules/page_cache_form_test.module
@@ -0,0 +1,20 @@
\ No newline at end of file

+++ b/core/modules/page_cache/tests/modules/page_cache_form_test.routing.yml
@@ -0,0 +1,6 @@
\ No newline at end of file

Missing empty line at the end.

@vijaycs85
somehow only the interdiff has the "\ No newline at end of file" errors, not sure why, but they are not in the main patch :)

+++ b/core/modules/page_cache/tests/modules/page_cache_form_test.module
@@ -0,0 +1,20 @@
+ * @file
+ * Provides functionality for testing form caching

missing . ;)

The alter alter seems a bit tricky ( it depends on the order of alter hooks), maybe we can make the module weight explicit and add a comment why/how it works in the test, in case someone ends up debugging that when something breaks it.

Nice idea, though.

+++ b/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php
@@ -26,12 +26,16 @@
+  protected static $modules = ['page_cache'];
+
...
     // Enable page caching.
     $config = $this->config('system.performance');
-    $config->set('cache.page.use_internal', 1);
     $config->set('cache.page.max_age', 3600);
     $config->save();

I might be wrong about inheritance, but I am 70% sure that this won't fly and only works due to a side effect somewhere.

Because e.g.:

abstract class EntityCacheTagsTestBase extends PageCacheTagsTestBase {

  /**
   * Modules to enable.
   *
   * @var array
   */
  public static $modules = array('entity_reference', 'entity_test', 'field_test');

}

so no page_cache in here.

I think whenever we setup the page cache explicitly, we should enable the module, not just add to static $modules, which does not seem to work for inheritance - or I will be very surprised. Hm, maybe it does via some magic?

Anyway, can someone explain that it either inherits static properties or needs to be fixed?

EDIT:

Berdir explained in IRC that static $modules are indeed merged together by the TestBase. So this is not an issue.

As discussed in IRC, that does work fine and is explicitly supported in TestBase.

(needs work also for the @todo mentioned in #101)

will do #101 and #110

This issue will likely save 1-2 ms due to the not needed call to $config, which uses container::get which needs something else, etc.

Does this need some profiling, it's quite a change at this point if there isn't a noticeable benefit? Although I do like the idea of moving it out into its own module.

Sure, I am quite sure we see the same 1-2 ms saved with that :) (bringing us down to around 5 ms - which is quite close to the 2 ms of D7), which is nice!

Edit to clarify, the 1-2 ms I saw before was from profiling the page cache and just looking at the config() call.

#606840: Enable internal page cache by default has now landed. This will need a reroll. This will also need to remove the config/install/system.performance.yml files added to the standard/minimal/testing install profiles and instead add the page_cache module as a dependency of those install profiles.

The issue summary of #1744302: [meta] Resolve known performance regressions in Drupal 8 lists "Over ~1ms or more with the internal page cache and would have to be deferred to 9.x" as sufficient criteria for an issue to be Critical. I don't think this would need to be deferred to 9.x, so not raising priority, but I do think numbers like #117 would easily justify this still being in scope for 8.0.

Tagging "D8 upgrade path" per #2455949-7: [policy, no patch] Re-activate the head2head project and use it for beta2beta upgrades in the short-term.

Made a child of known performance regressions and yes this should be able to go in easily based on the removed dependency on config() and therefore container.

FYI: on my machine (PHP 5.5, no APCu class loader, ab -c 1 -n 1000 URL, 2.8 GHz Intel Core i7), without this patch (commit 87ade2e6d6cf1e9724e7d9e88246760b19967934):

Drupal 7

2.5 ms/request

400 requests/s

Drupal 8

8.3 ms/request

120 requests/s

So if this helps reduce it to, say, 6 ms, that'd already be 166 requests/s. (5 ms = 200 reqs/s. 4 ms = 250 reqs/s, etc.)

Every millisecond is worth many requests per second here.

Reroll.

- Not visible in the interdiff, moved/kept the kill switch inside the if, but kept the todo added by the other issue.
- added page_cache module dependeny, removed the system.performance.yml overrides.
- Updated a few new uses of the use_internal setting. kept the max_age, but I really think that's useless in most of those tests, we kind of know that now as most pages end up with a max_page: 0, private ;)
- I found quite a few tests that still attempt to enable page caching (before with the setting, now by enabling the module), but that's the default now. Quite a few of those are attempting to test both with and without page cache. I've removed a few of those. If we really want to bring it back, then we should instead explicitly disable page cache there. But I'm not sure what that would bring us?

The test failure has been resolved.

Great progress!

1. diff --git a/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php b/core/modules/page_cache/src/Tests/PageCacheTest.php
   similarity index 78%
   rename from core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
   rename to core/modules/page_cache/src/Tests/PageCacheTest.php
   index 610c9b1..ed27394 100644
   --- a/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
   +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   [...]
   @@ -38,6 +42,20 @@ protected function setUp() {
          ->set('name', 'Drupal')
          ->set('page.front', 'test-page')
          ->save();
   +
   +    $config = $this->config('system.performance');
   +    $config->set('cache.page.max_age', 300);
   +    $config->save();
   +
   +    // As the DefaultRequestPolicy deny cache for CLI requests,
   +    // build request policy without CLI check.
   +    $this->container->set('page_cache_request_policy', NULL);
   +    /** @var  \Drupal\Core\Session\SessionConfigurationInterface $session_configuration */
   +    $session_configuration = $this->container->get('session_configuration');
   +    /** @var \Drupal\Core\PageCache\RequestPolicyInterface $request_policy */
   +    $request_policy = new ChainRequestPolicy();
   +    $request_policy->addPolicy(new NoSessionOpen($session_configuration));
   +    $this->container->set('page_cache_request_policy', $request_policy);
      }
    
      /**
   

   Introduced in #97 , but that is actually wrong. The policy is executed in the simpletest child site (this is still a WebTestCase) and that clearly is not CLI. Remove this hunk completely.

2. diff --git a/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php b/core/modules/page_cache/src/Tests/PageCacheTest.php
   similarity index 78%
   rename from core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
   rename to core/modules/page_cache/src/Tests/PageCacheTest.php
   index 610c9b1..ed27394 100644
   --- a/core/modules/system/src/Tests/Bootstrap/PageCacheTest.php
   +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -196,16 +199,9 @@ function testConditionalRequests() {
       [...]
        // Fill the cache.
        $this->drupalGet('system-test/set-header', array('query' => array('name' => 'Foo', 'value' => 'bar')));
        $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', 'Page was not cached.');
   -    $this->assertEqual(strtolower($this->drupalGetHeader('Vary')), 'cookie,accept-encoding', 'Vary header was sent.');
        // Symfony's Response logic determines a specific order for the subvalues
        // of the Cache-Control header, even if they are explicitly passed in to
        // the response header bag in a different order.
   @@ -216,7 +212,6 @@ function testPageCache() {
        // Check cache.
        $this->drupalGet('system-test/set-header', array('query' => array('name' => 'Foo', 'value' => 'bar')));
        $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', 'Page was cached.');
   -    $this->assertEqual(strtolower($this->drupalGetHeader('Vary')), 'cookie,accept-encoding', 'Vary: Cookie header was sent.');
        $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'max-age=300, public', 'Cache-Control header was sent.');
        $this->assertEqual($this->drupalGetHeader('Expires'), 'Sun, 19 Nov 1978 05:00:00 GMT', 'Expires header was sent.');
        $this->assertEqual($this->drupalGetHeader('Foo'), 'bar', 'Custom header was sent.');
   @@ -225,14 +220,12 @@ function testPageCache() {
        $this->drupalGet('system-test/set-header', array('query' => array('name' => 'Expires', 'value' => 'Fri, 19 Nov 2008 05:00:00 GMT')));
        $this->assertEqual($this->drupalGetHeader('Expires'), 'Fri, 19 Nov 2008 05:00:00 GMT', 'Default header was replaced.');
        $this->drupalGet('system-test/set-header', array('query' => array('name' => 'Vary', 'value' => 'User-Agent')));
   -    $this->assertEqual(strtolower($this->drupalGetHeader('Vary')), 'user-agent,accept-encoding', 'Default header was replaced.');
    
        // Check that authenticated users bypass the cache.
        $user = $this->drupalCreateUser();
        $this->drupalLogin($user);
        $this->drupalGet('system-test/set-header', array('query' => array('name' => 'Foo', 'value' => 'bar')));
        $this->assertFalse($this->drupalGetHeader('X-Drupal-Cache'), 'Caching was bypassed.');
   -    $this->assertTrue(strpos(strtolower($this->drupalGetHeader('Vary')), 'cookie') === FALSE, 'Vary: Cookie header was not sent.');
        $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, post-check=0, pre-check=0, private', 'Cache-Control header was sent.');
        $this->assertEqual($this->drupalGetHeader('Expires'), 'Sun, 19 Nov 1978 05:00:00 GMT', 'Expires header was sent.');
        $this->assertEqual($this->drupalGetHeader('Foo'), 'bar', 'Custom header was sent.');
   

   Why do we remove the assertions for the Vary header without adding them elsewhere again?

3. +++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
   @@ -24,7 +24,7 @@ class BasicAuthTest extends WebTestBase {
   -  public static $modules = array('basic_auth', 'router_test', 'locale');
   +  public static $modules = array('basic_auth', 'router_test', 'locale', 'page_cache');
   
   +++ b/core/modules/block/src/Tests/BlockTest.php
   @@ -20,6 +20,11 @@
   +  protected static $modules = ['page_cache'];
   
   +++ b/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php
   @@ -17,11 +17,15 @@ class AjaxFormPageCacheTest extends AjaxTestBase {
   +  public static $modules = ['page_cache'];
   
   +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php
   @@ -26,12 +26,16 @@
   +  protected static $modules = ['page_cache'];
   
   +++ b/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php
   @@ -19,7 +19,7 @@ class FormStoragePageCacheTest extends WebTestBase {
   -  public static $modules = array('form_test');
   +  public static $modules = array('form_test', 'page_cache');
   

   Since #606840: Enable internal page cache by default the internal page cache is turned on by default in the testing profile. Including the page_cache module here seems redundant.

Did some profiling using ab -k -c1 -n100. There is no difference for uncached pages, the improvement for cached ones is ~2ms. Standard deviation is ~3ms, so not too much confidence.

On my machine, with ab -c 1 -n 1000:

• HEAD: 8.3 ms/request
• Patch: 6.6 ms/request

Standard deviation 0.5 ms in both cases.

(From 120 to 150 requests/second.)

1. Yes, that's a left-over of trying to implement the form test directly in the test, but that's just not going to work. Can be removed.

Also, I think the Needs profiling tag can be removed. We have a pretty clear understanding of the impact here.

Yes, good point. No further profiling necessary.

Please reroll with git diff -M10%, so that moved files don't show up as deletions + creations in the diff. The current patch is twice as large as it needs to be, making it very hard to review.

Thanks :)

Thanks for the suggestion, Wim. Please check now.

I tooks like #126 is not addressed yet, setting to needs work.

As per #126, 1 & 3 is done. Coming to 2, where should the assertions for Vary added back?

Great, thanks!

Confirmed that the reroll is correct. It's identical to @Berdir's reroll in #122, with the only change being the addition of the resetAll() call made in #124.

I'd like @Bedir to verify that that resetAll() is correct.

+++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
@@ -321,6 +321,7 @@ public function testFormImmutability() {
       ->install(['page_cache_form_test']);
+    $this->resetAll();
 
     $this->drupalGet('page_cache_form_test_immutability');

So, this does many, many things.

All we should need here is a router rebuild.

Can we switch this to \Drupal::service('router.builder')->rebuild() ?

That should also make the test a bit faster, which is nice if you're debugging..

Tagging novice for that.

1. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -2,23 +2,27 @@
   +use Drupal\Core\Form\FormState;
   +use Drupal\Core\PageCache\ChainRequestPolicy;
   +use Drupal\Core\PageCache\RequestPolicy\NoSessionOpen;
   ...
   +use Symfony\Component\HttpFoundation\Request;
   

   Remove, those use statements are probably remains of previous attempts to test the FormCache here.

2. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -29,8 +33,11 @@ class PageCacheTest extends WebTestBase {
   -  public static $modules = array('test_page_test', 'system_test');
   +  public static $modules = array('test_page_test', 'system_test', 'page_cache');
   

   No need to enable page_cache module explicitly.

3. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -46,12 +53,7 @@ protected function setUp() {
   -  function testPageCacheTags() {
   -    $config = $this->config('system.performance');
   -    $config->set('cache.page.use_internal', 1);
   ...
   -
   
   @@ -79,12 +81,7 @@ function testPageCacheTags() {
   -  function testAcceptHeaderRequests() {
   -    $config = $this->config('system.performance');
   -    $config->set('cache.page.use_internal', 1);
   -    $config->set('cache.page.max_age', 300);
   -    $config->save();
   -
   +  public function testAcceptHeaderRequests() {
   
   @@ -150,12 +147,7 @@ function testAcceptHeaderRequests() {
   -  function testConditionalRequests() {
   -    $config = $this->config('system.performance');
   -    $config->set('cache.page.use_internal', 1);
   -    $config->set('cache.page.max_age', 300);
   -    $config->save();
   -
   +  public function testConditionalRequests() {
   
   @@ -195,17 +187,10 @@ function testConditionalRequests() {
   -  function testPageCache() {
   -    $config = $this->config('system.performance');
   -    $config->set('cache.page.use_internal', 1);
   -    $config->set('cache.page.max_age', 300);
   -    $config->set('response.gzip', 1);
   -    $config->save();
   -
   +  public function testPageCache() {
   

   The configuration should be kept here with the exception of $config->set('cache.page.use_internal', 1);.

4. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -60,7 +62,7 @@ function testPageCacheTags() {
   -    $cid_parts = array(\Drupal::url('system_test.cache_tags_page', array(), array('absolute' => TRUE)), 'html');
   +    $cid_parts = array(\Drupal::url('system_test.cache_tags_page',array(), array('absolute' => TRUE)), 'html');
   

   This looks like an accidental edit.

5. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -311,11 +292,6 @@ function testPageCacheAnonymousRolePermissions() {
      public function testPageCacheWithoutVaryCookie() {
   -    $config = $this->config('system.performance');
   -    $config->set('cache.page.use_internal', 1);
   -    $config->set('cache.page.max_age', 300);
   -    $config->save();
   -
   

   I highly suspect that this is flawed. We need a max_age set, otherwise the Cache-Control will not be added and the code which is supposed to add Vary: Cookie will not be triggered. This means we are not actually testing something if we do not enable max_age.

As discussed in IRC, the assertions for the Vary header need to be restored in the PageCacheTest::testPageCache() test.

Looking at this now.

Ok, PageCacheTest is green again for me, also did some cleanup there, did another review of the patch and cleaned it up some other things. Reverted a few protected static $modules (they are supposed to be public, because externally called) and cleaned up a test method that was executed twice, identically.

1. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -201,6 +212,7 @@ public function testPageCache() {
   +    $this->assertEqual(strtolower($this->drupalGetHeader('Vary')), 'cookie,accept-encoding', 'Vary: Cookie header was sent.');
   
   @@ -209,12 +221,14 @@ public function testPageCache() {
   +    $this->assertEqual(strtolower($this->drupalGetHeader('Vary')), 'user-agent,accept-encoding', 'Default header was replaced.');
   ...
   +    $this->assertTrue(strpos(strtolower($this->drupalGetHeader('Vary')), 'cookie') === FALSE, 'Vary: Cookie header was not sent.');
   

   Can you explain these changes?

2. +++ b/core/modules/system/src/Tests/System/SiteMaintenanceTest.php
   @@ -39,23 +39,9 @@ protected function setUp() {
   +  protected function TestSiteMaintenance() {
   

   Nit: s/Test/test/

Talked to @berdir, point 1 was just restoring incorrectly removed assertions, and adding a few missing assertions.

Still leaves point 2, but that can be fixed on commit.

Back to RTBC.

I like you too testbot.

Tests look much better. Thanks!

Seems like catch has been all over this one.

ups..

This time with git settings set to:
[diff]
renames = copies

Reviewed the reroll on the phone in the Drupal dev days food queue, PageCacheTest looks good.

Follow-up: #2283477-60: [META] Review hook_help texts & copy text to drupal.org

Follow up: #2473327: PageCacheTest fails if mod_deflate is not enabled

We now have a page_cache.module component, so let's move it there.