Problem/Motivation

A number of problems exist with multiple webheads and phpstorage (ie #2301163: Create a phpstorage backend that works with a local disk but coordinates across multiple webheads).

Proposed resolution

Write a PHP storage backend storing and retrieving code directly from a cache backend. PHP 5.5 allows this be opcode cached. As a mental model, this is a chained cache solution where the slow, distributed cache is provided by Drupal while the local cache is the opcode cache itself provided by the incuded Zend Opcache extension.

Although the hardwired stream wrappers (opcache will cache file:// and phar:// and nothing else) cause a little problem but the solution doesn't require a lot of code just a special stream wrapper barely containing any logic, it is basically just a simple wrapper around the fread/feof etc (except url_stat). CacheStorage is a little tricky but again not heavy at all.

The speed is quite probably close to equal to the atomic counter + file solution since the contents of the file will get opcode cached and the filesystem will cache the stats of the file in question so on warm cache the cost is similar: get an integer out of a distributed store, check stat (for atomic counter, you are looking for existence, my solution compares mtime) and use the opcode cached version onwards. However, this solution does not require any extra code per cache backend, any cache backend is immediately usable as it is without the requirement to have any atomic counters. Other advantage is simply not needing to write the disk. A read only file system has its own advantages and this makes that possible and easy.

Also, since we are using cache we can play with invalidate vs delete later if we want to.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review
FileSize
10.47 KB

The PhpStorageFactory change can be removed , it's just for testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2513326_1.patch, failed testing.

chx’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
10.74 KB

So all the tests have passed except the ones that the temporary override effected so I have changed the override to be for service_container only.

chx’s picture

Issue summary: View changes
Issue tags: +Needs manual testing

Status: Needs review » Needs work

The last submitted patch, 3: 2513326_3.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
10.89 KB
benjy’s picture

Issue tags: -Needs manual testing

Tested this on PHP 5.5.9 with opcache and all seems to be working as expected. Awesome work!

chx’s picture

Issue summary: View changes
Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,188 @@
    +      $bootstrap_php_storage = Settings::get('php_storage_cache_backend');
    +      if (is_callable($bootstrap_php_storage)) {
    +        $this->cacheBackend = $bootstrap_php_storage();
    +      }
    

    So this needs a callback function that returns the cache backend.. makes sense, for arguments and so on...

    Can we test this? write one into settings.php and then verify it's written into the memory bin or something on a page request.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/SpecialCacheStream.php
    @@ -0,0 +1,150 @@
    +
    +class SpecialCacheStream {
    

    Can we come up with a better rename for this? InMemoryCacheStream or something?

  3. +++ b/core/modules/system/src/Tests/Cache/PhpBackendUnitTest.php
    @@ -7,7 +7,9 @@
     
    +use Drupal\Core\Cache\MemoryBackend;
     use Drupal\Core\Cache\PhpBackend;
    +use Drupal\Core\Site\Settings;
     
    

    left-over?

Fabianx’s picture

I like the idea very much (especially getting away from file system, but still being able to use opcache), but it won't solve the coordinated invalidation problem, because the op-cache is obviously only local to the web head itself.

Can / should we somehow sign the PHP code in the Cache Backend?

Berdir’s picture

@Fabian: Isn't that what the mtime stuff is for?

jibran’s picture

Please add the interdiffs it's helpful even with the failing patch. Picked up some doc issues.

  1. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,188 @@
    +    // file is not ideal because all the classes and interfaces in the
    +    // container would need to go through our wrapper. So hijack phar instead.
    

    Capitalization but I think this whole sentence can be re-written.

  2. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,188 @@
    +    #var_dump(opcache_get_status(TRUE)['scripts']["phar://$name"]);
    

    debug code.

  3. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,188 @@
    +   * @param $name
    +   * @return bool|resource
    ...
    +   * @return \Drupal\Core\Cache\CacheBackendInterface
    
    +++ b/core/lib/Drupal/Core/StreamWrapper/SpecialCacheStream.php
    @@ -0,0 +1,150 @@
    +   * @var \Drupal\Core\PhpStorage\CacheStorage
    ...
    +   * @var int
    

    Needs doc love.

  4. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,188 @@
    +    // We do not need a real mtime, we just need a timestamp that changes when
    ...
    +    $mtime = hexdec(substr($hash, 0, 7));
    ...
    +      "$key:mtime" => ['data' => $mtime],
    

    Let's name it timestamp then.

  5. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,188 @@
    +   * @param $filename
    

    @param type missing.

  6. +++ b/core/lib/Drupal/Core/StreamWrapper/SpecialCacheStream.php
    @@ -0,0 +1,150 @@
    +class SpecialCacheStream {
    ...
    +  public static function init($storage, $mtime) {
    ...
    +  public function dir_closedir() {
    ...
    +  public function dir_opendir($path) {
    ...
    +  public function dir_readdir() {
    ...
    +  public function dir_rewinddir() {
    ...
    +  public function mkdir() {
    ...
    +  public function rename() {
    ...
    +  public function rmdir() {
    ...
    +  public function stream_cast() {
    ...
    +  public function stream_close () {
    ...
    +  public function stream_eof() {
    ...
    +  public function stream_flush() {
    ...
    +  public function stream_lock($operation) {
    ...
    +  public function stream_metadata() {
    ...
    +  public function stream_open($path) {
    ...
    +  public function stream_read($count) {
    ...
    +  public function stream_seek($offset, $whence = SEEK_SET) {
    ...
    +  public function stream_set_option() {
    ...
    +  public function stream_stat() {
    ...
    +  public function stream_tell() {
    ...
    +  public function stream_truncate() {
    ...
    +  public function stream_write() {
    ...
    +  public function unlink() {
    ...
    +  public function url_stat() {
    

    Docs missing.

Fabianx’s picture

Ah, I see, the cache is always asked for the 'mtime' when using this. Nice :). And I found the signing of the code now.

chx’s picture

> Can we test this? write one into settings.php and then verify it's written into the memory bin or something on a page request.

We do not test BootstrapConfigStorageFactory either -- but I can change how the unit test is set up.

> Can we come up with a better rename for this? InMemoryCacheStream or something?

Of course. I renamed it quite a few times :) but I'd like to keep the world "special" because it's not usable in a generic sense.

> left-over?

Yes.

> Please add the interdiffs

interdiff from patchutils fails with this, I will do the git dance when I reroll next.

> debug code.

Please check the issue summary.

> Needs doc love.

Agreed.

> Let's name it timestamp then.

Please check url_stat, it's used there as the mtime and that's the only thing it is ever used for so why not call it mtime?

> Docs missing.

From init. The rest will stay undocumented as this is a PHP stream wrapper, what's there to document?

> And I found the signing of the code now.

Yes! dawehner asked for the filename hash to make sure a simple SQLi attack doesn't get turned into a full blown code execution attack. Also, if we want, we can use hash_hmac for the mtime hash as well and in open we can compare this for code signing itself but I do not think there's much of a point in that.

Crell’s picture

+++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
@@ -0,0 +1,188 @@
+    // Load the data from cache.
+    // Hook in the fake phar wrapper. Opcode included in PHP 5.5 hardwires file
+    // and phar as the only two stream wrappers which can be opcode cached.
+    // file is not ideal because all the classes and interfaces in the
+    // container would need to go through our wrapper. So hijack phar instead.
+    stream_wrapper_unregister('phar');
+    stream_wrapper_register('phar', 'Drupal\Core\StreamWrapper\SpecialCacheStream');

Yikes! Clever hack, although the necessity for it is a problem. (And this means we can never use phar files from Drupal. Maybe not a huge loss in practice, but still a loss.)

We should definitely file an upstream issue to let streams opt-in to opcache-support. There's some discussion about overhauling the way streams work in 7.1 so let's get this bug in the right people's ears. (Won't help us until Drupal 9, but at least it would help us then.)

chx’s picture

> And this means we can never use phar files from Drupal.

Why...? The moment loading is done the phar wrapper is restored. So we absolutely can use phar.

Crell’s picture

Ah! So you can, yes. But then, I don't quite understand why the comment says we need to use phar instead of file... (Not against it, I just don't see why it makes a difference if we only keep it in place for one file.)

Still, an upstream issue to avoid this song and dance in the future would be good.

chx’s picture

> Not against it, I just don't see why it makes a difference if we only keep it in place for one file.

First, a reminder: PHP reads /path/to/foo by reading file:///path/to/foo. That's what file:// is.

Now, the include command triggers the reading of the container and parsing the container class, in turn triggers the reading of all the classes and interfaces in extend, implement, typehint (which causes #2408371: Proxies of module interfaces don't work) so it's not exactly one file. Like, you will be reading at least core/lib/Drupal/Core/DependencyInjection/Container.php and core/vendor/symfony/dependency-injection/Container.php because our container class extends that so if we were overriding file:// then we would need to do a lot more of this register-restore dance so that normal files can still be read. It's a major hassle, I tried. The only thing you can't do in here is to use an autoloader which reads the classes and interfaces used in the container from a phar archive.

dawehner’s picture

Issue tags: +needs profiling

Do we have any numbers how this increases/decreases the performance of actual rendered output? I could imagine that for twig templates the additional levels
could be quite an effort.

In general while I in general like the idea of being able to ship PHP like that, I'm worried about the additional level of complexity.

chx’s picture

I am not sure what's there to profile. If you are on a single webhead then use mtime protected storage, that's going to be faster, obviously. If you are on multiple webheads then you can't really use file based storage without some sort of distributed check. So ... profile compared to what?

Fabianx’s picture

For twig this is different, because we have an actual mtime, we would just need to start using it.

So even for twig this can be useful, but instead of the hash() we can use the real file mtime or provide an mtime.

Crell’s picture

chx: Ah, OK. I follow now, thanks.

I agree some benchmarks here are critical to see what we gain here.

Berdir’s picture

What @chx said in #20. This isn't about being *faster* than something else, it's about solving problems like #2301163: Create a phpstorage backend that works with a local disk but coordinates across multiple webheads and #2497243: Replace Symfony container with a Drupal one, stored in cache. Does it actually solve the last one? We should test the scenario of that issue against this.

So there's not much to profile it against except possibly comparing it against those alternative solutions.

I was wondering if it would be worth using chained fast for the cache backend. but if I understand it correctly then it's already optimized to store the modified time separately and we only need to load the full container if that changed, correct? That's really neat and I think the only scenario where ChainedFast would be better is if we would use the bootstrap cache bin, so we can share the check whether the fast storage is still valid, we'd just do it a bit earlier than we do now.

I think for twig, it's fairly save to assume that they don't change unless you also change code (not 100% sure how it works for views and inline templates, those are possibly hashed or so and should be fine too, changing them will just result in a different file?). So if you have multiple servers, you likely also have a deployment process that can empty a folder on each server. So twig, I think, doesn't have to use this.

dawehner’s picture

So there's not much to profile it against except possibly comparing it against those alternative solutions.

Well sure, this is what I tried to point out. Its an alternative solution for the two other issues, so it would be interesting to apply here.

chx’s picture

> Does it actually solve the last one? We should test the scenario of that issue against this.

So the current rebuild is not an atomic operation and we want an atomic operation.

Mine depends on the semantics of setMultiple method on the CacheBackendInterface. In SQL and Redis that's an atomic operation (because SQL uses a transaction and Redis has an atomic MSET) but in memcached it is not. We would need to amend the setMultiple doxygen to say it must be atomic and the memcached project should wrap the set loop in if ($memcached->add(hash('sha256', serialize($items)), 1) so that only one client can succeed in setting the same $items. Then we are good. This would make a lot of sense because currently our setMultiple (as defined) offers no advantage over looping set commands (while the SQL implementation might offer some speedup if the caller is not already inside a transaction this is not something the caller should or could rely on).

chx’s picture

Actually? You don't need any of that. So here's what happens: container rebuild nukes the cache bin. process A misses the mtime from the cache so it begins to rebuild. process B also begins to rebuild. Process A sets the new container code and then sets the mtime but the mtime depends only on the new container code, it's deterministic. benjy even saw opcache keep hitting after a reinstall because of this determinstic property. Process B might set the container code later but it won't make any difference , it's setting the exact same data. The container code is set then mtime is set so if an mtime is read the corresponding container is there, there's no partial rebuild, it's always full.

The stampede in HEAD comes from each rebuilder changing the mtime of the containing dir.

The one thing we need to fix (already fixed locally, not posted yet) is the order in which the items are put in cache and document setMultiple that it needs to keep the order. Much better than requiring an atomic setMultiple.

chx’s picture

FileSize
7.1 KB
11.35 KB

> Still, an upstream issue to avoid this song and dance in the future would be good.

I do not think I want to do that. Please, feel free, but I won't.

Addressed all other feedback. Even rolled an interdiff.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
@@ -0,0 +1,191 @@
+      $bootstrap_php_storage = Settings::get('php_storage_cache_backend');
+      if (is_callable($bootstrap_php_storage)) {
+        $this->cacheBackend = $bootstrap_php_storage();
+      }

I am wondering if instead of global settings this should be a part of the $this->configuration.

Status: Needs review » Needs work

The last submitted patch, 27: 2513326_27.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
11.65 KB
chx’s picture

FileSize
11.72 KB
3.08 KB

benjy asked how the factory will get access to the configuration array, most importantly the bin variable. Excellent question. Refactored the class to allow for that and the test to show how it's done.

chx’s picture

FileSize
2.34 KB
11.85 KB

Further simplification, doxygen to __construct, a minor fix to exists.

benjy’s picture

+++ b/core/tests/Drupal/Tests/Core/PhpStorage/CacheStorageTest.php
@@ -31,7 +31,7 @@ public function testCRUD() {
+      'cache_backend_factory' => function (array $configuration) { return new MemoryBackend($configuration['bin']);},

:) great!

  1. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,199 @@
    +      if (isset($this->configuration['cache_backend_factory']) && is_callable($this->configuration['cache_backend_factory'])) {
    +        $this->cacheBackend = call_user_func($this->configuration['cache_backend_factory'], $this->configuration);
    +      }
    +      else {
    +        $this->cacheBackend = static::getDatabaseBackend($this->configuration);
    +      }
    

    If something isn't callable, would it be better to just let this blow up rather than falling back to the database backend? My thinking is, $configuration['cache_backend_factory'] is only ever going to be set if a developer attempts to override this?

  2. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,199 @@
    +  protected static function getDatabaseBackend($configuration) {
    +    $connection = Database::getConnection();
    +    return new DatabaseBackend($connection, new DatabaseCacheTagsChecksum($connection), 'php_' . $configuration['bin']);
    

    I can't see any reason for this to be static?

  3. +++ b/core/lib/Drupal/Core/PhpStorage/PhpStorageFactory.php
    @@ -48,6 +48,9 @@ static function get($name) {
    +    if ($name == 'service_container') {
    +      $class = 'Drupal\Core\PhpStorage\CacheStorage';
    +    }
    

    Was we going to remove this and then update the relevant tests?

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/SpecialInMemoryStream.php
    @@ -0,0 +1,164 @@
    +/**
    + * The stream wrapper for \Drupal\Core\PhpStorage\CacheStorage .
    + *
    + * This class is not usable as a generic stream wrapper.
    + */
    +class SpecialInMemoryStream {
    

    If we're going with SpecialInMemoryStream, maybe we should explain why it's special. And if we're saying it is not usable as a generic stream wrapper, lets say when/what it is used for currently?

chx’s picture

FileSize
5.11 KB
10.43 KB

> If something isn't callable, would it be better to just let this blow up

Sure.

> I can't see any reason for this to be static?

Following Wim Leers, I am making everything static that does not specificaly need $this to show it is independent of state

> Was we going to remove this and then update the relevant tests?

We still need to see what we will do. I doubt we want to default to this but I will profile later today. If we do default to this then yes we need to update at least the PhpStorage cache test.

> If we're going with SpecialInMemoryStream, maybe we should explain why it's special. And if we're saying it is not usable as a generic stream wrapper, lets say when/what it is used for currently?

Done.

I have removed the unnecessary methods; it's not implementing an interface so why bother?

twistor’s picture

I have removed the unnecessary methods; it's not implementing an interface so why bother?

Yup. That is correct, PHP will handle unimplemented stream wrapper methods automatically.

chx’s picture

Re upstream: in light of recent events where PHP internals people have actually treated me decently and were very helpful I have emailed the internals list. I gave up years ago trying to talk to that list because we live in so different worlds but let's try once more.

Crell’s picture

Following Wim Leers, I am making everything static that does not specificaly need $this to show it is independent of state

... I don't know why you're listing that as "following Wim Leers", but that's not how most of core is written. Non-static methods are fine. Don't confuse matters by having methods be a mix of contexts. There is no value in doing so other than inconsistency.

chx’s picture

> I don't know why you're listing that as "following Wim Leers"

Because Wim told me :)

[Thursday, January 08, 2015] [16:54:48] <WimLeers>      "static" indicates that a method is not going to modify the object state. I find that a very helpful guarantee
[Thursday, January 08, 2015] [16:56:15] <chx>   to me static means that it stays alive across instantiations
[Thursday, January 08, 2015] [16:56:23] <chx>   so i always wonder when i saw a static call
[Thursday, January 08, 2015] [16:56:42] <WimLeers>      that's because static variables
[Thursday, January 08, 2015] [16:56:53] <WimLeers>      if you don't use static variables, there's no problem

So that's that. I found this a very compelling argument and follow it.

dawehner’s picture

Yeah I also never was a fan of indicating it with static. Especially on interfaces it limits the implementation. Feel free to skip it.

chx’s picture

Title: Create a PHP storage backend directly backed by cache » Performance: create a PHP storage backend directly backed by cache
Priority: Normal » Critical
Issue tags: -needs profiling
FileSize
83.63 KB

On a hit this backend is faster than the current MTimeProtectedFileStorage default. Raising to critical.

Edit: and it can be made even faster by using memcached.

chx’s picture

Issue summary: View changes
dawehner’s picture

This would be amazing, but well I fear its more of a random fluctuation.
The first thing I see is the total walltime of 300ms, is that the default frontpage with 50 nodes? Given that 1% would be quite an improvement on there, especially because its on warm caches.

I did some benchmarks using xhprof-kit, a frontpage with 1 node, page_cache disabled and anonymous user.

Database cache backend

### FINAL REPORT

=== 8.0.x..8.0.x compared (559627488b7d6..5596275e247de):

ct  : 29,492|29,492|0|0.0%
wt  : 70,993|71,602|609|0.9%
mu  : 16,673,168|16,673,136|-32|-0.0%
pmu : 16,702,384|16,702,352|-32|-0.0%

=== 8.0.x..2513326 compared (559627488b7d6..55962771a4a31):

ct  : 29,492|29,785|293|1.0%
wt  : 70,993|79,660|8,667|12.2%
mu  : 16,673,168|18,335,632|1,662,464|10.0%
pmu : 16,702,384|18,362,720|1,660,336|9.9%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           |     29,785 |     32,112 |     29,808 |     29,785 |     29,785 |
| 8_0_x             |     29,492 |     31,819 |     29,515 |     29,492 |     29,492 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           |     79,660 |    103,230 |     83,765 |     83,267 |     91,216 |
| 8_0_x             |     71,602 |     95,208 |     75,529 |     74,496 |     80,585 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           | 18,335,592 | 18,860,808 | 18,367,386 | 18,335,592 | 18,726,000 |
| 8_0_x             | 16,673,112 | 17,217,448 | 16,716,037 | 16,673,168 | 17,066,840 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           | 18,362,720 | 18,990,304 | 18,395,451 | 18,362,800 | 18,751,768 |
| 8_0_x             | 16,702,328 | 17,349,128 | 16,746,120 | 16,702,384 | 17,094,480 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

Using the apcu cache backend, which more or less "simultations" memcache

Note, its clear that the opcache which kicks in would not cause a big difference.

### FINAL REPORT

=== 8.0.x..8.0.x compared (55962c79576d6..55962c9a45788):

ct  : 29,492|29,492|0|0.0%
wt  : 69,457|69,344|-113|-0.2%
mu  : 16,673,168|16,673,168|0|0.0%
pmu : 16,702,384|16,702,384|0|0.0%

=== 8.0.x..2513326 compared (55962c79576d6..55962cb0780d9):

ct  : 29,492|29,725|233|0.8%
wt  : 69,457|76,879|7,422|10.7%
mu  : 16,673,168|18,324,672|1,651,504|9.9%
pmu : 16,702,384|18,351,944|1,649,560|9.9%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           |     29,725 |     32,052 |     29,748 |     29,725 |     29,725 |
| 8_0_x             |     29,492 |     31,819 |     29,515 |     29,492 |     29,492 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           |     76,879 |     96,717 |     79,642 |     77,932 |     85,377 |
| 8_0_x             |     69,344 |    100,033 |     73,050 |     72,108 |     76,828 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           | 18,324,584 | 18,865,688 | 18,365,294 | 18,324,672 | 18,715,168 |
| 8_0_x             | 16,673,032 | 17,228,472 | 16,705,745 | 16,673,168 | 17,066,704 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513326           | 18,351,696 | 18,994,896 | 18,393,530 | 18,351,944 | 18,741,032 |
| 8_0_x             | 16,702,288 | 17,360,728 | 16,735,790 | 16,702,384 | 17,094,376 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

Summary

I don't see this higher numbers

Fabianx’s picture

Priority: Critical » Major

Wow, 8 ms slower?

Berdir’s picture

Seeing quite different results. I was testing the page cache with ab, because we only care about bootstrap, anything after that is just noise.

I first saw a similar difference, page cache response was 12ms instead of 5.

Then I disabled xdebug and I'm not really seeing a difference anymore. Was between 4 and 4.5ms for both of them. I also looked with xhprof, and there's nothing that's jumping out. Database connection happens earlier ( didn't even bother using something like the apc backend directly) but it has to happen anyway for me for the page cache cache get.

Berdir’s picture

Some more tests, there does seem to be a small difference. With ab -n 5000 -c 1, I'm seeing 4.0-4.1ms for HEAD, with this patch, it's 4.4-4.6ms.

chx’s picture

FileSize
10.42 KB
534 bytes

benjy found that the latest PHP 5.6 broke this, I fixed it. (Edit: I needed gdb to find this one out. That was hard.)

Also, sorry for getting carried away and making this critical. So should we use this or the atomic counter version?

chx’s picture

FileSize
2.33 KB
11.05 KB

Commented the unusual methods of the class heavily.

chx’s picture

Issue summary: View changes
FileSize
4.36 KB
12.38 KB

Now that we are on 5.5 we can unit test whether opcache hits.

Status: Needs review » Needs work

The last submitted patch, 48: 2513326_48.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
12.53 KB
1019 bytes
chx’s picture

FileSize
2.6 KB
11.68 KB

Disregard #50 please.

The last submitted patch, 50: 2513326_50.patch, failed testing.

chx’s picture

FileSize
13.21 KB

We could combine these approaches which besides the cacheability test is rather useful to keep CacheStorage very strongly tested: instead of MemoryBackend I use a mock class where every method is counted for.

chx’s picture

FileSize
13.45 KB

Finally, let's make sure nothing else on the cache is being called. "Only this and nothing more"

anavarre queued 54: 2513326_54.patch for re-testing.

anavarre’s picture

Issue tags: +Performance, +scalability
klausi’s picture

Patch does not fully apply anymore for the test files, but I could get this working with this in settings.php:

$settings['php_storage']['default'] = array(
        'class' => 'Drupal\Core\PhpStorage\CacheStorage',
        'secret' => 'EiNithe8niechi2rohPhou9thia2ie',
      );

I tested this against the container rebuilding stampede from #2497243: Replace Symfony container with a Drupal one, stored in cache, but this does not solve our problem of too many container rebuilds. I added this to your CacheStorage::save() method for debugging:

file_put_contents('/home/klausi/workspace/drupal-8/sites/default/files/debug.txt', time(). ": $name\n", FILE_APPEND);

That adds a line to the debug file every time the container is saved to the database. When truncating the cache_php_service_container DB table and then running

ab -n 100000 -c 100 http://drupal-8.localhost/

it still builds the container 100 times, so this patch does not stop the rebuild stampede.

chx’s picture

Thanks for the test!

> it still builds the container 100 times, so this patch does not stop the rebuild stampede.

Of course: you started 100 concurrent issues which started 100 builds. But it's only 100 so the builds finish unlike with mtime where it doesn't. To remove the stampede we would need locking that's a different issue. The critical issue is that the stampede causes the build to last forever. Ie: try a similar statement in HEAD, it'll be a lot more than 100.

olli’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.php
    @@ -120,6 +120,7 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
        *   @endcode
    +   *   Backend implementations must keep the order of items.
        */
       public function setMultiple(array $items);
    

    Don't we have tests for this..?

  2. +++ b/core/lib/Drupal/Core/PhpStorage/CacheStorage.php
    @@ -0,0 +1,198 @@
    +  public function open($name) {
    +    $key = $this->getKeyFromFilename(substr($name, 7));
    +    if (!$cached = $this->cacheBackend()->get($key)) {
    +      return FALSE;
    +    }
    +    // Copy it into a file in memory.
    +    if (!$handle = fopen('php://memory', 'rwb')) {
    +      return FALSE;
    +    }
    +    if (fwrite($handle, $cached->data) === FALSE || fseek($handle, 0) === -1) {
    ...
    +   * By using a secret key, a SQL injection does not lead immediately to
    +   * arbitrary PHP inclusion.
    +   *
    +   * @param string $filename
    +   *   The filename.
    +   *
    +   * @return string
    +   *   The secret hash.
    +   */
    +  protected function getKeyFromFilename($filename) {
    +    return hash_hmac('sha256', $filename, $this->configuration['secret']);
    +  }
    

    Silly question: what kind of sql injection this prevents? It looks like it only makes the cid column value hard to guess.

chx’s picture

> Silly question: what kind of sql injection this prevents? It looks like it only makes the cid column value hard to guess.

If you have an SQL injection bug somewhere else then you can't just write a query out of the blue that writes PHP code to be included -- you need the secret for that and presumedly it's not in the database. It's a small bit of protection when you are already hacked but every bit helps.

chx’s picture

Status: Needs review » Closed (won't fix)

The fun continues in https://www.drupal.org/project/phpstorage and #2550311: Allow extending PhpStorageTestBase. I will take a look whether we have a test for setMultiple and getMultiple keeping order and file an issue accordingly either with just the doxygen or a test.