Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend

Without looking at this in detail, just wondering if we could somehow integrate this into the existing CacheBackendChain that we have but never use right now.. Seems like this would be an important feature to reliably use that anyway, at least when you have multiple servers?

This looks great, but could use some more scrutiny from others :)

I fixed all the nitpicks I could find. But I also had trouble understanding ChainedConsistentAndInconsistentBackend… so I added a paragraph in the class's docs, explaining the algorithm used. In doing so, I got actually even more confused by the fact that there was both $this->consistencyTimestamp and $this->lastSetTimestamp. Because, really, you only need *one*! I also found "consistency timestamp" to be a rather confusing term: to me, it means "this is the last known time where the inconsistent backend is fully consistent with the consistent backend". But that's not at all what it does. All it does is tracking the timestamp after which the cache entry in the inconsistent backend must be created, if not, then it's stale/wrong/inconsistent. Therefore, I consolidated those two variables into a single one called $this->lastWriteTimestamp. Hopefully I'm not the only one who thinks that's less ambiguous. (If not, you can easily restore the original variable name, at least there's now only 1 instead of 2 variables.)

1. +++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
   

   Maybe ApcuBackend.phpor ApcUserBackend.php would be a better name?

2. +++ b/core/lib/Drupal/Core/Cache/ApcBackend.php
   @@ -0,0 +1,311 @@
   +  public function isEmpty() {
   +    $apc = new ApcIterator('user');
   +    return $apc->getTotalCount > 0;
   +  }
   

   This counts the data in *all* APC cache bins, rather than the current one?

3. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -51,7 +51,7 @@ function __construct(Settings $settings) {
        if (isset($cache_settings['bins'][$bin])) {
   -      $service_name = $cache_settings[$bin];
   +      $service_name = $cache_settings['bins'][$bin];
   

   Nice catch!

   But… shouldn't this have been caught a long time ago by test coverage? Scary… :(

4. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   --- /dev/null
   +++ b/core/lib/Drupal/Core/Cache/ChainedConsistentAndInconsistentBackend.php
   

   Why doesn't this subclass \Drupal\Core\Cache\BackendChain? I think it's because BackendChain "stupidly" chains cache back-ends, not allowing for inconsistent cache back-ends, only allowing for consistent-but-automatically-discarding-cache-entries cache back-ends (i.e. LRU/MRU/…)?

   If so, it'd be great if this were documented on ChainedConsistentAndInconsistentBackend.

3: #2233337: Impossible to specify per-bin cache backend service from within settings.php. The bug was introduced in the cache.default issue, the bins subkey didn't exist before that.

In chat, beejeebus pointed me to a silly mistake.

Here's an updated that adds a couple small tests and fixes what I believe would have been a bug in getMultiple. I think the new backend wrappers need to be renamed, but holding off for now to keep the interdiff sane.

I may have been wrong about my changes to getMultiple in #11. I was convinced it was necessary this morning, but after talking with beejeebus, I think I may have been confused.

Simple change fixing the tests from #11.

Implementing Mark's TODO using a mock for the inconsistent cache.

#16: can you re-post that interdiff with git diff -M? Then we can see what *really* changed between the diffs — thanks :)

Now with fewer lulz and hopefully all tests passing.

Since #37 passes testing, I took this for a spin.

After installing, I put the required

$settings['cache']['default'] = 'cache_inconsistent_factory';

into settings.php, and I am registering APC cache hits. I took the /admin/people page on a fresh install, dumped APC user cache, and then requested it 6 times. On the subsequent 5 page loads, I got 35 cache hits each time from APC, as the attached apc.php screenshot shows.

Unfortunately, I don't think we can quite turn this on by default. Starting with 8.x core with the patch applied and adding the cache line to a fresh settings.php, I get this exception after entering MySQL information into the installer. Looks like KeyValueStore can't work with no DB. What would be the easiest way to short-circuit this in the installer?

Additional uncaught exception thrown while handling exception.

Original

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8.key_value' doesn't exist: SELECT 1 AS expression FROM {key_value} key_value WHERE ( (name = :db_condition_placeholder_0) AND (collection = :db_condition_placeholder_1) ); Array ( [:db_condition_placeholder_0] => cache_last_write_timestamp_cache_config [:db_condition_placeholder_1] => state ) in Drupal\Core\Database\Connection->query() (line 569 of /Users/smerrill/Projects/drupal8/core/lib/Drupal/Core/Database/Connection.php).

Drupal\Core\Database\Connection->query('SELECT 1 AS expression
FROM 
{key_value} key_value
WHERE ( (name = :db_condition_placeholder_0) AND (collection = :db_condition_placeholder_1) )', Array, Array)
Drupal\Core\Database\Query\Select->execute()
Drupal\Core\Database\Query\Merge->execute()
Drupal\Core\KeyValueStore\DatabaseStorage->set('cache_last_write_timestamp_cache_config', 1398132276.0914)
Drupal\Core\State\State->set('cache_last_write_timestamp_cache_config', 1398132276.0914)
Drupal\Core\Cache\InconsistentBackend->markAsOutdated()
Drupal\Core\Cache\InconsistentBackend->delete('typed_config_definitions')
Drupal\Core\Config\TypedConfigManager->clearCachedDefinitions()
Drupal\Core\Config\ConfigInstaller->installDefaultConfig('core', 'core')
drupal_install_system(Array)
install_base_system(Array)
install_run_task(Array, Array)
install_run_tasks(Array)
install_drupal()
Additional

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8.key_value' doesn't exist: SELECT 1 AS expression FROM {key_value} key_value WHERE ( (name = :db_condition_placeholder_0) AND (collection = :db_condition_placeholder_1) ); Array ( [:db_condition_placeholder_0] => cache_last_write_timestamp_cache_bootstrap [:db_condition_placeholder_1] => state ) in Drupal\Core\Database\Connection->query() (line 569 of /Users/smerrill/Projects/drupal8/core/lib/Drupal/Core/Database/Connection.php).

Drupal\Core\Database\Connection->query('SELECT 1 AS expression
FROM 
{key_value} key_value
WHERE ( (name = :db_condition_placeholder_0) AND (collection = :db_condition_placeholder_1) )', Array, Array)
Drupal\Core\Database\Query\Select->execute()
Drupal\Core\Database\Query\Merge->execute()
Drupal\Core\KeyValueStore\DatabaseStorage->set('cache_last_write_timestamp_cache_bootstrap', 1398132276.0935)
Drupal\Core\State\State->set('cache_last_write_timestamp_cache_bootstrap', 1398132276.0935)
Drupal\Core\Cache\InconsistentBackend->markAsOutdated()
Drupal\Core\Cache\InconsistentBackend->set('hook_info', Array)
Drupal\Core\Extension\ModuleHandler->getHookInfo()
Drupal\Core\Extension\ModuleHandler->buildImplementationInfo('watchdog')
Drupal\Core\Extension\ModuleHandler->getImplementationInfo('watchdog')
Drupal\Core\Extension\ModuleHandler->getImplementations('watchdog')
watchdog('php', '%type: !message in %function (line %line of %file).', Array, 3)
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)

In IRC, beejeebus points out that we're not yet trying to make this the default cache backend, so I'll set this back to Needs Review.

If this becomes the default cache backend, we may be able to check for MAINTENANCE_MODE === "install" in the factory and return a regular database cache when that is the case to fix the installer.

I agree, the installer stuff definitely belongs in another issue.

1. +++ b/core/includes/bootstrap.inc
   @@ -904,8 +904,12 @@ function drupal_serve_page_from_cache(stdClass $cache, Response $response, Reque
   +  // Cache items have a created time accurate to the millisecond, however http
   +  // times only work up to the second, round created.
   

   The wording here is sloppy, what about something like: "[…] HTTP's caching mechanism works with timestamps with precision up to a second, so perform the necessary rounding."

2. +++ b/core/includes/bootstrap.inc
   @@ -904,8 +904,12 @@ function drupal_serve_page_from_cache(stdClass $cache, Response $response, Reque
   +  $created = (int) $cache->created;
   

   The comment says rounding should be applied, but we're really just dropping all decimals.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -102,9 +74,18 @@ public function onRespond(FilterResponseEvent $event) {
   +      $response->setEtag((int) $cache->created);
   +      $response->setLastModified(new \DateTime(gmdate(DATE_RFC1123, (int) $cache->created)));
   

   These are unnecessary; drupal_serve_page_from_cache() already sets these.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -57,34 +57,6 @@ public function onRespond(FilterResponseEvent $event) {
   -    $response->setLastModified(new \DateTime(gmdate(DATE_RFC1123, REQUEST_TIME)));
   ...
   -    $response->setEtag(REQUEST_TIME);
   
   @@ -102,9 +74,18 @@ public function onRespond(FilterResponseEvent $event) {
   +      $response->setLastModified(new \DateTime(gmdate(DATE_RFC1123, REQUEST_TIME)));
   

   This change implies no ETag header is being set anymore in the case page caching is disabled (globally or on the current page).

   There was a todo to remove it, but is this really the right place to be changing this?

   Overall, it seems all of the changes in FinishResponseSubscriber are out of scope?

I agree that we can probably not worry about messing with drupal_serve_page_from_cache() more than is necessary until #2016629: Refactor bootstrap to better utilize the kernel lands.

The attached patch rolls back any changes in FinishResponseSubscriber and tries to have clearer language regarding the HTTP cache timestamps.

Well, testbot says we do need those items, so here's the patch with them added back in.

#48: I think #45 only failed because it restored $response->setLastModified(new \DateTime(gmdate(DATE_RFC1123, (int) $cache->created)));, which uses $cache->created as the number of seconds since epoch, but now $cache->created is actually 1000 times larger (microtime(TRUE)), so a simple division by 1000 should fix #45.

48: 2231595-48.patch queued for re-testing.

Removed an embedded image in comment #18 by @beejeebus, which appears to be a corrupted screenshot; the file contains additional garbage of ~25 MegaBytes (which has been downloaded by everyone who visited this issue).

1. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
   @@ -0,0 +1,260 @@
   +   * Constructs a InconsistentBackend object.
   

   s/a/an/

2. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
   @@ -0,0 +1,260 @@
   +  protected function setLastWriteTimestamp($timestamp) {
   

   Missing docs.

   But more importantly: why even have this method, if it's called only once and its body is a single statement? :)

3. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
   @@ -0,0 +1,260 @@
   +   * Mark the inconsistent cache bin as outdated because of a write.
   

   s/Mark/Marks/

   (Always 3rd person singular.)

4. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
   @@ -0,0 +1,260 @@
   +    $now = microtime(TRUE);
   +    if ($now > $this->lastWriteTimestamp()) {
   

   Won't this always evaluate to true?

5. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
   @@ -0,0 +1,260 @@
   +    // Retrieve as many cache entries as possible from the (faster) inconsistent
   

   I also prefer "cache entries", but CacheBackendInterface calls them "cache items", so let's use that too for consistency.

6. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
   @@ -0,0 +1,260 @@
   +          $cids[] = $item->cid;
   

   This modifies the incoming &$cids parameter by reference…

   That parameter should be modified to only contain those cache IDs of the cache items that are successfully returned. I don't think that's happening right now.
   (And if I'm right, then GenericCacheBackendUnitTestBase's coverage is lacking.)

   (I have the feeling I already pointed this out before, but I can't find it in this issue, so it must've been another issue.)

7. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
   @@ -0,0 +1,260 @@
   +    // If there were any cache entries that weren't available in the
   +    // inconsistent backend, retrieve them from the consistent backend and store
   +    // them in the inconsistent one.
   

   80 cols.

8. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,69 @@
   +/**
   + * @group Cache
   + */
   +class InconsistentBackendTest extends UnitTestCase {
   +
   +  public static function getInfo() {
   +    return array(
   +      'name' => '',
   +      'description' => '',
   +      'group' => 'Cache'
   +    );
   +  }
   

   Docs incomplete.

9. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,69 @@
   +  public function setUp() {
   

   {@inheritdoc}

10. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
    @@ -0,0 +1,69 @@
    +  public function testGetFromInconsistentCache() {
    

    A @covers docblock might be sufficient in this and other cases.

11. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
    @@ -0,0 +1,69 @@
    +    $inconsistent_backend->set("foo", "bar");
    

    This confused me so much, that I think that ChainedBackend might be a better name for this new cache back-end.

12. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
    @@ -0,0 +1,69 @@
    +    $this->assertEquals("bar", $inconsistent_backend->get("foo")->data);
    

    I'm missing an assertion here that the consistent cache back-end never gets called. (i.e. no fallthrough)

13. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
    @@ -0,0 +1,69 @@
    +  public function testFallThroughToConsistentCache() {
    

    This does not test a get (read) fallthrough, but a set (write) fallthrough.

    I think we should test both.

This adds a test for when we have an inconsistent cache hit, the consistent cache does not get called.

Also a few code style cleanups.

#57.4: hah, of course — I *knew* I was overlooking something. Comment helps, thanks.

#57.6: Ok then. Must've misread that also. I just remembered we have test coverage for this, so surely it must be correct.

+++ b/core/lib/Drupal/Core/Cache/InconsistentBackend.php
@@ -105,17 +105,16 @@ public function lastWriteTimestamp() {
+    // Clocks on a single server can shift. Multiple webheads may have slightly
+    // differing opinions about the current time. Given that, don't assume
+    // 'now' on this webserver is always later than our stored timestamp.

"server", "webheads" and "webserver" — let's use consistent terminology?

Also, "clock drift" is the correct term, not "clock shift".

#58: thanks!

1. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,156 @@
   +   * The consistent cache.
   ...
   +   * The inconsistent cache.
   

   s/cache/cache backend/

2. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,156 @@
   +   * Tests getting data from the inconsistent cache.
   ...
   +   * Tests a cache miss gets data from the inconsistent cache.
   

   s/inconsistent cache/inconsistent cache backend/

3. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,156 @@
   +   * Tests getting data from the inconsistent cache doesn't hit the backend.
   

   s/backend/cache backend/

4. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,156 @@
   +    // Test outdated cache item.
   +    $inconsistent_cache = new MemoryBackend('bar');
   

   If we want to test that a cache get does *not* hit the consistent cache back-end, then why would we want to store an outdated cache item in the inconsistent cache back-end?
   This comment is wrong.

5. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,156 @@
   +    // The test cache item.
   +    $item = (object) array('cid' => 'foo', 'data' => 'baz', 'created' => 22222);
   

   This is not used anywhere.

6. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,156 @@
   +
   ...
   +
   +
   ...
   +
   

   Extraneous newlines.

Thanks Wim. I've made fixes for #59.

Leaving at NR to gather some more feedback.

I think the code looks fine overall. But I'm still not too fond of the name, I think it's rather confusing. I think ChainedBackend might be better.

One remaining stupid nitpick that can definitely be ignored if no more rerolls are needed:

+++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
@@ -0,0 +1,151 @@
+  }
+}

Missing newline.

A backport of this issue to D7 is welcome, there's a few modules that are confronted to the cache consistency problem (such as APC) and having a generic solution in core would be a good idea.

@pounard Beejeebus has a D7 port already at https://drupal.org/project/schrodicache

Would be nicer to have in core I meant !

A big +1 on having deep support for inconsistent caching. On Pantheon's Redis boxes, we know they're full when they saturate their network connections. Reducing the common case to just checking for changes rather than pulling the actual data would help stuff scale much better. It would also probably make the MySQL/MariaDB plus local APC/APCu work really well even for huge sites.

Re-roll and fix for #61.

Patch implementation doesn't match the issue summary. If the implementation is what's desired, can we get an updated summary? Specifically:

Add an extension of the CacheChain class in core, that includes a consistency check.

In the patch, InconsistentBackend does not extend BackendChain. Would be good for the summary to explain why: I'm guessing something related to a need for timestamp checking making none (or not enough of) the methods reusable, but if there's another reason, would be good to have it documented in the summary.

on set/delete, write a timestamp to the shared cache, also store the timestamp in the local storage

Patch implements timestamp in state rather than in the caches. Why?

Also:

1. +++ b/core/lib/Drupal/Core/Cache/InconsistentBackendFactory.php
   @@ -0,0 +1,46 @@
   +    $consistent_service = 'cache.backend.database';
   +    $inconsistent_service = 'cache.backend.apcu';
   +
   +    $cache_settings = $this->settings->get('cache');
   +    if (isset($cache_settings['inconsistent_cache']) && is_array($cache_settings['inconsistent_cache'])) {
   +      if (!empty($cache_settings['inconsistent_cache']['consistent'])) {
   +        $consistent_service = $cache_settings['inconsistent_cache']['consistent'];
   +      }
   +      if (!empty($cache_settings['inconsistent_cache']['inconsistent'])) {
   +        $inconsistent_service = $cache_settings['inconsistent_cache']['inconsistent'];
   +      }
   +    }
   

   Why do we need this controllable from settings.php? Why not pass the 2 services as constructor dependencies in the core.services.yml entry?

2. +++ b/core/core.services.yml
   @@ -26,6 +26,11 @@ services:
   +  cache_inconsistent_factory:
   

   More consistent with other backends to rename this to cache.backend.inconsistent? Or, if we do the suggestion above, then perhaps even cache.backend.inconsistent.apcu.database? People can then create differently named services for different backend combinations. Not sure if that level of specificity in the service name would be useful or not.

#67: both of your "also" points sound great.

Awesome feature, but the terminology seems confused atm. The patch uses "inconsistent backend" termonology for both the "local, fast but possibly outdated backend" and the "wrapper around a chain of the former and a remote, reliable backend" (see class name, property names inside that class, service and factory names).

Also, method order in the class feels a bit random ?

Patch implements timestamp in state rather than in the caches. Why?

Because that is not ephemeral data.

Because that is not ephemeral data.

How so? It can safely get dropped, and that just means you need to fetch from the slower cache. Meanwhile, why make a cache backend have a dependency on a non-ephemeral storage service that is potentially slower than even the slow cache?

At some point I remember talking through this with Justin and at the time, losing the timestamp meant it could result in inconsistent data being read. Either I was wrong or the design changed since then, because looking at it now, it does seems fine if it goes away.

It's still a bit of a micro-optimization though, so I dont really care if that gets changed.

It's still a bit of a micro-optimization though

#39 indicates it could solve a functional issue related to being able to actually use this by default, not just a micro-optimization, but not sure if that comment is still relevant.

Awesome feature, but the terminology seems confused atm.

renamed InconsistentBackend to ChainedFastBackend.

What do you all think of SynchronizedBackend and cache.backend.synchronized as names? I don't think we need the word "chained" in there, since this isn't really a chain, in that it contains only two "links" rather than an arbitrary number, and the two links are not equivalent: one is authoritative and the other isn't.

Like the new naming. It makes more sense to me.

Found a few minor issues:

1. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,258 @@
   +   * @param \Drupal\Core\Cache\CacheBackendInterface
   

   Missing variable name

2. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,258 @@
   +   * @param \Drupal\Core\Cache\CacheBackendInterface
   

   Missing variable name

3. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,258 @@
   +  public function isEmpty() {
   

   This method doesn't exist on the interface.

4. +++ b/core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php
   @@ -0,0 +1,152 @@
   + * Contains \Drupal\Tests\Core\Cache\ChainedFastBackendTest.
   

   Class name doesn't match file name.

Not sure SynchronizedBackend adds much over ChainedFastBackend. It doesn't really imply what's happening.

It's true that chained isnt totally accurate, but if we're going for accuracy, it's more of a write-through backend or something.

Adding to the naming brainstorm : the main idea is "fast with fallback", with the rest being implementation details ?

Also, +1 on the timestamp management being handled by the "fast backend" internally in its own storage without depending on an external system like state() That probably means that it needs to be slighty more than a CacheBackendInterface, though.
--> FastCacheBackendInterface extends CacheBackendInterface ?

@#74, #75, #76
One of the abilities of Drupal 7 was to be able to bootstrap without database until it gets after the page cache stuff, so I guess in that regard keeping the timestamps into the cache backend itself makes sense, but in Drupal 8 the K/V store that serves for the state API is supposed to be pluggable, so problem solved. You can safely plug the K/V store in a faster backend, even the same as the cache I suppose.

Re my own #80

Also, +1 on the timestamp management being handled by the "fast backend" internally in its own storage without depending on an external system like state() That probably means that it needs to be slighty more than a CacheBackendInterface, though.
--> FastCacheBackendInterface extends CacheBackendInterface ?

Scratch that of course, the timestamps can't be stored by the "fast, local" backend, precisely since they have to be shared between web heads.
Still, +1 on having the ChainedFastBackend wrapper store them as a special "housekeeping" entry in the "remote, consistent" backend rather than in state(). The timestamps are metadata about the entries in the consistent backend when it's used as fallback for a faster backend, it seems only natural to store them along with those entries ?

re @pounard #81 : sure, storing them in state() works functionnally, and you can indeed switch the state() k/v backend to keep the whole thing efficient, but then it means this "fast cache with fallback" feature de facto constrains where you put your (whole) state store. Why would we choose to couple with state() if we can have a purely standalone system ?

Yes, I do agree with that last statement, indeed.

Code looks fine to me :-)

Nitpick / docs review :

1. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,241 @@
   + * Defines a backend with a fast and consistent backend chain.
   

   a fast and *a* consistent backend ? Otherwise it reads like its the same backend that is both fast and consistent :-)

   + In order to grasp the full picture, "one fact and one consistent" is a bit short, the body of the doc could first expand the characterization of the backends :
   - one backend is fast (typically APCu) but inconsistent (local to each web head).
   - one backend is slower (the usual cache backends like sql or memcache), but consistent (shared between web heads).

   Then the following paragraph explains the sync and "last write timestamp" logic alright, but without the above it's not really clear why that logic is even needed.
   It would also make it clearer than this "chained backend" is only useful on sites with several web heads, and that sites with a single head might as well use the fast, local backend directly ?

2. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,241 @@
   + * the consistent (slower) cache backend instead; at the same time we update the
   

   80 chars

3. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,241 @@
   +   * Cache entry key prefix, for the bin-specific entry to track the last write.
   

   80 chars :-/ Lose the comma ?

4. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,241 @@
   +  public function get($cid, $allow_invalid = FALSE) {
   +  protected function lastWriteTimestamp() {
   +  protected function markAsOutdated() {
   +  public function setMultiple(array $items) {
   +  public function getMultiple(&$cids, $allow_invalid = FALSE) {
   +  public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array()) {
   

   method order is a bit random, could we reorder those ?

   +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -0,0 +1,45 @@
   +   * Instantiates an inconsistent cache backend class for a given cache bin.
   

   remnant of 'inconsistent'. This is now called "chained fast backend".

5. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,241 @@
   +   * Initialize the last write timestamp.
   +   */
   +  protected function lastWriteTimestamp() {
   

   3rd person.

   + The rest of the code uses this method as a getter, not as an initializer.
   --> getLastWriteTimestamp() ?
   (then do we need a setter for consistency ?)

6. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -0,0 +1,241 @@
   +    // differing opinions about the current time. Given that, don't assume
   ...
   +    // If there were any cache entries that weren't available in the fast
   

   I think we favor "do not", "are not", ... over "don't", "aren't", ... in comments.

Also, looks like this chained backend is great for "registry"-like cache bins (e.g plugin discovery), where the set of entries is relatively stable over a period of time, but probably not so much for stuff like the page cache or entity / entity render cache, where new entries will constantly invalidate the local cache anyway ?

If the above is correct, might be worth mentioning as a hint in the class doc ?

Thanks !

public function get()
public function setMultiple()
public function getMultiple()
public function set()

is still an unnatural / intertwined order that doesn't really help when reading the code in the class :-p

No need to block commit for this though.
Looks good to me, but I jumped pretty late in the issue, so I'll let others confirm the RTBC.

I think we are finally RTBC here. if anyone re-rolls they can do the method reorder per #93

quick followup for method reorder.

Thanks yched

Fixed the whitespace issues in the patch on commit.

Committed e2800b2 and pushed to 8.x. Thanks!

Follow-up issue: #2541432: Follow-up for #2231595: Document why ChainedFastBackend cannot use BackendChain.