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
Comment | File | Size | Author |
---|---|---|---|
#54 | 2513326_54.patch | 13.45 KB | chx |
#51 | 2513326_51.patch | 11.68 KB | chx |
#51 | interdiff.txt | 2.6 KB | chx |
#48 | 2513326_48.patch | 12.38 KB | chx |
#48 | interdiff.txt | 4.36 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedThe PhpStorageFactory change can be removed , it's just for testing.
Comment #3
chx CreditAttribution: chx commentedSo 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.
Comment #4
chx CreditAttribution: chx commentedComment #6
chx CreditAttribution: chx commentedComment #7
benjy CreditAttribution: benjy at CodeDrop commentedTested this on PHP 5.5.9 with opcache and all seems to be working as expected. Awesome work!
Comment #8
chx CreditAttribution: chx commentedComment #9
BerdirSo 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.
Can we come up with a better rename for this? InMemoryCacheStream or something?
left-over?
Comment #10
Fabianx CreditAttribution: Fabianx as a volunteer commentedI 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?
Comment #11
Berdir@Fabian: Isn't that what the mtime stuff is for?
Comment #12
jibranPlease add the interdiffs it's helpful even with the failing patch. Picked up some doc issues.
Capitalization but I think this whole sentence can be re-written.
debug code.
Needs doc love.
Let's name it timestamp then.
@param type missing.
Docs missing.
Comment #13
Fabianx CreditAttribution: Fabianx as a volunteer commentedAh, I see, the cache is always asked for the 'mtime' when using this. Nice :). And I found the signing of the code now.
Comment #14
chx CreditAttribution: chx commented> 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.
Comment #15
Crell CreditAttribution: Crell as a volunteer commentedYikes! 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.)
Comment #16
chx CreditAttribution: chx commented> 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.
Comment #17
Crell CreditAttribution: Crell as a volunteer commentedAh! 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.
Comment #18
chx CreditAttribution: chx commented> 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.
Comment #19
dawehnerDo 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.
Comment #20
chx CreditAttribution: chx commentedI 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?
Comment #21
Fabianx CreditAttribution: Fabianx as a volunteer commentedFor 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.
Comment #22
Crell CreditAttribution: Crell as a volunteer commentedchx: Ah, OK. I follow now, thanks.
I agree some benchmarks here are critical to see what we gain here.
Comment #23
BerdirWhat @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.
Comment #24
dawehnerWell 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.
Comment #25
chx CreditAttribution: chx commented> 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).Comment #26
chx CreditAttribution: chx commentedActually? 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.
Comment #27
chx CreditAttribution: chx commented> 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.
Comment #28
Fabianx CreditAttribution: Fabianx as a volunteer commentedI am wondering if instead of global settings this should be a part of the $this->configuration.
Comment #30
chx CreditAttribution: chx commentedComment #31
chx CreditAttribution: chx commentedbenjy 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.
Comment #32
chx CreditAttribution: chx commentedFurther simplification, doxygen to __construct, a minor fix to exists.
Comment #33
benjy CreditAttribution: benjy at CodeDrop commented:) great!
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?
I can't see any reason for this to be static?
Was we going to remove this and then update the relevant tests?
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?
Comment #34
chx CreditAttribution: chx commented> 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?
Comment #35
twistor CreditAttribution: twistor as a volunteer commentedYup. That is correct, PHP will handle unimplemented stream wrapper methods automatically.
Comment #36
chx CreditAttribution: chx commentedRe 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.
Comment #37
Crell CreditAttribution: Crell as a volunteer commented... 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.
Comment #38
chx CreditAttribution: chx commented> I don't know why you're listing that as "following Wim Leers"
Because Wim told me :)
So that's that. I found this a very compelling argument and follow it.
Comment #39
dawehnerYeah I also never was a fan of indicating it with static. Especially on interfaces it limits the implementation. Feel free to skip it.
Comment #40
chx CreditAttribution: chx commentedOn 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.
Comment #41
chx CreditAttribution: chx commentedComment #42
dawehnerThis 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
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.
Summary
I don't see this higher numbers
Comment #43
Fabianx CreditAttribution: Fabianx as a volunteer commentedWow, 8 ms slower?
Comment #44
BerdirSeeing 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.
Comment #45
BerdirSome 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.
Comment #46
chx CreditAttribution: chx commentedbenjy 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?
Comment #47
chx CreditAttribution: chx commentedCommented the unusual methods of the class heavily.
Comment #48
chx CreditAttribution: chx commentedNow that we are on 5.5 we can unit test whether opcache hits.
Comment #50
chx CreditAttribution: chx commentedComment #51
chx CreditAttribution: chx commentedDisregard #50 please.
Comment #53
chx CreditAttribution: chx commentedWe 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.
Comment #54
chx CreditAttribution: chx commentedFinally, let's make sure nothing else on the cache is being called. "Only this and nothing more"
Comment #56
anavarreComment #57
klausiPatch does not fully apply anymore for the test files, but I could get this working with this in settings.php:
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:
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
it still builds the container 100 times, so this patch does not stop the rebuild stampede.
Comment #58
chx CreditAttribution: chx commentedThanks 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.
Comment #59
olli CreditAttribution: olli commentedDon't we have tests for this..?
Silly question: what kind of sql injection this prevents? It looks like it only makes the cid column value hard to guess.
Comment #60
chx CreditAttribution: chx commented> 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.
Comment #61
chx CreditAttribution: chx commentedThe 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.