Add a local, PhpStorage-based cache backend

The patch attached adds a PhpBackend that stores cache keys in a PHP file. Currently this creates one file per bin - a standard install creates a php file of about 21k.

1. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -57,7 +57,13 @@ public function get($bin) {
        }
        else {
   -      $service_name = 'cache.backend.database';
   +      // @todo do something more sensible.
   +      if ($bin == 'config') {
   +        $service_name = 'cache.backend.php';
   +      }
   +      else {
   +        $service_name = 'cache.backend.database';
   +      }
        }
        return $this->container->get($service_name)->get($bin);
      }
   

   This is essentially the same problem as the cache backend chain would have if we'd ever use it. We currently don't have a good way to provide configuration for this from outside CacheFactory (this would be fine within, and could simply add configuration for those bin by default to $cache_settings.

2. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,301 @@
   +<?php
   ...
   +
   +  /**
   +   * Writes the cache to PHP storage.
   +   */
   +  protected function writeCache() {
   +    $storage = $this->storage();
   +    $data = Variable::export($this->cache);
   +    $content =<<<EOF
   +<?php
   +\$cache = $data;
   +EOF;
   +    $storage->save($this->bin, $content);
   +  }
   

   We're going to have pretty serious concurrency issues when writing out to a single file. This will need a similarly complex approach as CacheCollector with locking and partial updates and all that stuff.

   It also might not scale for the cache_config use case, because you probably don't want to every single config entity in one include file.

   I guess a file per bin is the saner approach, as suggested in the issue summary. Should still be fairly fast if you don't check the timestamp on every request (opcache in 5.5. is much more flexible than apc I think to configure that)

3. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,301 @@
   + * Definition of Drupal\Core\Cache\PhpBackend.
   

   Contains ;) (SCNR)

4. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,301 @@
   +
   +    // Whether or not Drupal\Component\PhpStorage\PhpStorageInterface::getPath()
   +    // checks for the existence of the file is an implementation of the storage.
   +    if ($filename = $storage->getPath($this->bin)) {
   +      // If this file exists it'll set a variable called $cache.
   +      @include_once $filename;
   +    }
   

   Why not use $storage->load() ? See DrupalKernel::initializeContainer().

   Also not sure if include*_once* is what we want here. Especially in tests, we will write and load that file many times in the same request?

Did we consider the option to append new and updated entries to an existing file per bin + only flush that out entirely upon dfac()?

I.e., like this:

$cache['foo'] = 'foo';
$cache['bar'] = 'bar';
# cache->set('foo', ...
$cache['foo'] = 'baz';

#7: very interesting thought :) But do we know that appends will be significantly faster? And what if the file size grows too large before dfac() is called? Finally: it'll leave less space in the opcode cache for other things.

It feels like that could be an optimization. Let's do the simplest possible thing now, which will already be a significant performance boost.

It feels like that could be an optimization. Let's do the simplest possible thing now, which will already be a significant performance boost.

In a way, it is funny that we're considering to inverse the current situation (yml files + database cache to config in db + caches in files) :)

Obviously, for actual sites this will be a nice performance improvements, as this should be very fast for reads. Tests do an insane amount of cache writes, so I'm not so sure there...

The test run above took 36min on #1733, another patch that I checked 30m. Maybe the single file approach will be faster, I'm not sure.

Simple note, the PHP code in the file can return array(); this actually allows code such as if ($data = @include $file) {}

Note that include_once might prevent us from re-loading the file content later in the same request if needed while include won't cause any trouble (for exemple if I drop the cache object and create a new instance).

Also the $data = include allows the array inside the file to be anonymous (unnamned) and thus decouples the array name from the file and unties the file from the backend.

Also, may be the keys should be subjected to a consistent hashing mecanism when wrote, so we could dispatch entries in more than one file, but I'm not sure if this actually worthes anything. Considering that those kind of PHP cache should be only used for performance critical path, it should not contain thousands of entries and trying to avoid scaling problems over this seems like premature and useless optimisation.

To avoid concurency problem, a lock wait should be enough, but when the lock switches to wait mode then acquires, the thread should clear its own loaded entries reload the file (hence the include instead of include_once) to be up to date before writing its own local modifications into it so we don't loose the previous thread writes. Note that I didn't looked at how the storage actually works, but I think that it doesn't do such things, so the cache file storage should probably use its own implementation.

In order to gain a few I/O the file could be wrote at PHP shutdown time (such as some caches are actually doing it). This needs to historize all modifications being done during the thread run in order to be able to refresh from the file (if changed by another thread) before writing.

The attached patch uses a file per key and var_export. If the $data provided to a cache set is an object it must implement __set_state or an exception is thrown. This patch also suggests that we really ought to have a CacheItem object instead of just returning a \stdClass since then this could implement __set_state and we would not need to futz with array to object conversions.

This patch reduces the number of queries from 105 to 78 and reduces the memory usage due to not using unserialize for each cache item.

CachedStorage::readMultiple

Run 1: Without patch
Run 2: With patch

Top level summary

I am not sure this is a really good idea to have this amount of files, you are going to do an awful lot of file I/O operations. Using the checkFile() function as defined, doing file_exists() calls will bypass the OPCode cache and can make it very slow depending on the underlaying FS.

The checkFile() should not even exist, if (false !== ($data = @include $file)) {} would probably do a lot less I/O. Using one file per cache entry is probably shooting yourself in the foot. Storing eveything in one single file was not a bad idea for a first pass. Using a grosser approach for dividing it later if necessary shounds a better idea (per language, using an hashing algorithm, or whatever else...).

My opinion is but I might be wrong is that the concurency problem should be dealt with a custom approach: tracking locally modified keys, put a file lock then reload the file before write, then rewrite it modifying only the locally modified keys sounds like a finer and safe approach.

The more I think about this, the more I agree with #15. Apologies for restating some of the previous arguments.

We cannot assume the filesystem is fast on any multi-web setups, so this approach is only helping single server deployments, which is hardly worth doing. We have to assume 1 stat call per file, which is not cheap on shared filesystems. Relying on settings like apc.stat and opcache.revalidate_freq is not a reasonable solution.

Also, I don't think we can say that read-only is a valid approach without having experienced any real-world D8 usage. I suspect this will rarely be acceptable.

If we're going to do this, we should treat the files as a local, inconsistent cache bin, the same way we'd treat apcu. At the start of the request, get a set of timestamps from the consistent cache, use that to know whether or not its safe to use the local cache. If we make the assumption from the start that the files are local, then we can be reasonably confident that this is a performance improvement.

So, I like this issue if it's about adding support for an inconsistent cache API, but I'm not convinced that a PhpStorage-based cache backend alone is worth our time.

All I ask is that the multi-web scenario isn't an afterthought, which it was when we decided to compile to container to php storage. That's the hard problem. Solving this for single server isn't.

We cannot assume the filesystem is fast on any multi-web setups, so this approach is only helping single server deployments, which is hardly worth doing.

I couldn't disagree more. Many — if not most — Drupal sites run on a single server. We develop on a single server. Let's not delay a vast improvement for many Drupal sites.

AFAICT Drupal core cannot solve the multi-web scenario anyway: it depends on the particular multi-web scenario what the best solution is, and especially on the available infrastructure (memcached, redis…).

In principle I agree with "let's not make multi-web an afterthought". But in practice, that pretty much implies we cannot make progress for single-web either. Let us at least move forward with single-web, so that we can profile more effectively in the single-web case, and thus improve performance for both single- and multi-web cases.

This issue is incredibly important for a "fast by default" Drupal 8. If you run a multi-web website, then you're an expert already, since you choose the components in your multi-web infrastructure, therefore you're also in the best position to judge how to make it work performantly. In the single-web case, Drupal can and should make things fast out of the box for you.

also - what about APC/APCu for the fast cache option?

Right. This solution has a limited audience IMO. Anyone who has APC on 5.4 can just use that as cache backend and it will be as fast as this, I think. Same for anyone with APCu on 5.5. And isn't APCu already nearly a necessity for good performance considering thats where settings.php suggests you put your class_loader.

I'm OK with going ahead here, but its a small win AFAICT.

Just for completeness: Drupal core happens to ship with some prior art from Doctrine already:

PhpFileCache
FilesystemCache

Nice catch, this might worth the shot to use, depending on how well it fits to the current API.

The test failure was legitimate, core/tests/Drupal/Tests/Core/Cache/InconsistentBackendTest.php was accidentally included from the #2231595: Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend patch.

Straight reroll that simply removes that accidentally added file.

(That also explains why #38 was larger than #36 despite only removing code.)

1. +++ b/core/lib/Drupal/Component/PhpStorage/FileReadOnlyStorage.php
   @@ -83,4 +83,30 @@ function writeable() {
   +  public function listAll() {
   
   +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
   @@ -248,4 +255,23 @@ protected function unlink($path) {
   +  public function listAll() {
   

   It's a shame we have to duplicate this code.

2. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,284 @@
   +   * Constructs a MemoryBackend object.
   

   PhpBackend

3. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,284 @@
   +      if ($this->bin != 'cache') {
   +        $store = 'cache_' . $this->bin;
   +      }
   +      else {
   +        $store = 'cache';
   +      }
   

   We don't need this logic anymore, now we don't have a 'cache' bin.

Trying to find all nitpicks so we can finally RTBC this! :)

1. +++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
   @@ -78,4 +78,25 @@ public function delete($name);
   +   *   The full file path for the provided name. Return FALSE if the
   +   *   implementation needs to prevent access to the file.
   +   */
   +  public function getPath($name);
   

   Wouldn't getAbsolutePath() be a better name? Or getFullPath()?

2. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,278 @@
   + * Definition of Drupal\Core\Cache\PhpBackend.
   

   s/Definition of Drupal/Contains \Drupal/

3. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,278 @@
   + * Defines a PHP cache implementation.
   

   Maybe mention OpCache?

4. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -0,0 +1,278 @@
   +  protected function prepareItem($cache, $allow_invalid) {
   

   Docs for $allow_invalid are missing.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.php
   @@ -136,10 +136,11 @@ public function testSetGet() {
   -    $backend->set('test1', 7);
   +    $with_backslash = array('foo' => '\Drupal\foo\Bar');
   +    $backend->set('test1', $with_backslash);
   ...
   -    $this->assertIdentical(7, $cached->data);
   +    $this->assertIdentical($with_backslash, $cached->data);
   

   Sounds like these tests are added specifically for the PHPStorage backend. Let's not put them in the generic cache backend test?

6. +++ b/core/modules/system/lib/Drupal/system/Tests/Cache/PhpBackendUnitTest.php
   @@ -0,0 +1,35 @@
   + * Definition of Drupal\system\Tests\Cache\PhpBackendUnitTest.
   

   s/Definition Drupal/Contains \Drupal/

Fixes incorrect 'protected' access on a public interface method.

#44: fair.

I can't find any other things to nitpick.

php -r 'print serialize("\nEOF\nyou are screwed");'

s:20:"
EOF
you are screwed"

Escape, my friends, escape.

Edit: str_replace('\\', '\\\\', serialize($item)); this is not helping; serialize doesnt put a literal \n in there.

OK, so the reason for not using an escape and instead finding a usable heredoc delimiter is because we can't unescape upon reading. I understand now (although a comment would be helpful), and I am fine with the idea but the random and the guard is weird. Why not:

    $suffix = '';
    do {
      $EOF = 'EOF' . $suffix++;
    } while (preg_match('/^' . $EOF . ';?$/m', $data));

There are a couple bugs caught by the preg_match: EOF and EOF; both are deadly. You don't reaaaaally know the line endings (Mac, Windows, Unix, fun!) so let PCRE deal with that crap. This loop will produce EOF, EOF1, EOF2 etc and surely will terminate (as long as $data is finite but it is).

This already seems like it should be a follow up in a separate issue tbh.

+++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
@@ -263,11 +263,22 @@ public function removeBin() {
+    $suffix = '';
...
+    } while ($suffix++ < 1000 && ($unsafe_eof_in_data = preg_match('/^' . $EOF . ';?$/m', $data)));

It might be me, but ''++ freaks me out. Is there a way around that? I.e. can we initialize $suffix as 0 or something?

We could but then the default EOF wouldn't be EOF but EOF0. This is easy and safe. I know my way around PHP ++ . This is not the character increment which leads to $a='2d9';$a++;$a++ which is my all time favorite PHP WTF. This is simply casting the empty string to an int which is a 0 then adding 1.

OK. I personally would favor having EOF0 then, but I don't really mind either. Just wanted that reassurance. :-) Thanks!

#63: the patch in #58 applies cleanly on HEAD?

Filed a follow-up #2265977: Cache\PhpBackend expands $foo and ${bar()}.