Use fast, local cache back-end (APCu, if available) for low-write caches (bootstrap, discovery, and config)

Bumping to major and tagging 'Fast by default'.

The two bins we want this for are config and discovery. We could hard code those bins in the factory, or somehow try to indicate per-bin whether it can use local cache or not. If we want the latter this could be yet another follow-up.

In irc beejeebus mentioned an intelligent fallback based on environment (i.e. check for apc support and use that, fall back to phpstorage if not). That seems OK. If you configure stuff that code won't kick in at all.

The problem with using APC (u) by default might be that the default APC size of 32MB could be too small, but likely less a problem with 5.5 and APCU.

We could potentially even check the size?

Yeah, maybe, possibly as a separate hook_requirements warning or so? As it's not specific to caching, using apc/opcode for the files alone is going to fill up those 32MB *very* quickly...

#1807662: Built-in APCu support in core (PHP 5.5 only) just landed, now let's get this done!

Something else to consider is that this could have unexpected side effects because it would be something else you have to empty when you want to re-install for example. Very bad things will happen if you start a new installation and then run into existing APC caches :) Good luck trying to explain to somehow who has never heard of APC that he has to restart apache now when he wants to re-install Drupal... and on shared hosting, might not even be able to do so?

We don't enable the APC classloader by default either, because that can be very nasty too if you're running into weird behavior if it tries to load classes that no longer exist and so on.

I'm considering that it might be a better option to provide a production.settings.local.php (and the existing one should be development.settings.local.php ;)) with explanations for things that should/can be enabled on a production site, this would also give us a nice place to document currently undocumented settings like the page_cache_without_db/config setting.

I'm all for fast (by default) but we need to consider the costs of it, in some cases, faster-with-three-mouseclicks might be preferrable :)

To address #8, I'd suggest to simply make the very first installer screen force-empty the cache.

The new APCu cache backend uses the (hashed) site directory/path as global key prefix already, so that prefix will remain the same when re-installing (into the same conf/site path).

Ideally, we'd have an algorithm like CoreServiceProvider::registerUuid() in CacheFactory (or a more adequate place, if there is one) — i.e., if there is no manually configured settings.php setting, then we automatically figure out the best / most performant implementation that is available.

I think we're definitely able to resolve the case of clearing external caches upon re-installation - as long as such external storages are using a reliable, site-specific prefix that is based on the conf/site directory (as opposed to hash_salt or similar, which would change with every installation and thus be unknown upon re-installation). Even confident enough to recommend to defer/postpone that topic to later / separate issue.

The rebuild script could (probably has to) also clear the entire APC cache for a site - that way any stale APC issue is recoverable from

+1 to that "dumb" (== KISS) solution, in case it works.

Originally thought of putting that logic into CoreServiceProvider, but embedding the default/fallback negotiation directly into CacheFactory makes some good level of sense, too.

We should limit this to discovery and maybe bootstrap cache bins (which is currently almost empty) I think? We don't want to default to APC for things like the render cache because that can get huge?

Test is already running 1h25 on testbot, that testbot usually finishes in 52-53 minutes... so that's not exactly good. Either a Undecillion test fails/exceptions or this is not actually making anything faster. One downside of this is that test methods and web requests have to maintain their own separate caches, so all the plugin discovery stuff is now happening separately for web requests...

I was going to suggest we add this to a settings file but after reading, looks like berdir already did that in #8. To me that seems more sensible, it seems the main reason to enable this by default is to try and earn the accolade 'faster than D7 out of the box'? If we have to do stuff like this to do that, it seems a bit false.

I think there would be more random issues than we can foresee now if we try to enable this by default. That is not including the need to restrict this to particular bins, which adds more unnecessary hard-coded complexity to a cache class. Settings can handle this per bin configuration already.

13: 2248767-13.patch queued for re-testing.

Looks like the test bot had some trouble, we're not sure if it's the patch or testbot.

I applied locally and did run tests through CLI and it doesn't halt like testbot but I'm seeing a lot of test fails, seems to be failing at some point, especially for tests with multiple test methods.. tested with "Drupal\block\Tests\BlockTest" for example. Works in fine in browser, 30 fails in CLI.

Important: If you run test, make sure to set apc.enable_cli = 1 in php.ini/apc.ini, otherwise it fails with fatal error class APCIterator not found (on 5.5 apcu)

Will try one more time to see if it was the bot (both runs were on the same bot).

13: 2248767-13.patch queued for re-testing.

iirc we have some tests that do things in the UI then check the cache API directly, I can't see those passing with APCu without #2231595: Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend.

Yeah, invalidation headaches between web and cli look insurmountable when not using #2231595: Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend. Sadly, I think this is a Won't Fix. I'd be happy if someone can propose a way forward. In that case, please reopen this.

This issue started off as a plea to default to the APC classloader when APC is available. Shall we open a new issue for that, in the spirit of Fast by Default?

#2231595: Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend was opened partly to enable this one. I don't see how this is insurmountable once that's in, so re-opening but postponing.

Postponed is fine.

My thinking is that CacheChain will only be used on multi-webhead sites and those are sufficiently complex that auto-detection is not that important. I guess one could use CacheChain on single webhead sites to get past the CLI/Web problem mentioned here. Not sure the complexity tradeoff is worth it.

I guess one could use CacheChain on single webhead sites to get past the CLI/Web problem mentioned here.

Well if we default to it for configuration and discovery, then single webhead sites will be able to default to reading from APC, rather than the database, while nothing will break on multiple web heads.

I somewhat opposed this in #27 but I support it now. I see the consistency and speed benefits of using cache chain by default, even on single webserver sites. This approach is also compatible with Drush even though Drush has to maintain a separate APC cache from the webserver.

This is all overridable from Settings which is good. We don't do much writing to settings.php from installer if we can help it. I don't see much benefit to adding that here.

+1 from me. Lets fix those test fails.

berdir suggested writing this out to settings.php during install. sun suggested something about putting this in the container during build.

I prefer sun's idea. Just as Moshe does in #32. How's this for an implementation?

Right. Makes sense. Service references need only a leading @, but parameter references need both leading and trailing %. Oh, Symfony!

i'd like to also add the configuration cache to this, but we don't use CachedStorage for config anymore, so that would make this a more complicated patch.

Possibly not very complicated. Let's see if this works.

This one is just to compare which failures are due to APCu and which are due to something else.

I'll need to open a separate issue for this language fix, but for now, just wanting to make some testbot progress.

Now that those failures are fixed, back to APCu.

Thanks! I'll take a look to see if I can make sense of the remaining failures, but meanwhile: #2311945: Add ResettableCacheBackendInterface, which despite the title, is related to the timestamp clearing during tests.

Hm. I don't think that any cache backend implementation should catch exceptions; it's going to hide real application errors caused by objects that cannot be unserialized. In addition, PHP Exceptions are very slow; regular runtime code is not supposed to throw exceptions.

+      catch (\Exception $e) {
+        // It's possible for cache items stored in the fast cache to implement
+        // PHP's object __wakeup() method, and throw exceptions. Treat an
+        // exception as 'we didn't find anything in the fast cache'.
+        $cids = $cids_copy;
+      }

@sun: What's the alternative? The challenge with ChainedFastBackend is that we *know* that the fast backend might return an invalid entry. Once we retrieve it, we check its timestamp and throw it out if it's invalid (doesn't match the timestamp of the slow backend), but we can only do that check after retrieving it. So we have a situation where an unserialize() is getting called (in this case by apc_fetch() internally, but with a different backend, could be with an explicit call to unserialize()) on invalid data. We can't make our __wakeup() implementations not throw any exceptions on all possible states of invalid data, can we? As far as performance, if the cost is only when one is thrown, then it's not "regular" runtime code; it's runtime code that happens after a cache has been invalidated in one place, but the invalidation hasn't propagated to all the web nodes yet (or in the case of tests, from the CLI process to the web process).

Also, re-adding related issue that appears to have been removed in #56 due to an xpost.

Also:

it's going to hide real application errors caused by objects that cannot be unserialized

No, because the catch is only for the fast backend. So throwing that one out, we proceed to check the consistent backend, which is the only authoritative one, so if there's something there that can't be unserialized, there's nothing in this patch that masks it.

Now that we have a green patch here, here's the spin-off I promised in #49: #2312189: Harmonize configuration collection names and language codes.

Also, it's probably time to post some profiling numbers to justify this patch. Anyone up for that?

Agreed on the exception catching, although it might be worth putting part of the answer in #59/#60 into the inline comment.

Discussed the timestamp ttl with beejeebus in irc and that works for me, also 300ms should mean that decently optimized requests finish before they get there anyway, once you're over that an extra call to the consistent backend is not your main problem.

#2313053: Field storage and instance call toArray() in __wakeup() is very slow, remove it?

Recreated anonymous user, front page with two articles, one comment on each. Search, recent content, recent comments, who's new blocks enabled.. Then did profiling.

HEAD

Lowest median out of 5 runs of ab -n 1 -c 100 http://domain: 228 ms/request.

Fastest out of 5 XHProf profiles: 329 ms

Patch

Lowest median out of 5 runs of ab -n 1 -c 100 http://domain: 204 ms/request.

I have to add that the standard deviation with this patch applied is significantly lower. I can consistently reproduce this number, which is not the case for HEAD.

Fastest out of 5 XHProf profiles: 314 ms

So ab shows it's ~10% faster (in the given scenario), XHProf shows it's ~5% faster.

AFAICT profiling was the last thing before this could be RTBC. I was able to confirm the performance improvement.

(My numbers are very different from beejeebus', but that can most likely be attributed to his environment; his responses take roughly twice as long. What matters most is that this is a repeatably measurable (and significant) performance improvement.)

Numbers by themselves don't mean anything, but the comparison before/after the patch is. By reducing the number of remote backend queries it will reduce concurrency problems in highly stressed environments, and this cannot be measured by neither of XHProf or apache benchmark. But my guess is that it will definitely be a huge benefit when used for caches that are not written too often.

Off-topic but still related, I'm really curious about seeing how Drupal 8 will behave on very high volume sites, or with a lot of logged in users, my guess is not that well, so this kind of patch is really necessary IMHO.

I think this is RTBC.

It's actually possible that xhprof is making the APC function calls slower than they actually are, so that might explain some of the difference.

Also, doing testing like this is not too useful here. What you are testing is the mysql query cache, likely over a socket, and if MySQL doesn't have anything else to do, then that can be very fast. (you are testing apc vs. mysql, so that is faster, but memcache/redis vs. mysql will not make a big difference when profiling). That will change a lot if you have a site under load with large cache tables.

So, what's more interesting IMHO to compare is how many (cache) queries we save because of this, so query count before/after.

ab: Are you sure you did -n 1 -c 100 and not the other way round? What might be useful to test might be something like -n 500+ -c 10-20, then you will actually have some parallel load. But you're mostly testing your specific environment at that point.

Thanks for the numbers and the RTBC! I'd like to add in a few more code comments and @todo links to followups before this lands though. Will do so today.

Ok, done adding all the comments I wanted to. Hopefully, they're straightforward and this can be re-RTBC'd quickly.

+ $this->fastService = function_exists('apc_fetch') ? 'cache.backend.apcu' : 'cache.backend.null';

D8.

I thought we were going to conditionally set the cache backend to chained based on APCu being available or not. Whereas the patch is always using the chained backend but falling back to the null service. This means we'll be doing things like getting the timestamp on sites that don't have APCu available which will actually make things worse?

Yes, he is. Sorry for the premature RTBC.

Fixes #74/#75.

Oh, I should point out that there is still one way in which #78 is worse than HEAD for people without APCu: it turns on caching for config, so on cache hits, it's neutral (a db query either way), but on cache misses, it's 2 queries (more if you then also include the set()s) instead of 1. However, cache misses on config should be rare.

Seems there's no clean way to use other cache backends (supposing xcache) without overriding a set of files still :(

@andypost: this patch doesn't change that everything is still overriddable via settings.php. I.e., you can do:

$settings['cache']['default'] = 'cache.backend.xcache';

if you want all cache bins using that (assuming you also have a module that implements that service, since core doesn't ship with an xcache backend implementation).

Or, if you want xcache as your fast backend in a chained implementation, you can do:

$settings['cache']['chained_fast_cache']['fast'] = 'cache.backend.xcache';

I don't think we have that latter one documented yet, and we may want to move that variable out of $settings['cache'], since 'chained_fast_cache' is not a bin, but those are pre-existing issues in HEAD not affected by this patch.

I was going to mirror what catch pointed out but he beat me to it, and it's already been fixed :) Using the null backend is...ugh.

1. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -27,13 +27,30 @@ class CacheFactory implements CacheFactoryInterface,  ContainerAwareInterface {
   +   * @param array $bin_default_backends
   

   Would $default_bin_backends be a better name?

2. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -102,14 +130,47 @@ public function get($cid, $allow_invalid = FALSE) {
   +      catch (\Exception $e) {
   

   We want to catch ANY exception here? Seems like a potential black hole.

3. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -6,11 +6,57 @@
   +  function __construct(Settings $settings) {
   

   public

4. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -6,11 +6,57 @@
   +    if (isset($cache_settings['chained_fast_cache']) && is_array($cache_settings['chained_fast_cache'])) {
   

   Why do we need to check is_array() here? I would rather see a notice if someone tried to configure this setting incorrectly (Like using a string there) rather than silent failure.

5. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -22,24 +68,19 @@ class ChainedFastBackendFactory extends CacheFactory {
   +      $backend = new ChainedFastBackend(
   +        $backend,
   

   Overriding the same variable gets a bit messy in situations like this IMO. How about just return ChainedFastBackend(..) directly?

Minor typo, nice to fix if it needs another iteration.

+++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
@@ -19,9 +19,11 @@
+ * Singe-node configurations that don't have that limitation can just use the

*Single

This fixes #82.1 and #82.3 in other places in the patch that weren't explicitly called out. It also fixes #86 and another miscellaneous typo.

I agree with #84's answer to #82.2, and this is commented at length (perhaps too much length?) in the patch. Suggestions on how to improve that comment are welcome.

Other than us possibly wanting to improve the long comments in ChainedFastBackend::getMultiple(), I think this is RTBC, but since I submitted some of the patches since #75, it's not for me to mark it as such.

damiankloip asked from some logging, but i'm not sure i agree, so i left it out for now.

I don't see the point of logging either, but perhaps I'm missing something? Per #84, if the item is stale, then the only reason it's still in the fast cache is because a delete hasn't propagated yet, and that's by design, so why would we be interested in having a log of that condition?

I looked over the code and it looks swell to me.

So this patch will make sites without APCu slower wrt to the config system because we're maintaining both {config} and {cache_config} tables - and we get a cache table for each config collection so multilingual will be even worse. I'm +1 to using an APCu cache for config - but I don't think it should be chained whilst config is in the database. So this then brings us to an interesting issue - can we use APCu to cache but back on to a non cache table? I.e. how do we cope with invalidation and using a timestamp to check for consistency. I'm not sure. Perhaps the best solution is to duplicate all the data in database cache bins as we do with the current patch and accept that. Hmmm.

I'm not setting this back to needs work for now - leaving this comment to explain why I have not committed it yet.

Also, as stated in its doc, the chained/apcu backend is great for cache bins with a stable set of entries (e.g config, plugin discovery data), but not so much for bins with ever-growing entries (e.g entity cache or render cache), where new entries constantly invalidate the local caches (and also possibly fill up memory, discarding data for other bins ?)

Thus, is it a good idea to make this backend the default for all bins ?

I'm fine with the extra database writes on sites that are using the database backend with no APCu. The config and discovery cache bins should be very, very low-write (less than any 7.x cache table since all of those mixed at least two of config, discovery, state or content), and we already have caches which are a 1-1 swap for a database query to make those easier to offload when you want to.

#93 answers #92, but the title is misleading and probably led yched to write what he wrote in #92. Updating the title.

Yup, my bad, but new title helps :-)
Thanks @beejeebus & @Wim

Whilst updating the issue summary I realised the following code in \Drupal\Core\Config\CachedStorage::construct is going to be problematic.

    if ($collection == StorageInterface::DEFAULT_COLLECTION) {
      $bin = 'config';
    }
    else {
      $bin = 'config_' . str_replace('.', '_', $collection);
    }
    $this->cache = $this->cacheFactory->get($bin);

Since the bin name of configuration depends on the config collection (with respect to core collections handle language overrides for config)

I wonder if the best approach here is not to use tags for the this but change the signature of CacheFactory::get($bin); to CacheFactory::get($bin, $default_backend = 'cache.backend.database');

And then in \Drupal\Core\Config\CachedStorage::construct

$this->cache = $this->cacheFactory->get($bin, 'cache.backend.chainedfast');

Sorry if everyone feels like I'm derailing the issue if the solution attached is not right feel free to rtbc #87 again.

Sorry if everyone feels like I'm derailing the issue if the solution attached is not right feel free to rtbc #87 again.

I'd prefer to do #87, so reuploading it and RTBCing it per the above quote.

I actually think we should change CachedStorage to not create a separate cache bin per collection, but put everything into the config bin, and put the collection name into the cache key instead. Because we have code like Cache::getBins() which assumes that all bins are known by the service container, so CachedStorage creating dynamic bins on the fly seems problematic. But we probably need to discuss that more in a separate issue.

Well we can't put collection into cache key because of cache key length restrictions.

We no longer have cache key restrictions, the cache backend automatically hashes the key if it is too long.

#99 is green again, and #101 answers #100, so back to RTBC.

As an alternative to #113, how about this instead? Interdiff is relative to #87/#99.

Typo fix.

Created #2326203: Config's cached storage should only use one bin to deal with the additional cache bins being created by CachedStorage.

I think #120 will help us resolve the last test failures here (because that issue makes CachedStorage use the actual cache.config service, rather than some anonymous object that isn't accessible for WebTestBase to reset), so postponing this on that.

#2326203: Config's cached storage should only use one bin just landed.

For starters, just a reroll, but assigning to myself because I want to make a couple other changes as a result of #122.

Because of #122, we no longer generate a new cache table for each collection, which makes the LanguageConfigFactoryOverride changes no longer strictly necessary for this issue, so I pulled it out of here. It's still in #2312189: Harmonize configuration collection names and language codes, where we can do it completely.

Also:

if i add a sleep() or reduce the ttl to 0.15 it passes.

Not happy with failures popping up when testbot or our tests become faster, so opened #2327013: Add a TTL to ChainedFastBackend to prevent long running requests from using data that is too stale and removed those changes from here, to verify that the reset() in WebTestBase is working in every case that it needs to.

Excellent! The 1 failure in #124 was being masked in #123 with the TTL, but is now uncovered. It's a strange one though: it happens right after $this->moduleHandler->uninstall(array('config_test')) which ends with a drupal_flush_all_caches(), but somehow, right after that, we still have a ChainedFastBackend object hanging around with an outdated $lastWriteTimestamp, despite deleteAll() having been called on all of the bin services. I don't know, but one guess is maybe a result of a container rebuild and some split-brain between an old and new cache.config service? It can be fixed by making ChainedFastBackend::deleteAll() invoke deleteAll() on the fast backend, but that would just be another mask.

It can be fixed by making ChainedFastBackend::deleteAll() invoke deleteAll() on the fast backend, but that would just be another mask.

On second thought, here's that fix. While it shouldn't be necessary, because the $lastWriteTimestamp should discard later gets, I also don't think it's a bad idea to delete things from the fast backend immediately.

Found the source of the split-brain (the test itself was holding on to an old container), so fixing it that way. We can have a followup to discuss whether to make ChainedFastBackend propagate deletes to the fast backend immediately or not; that's now out of scope for this issue.

Great clean-up/split in #124, and great detective work in #126/#127/#128!

Very close to RTBC; I only found nitpicks. But this will also need a change record.

1. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -27,13 +27,30 @@ class CacheFactory implements CacheFactoryInterface,  ContainerAwareInterface {
   +   * to fallback to the global default of 'cache.backend.database'.
   

   s/fallback/fall back/

2. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
   @@ -68,7 +70,7 @@ class ChainedFastBackend implements CacheBackendInterface {
   -   * @var int
   +   * @var float
   

   With the changes in #124, this is now an int again.

3. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -6,40 +6,80 @@
   +   * @param \Drupal\Core\Site\Settings $settings
   +   *   The settings array.
   

   s/array/object/

Fixed #129.1 and #129.3.

Re #129.2: nope, it's a float (see markAsOutdated()). HEAD's doc is wrong.

As far as change record, probably makes sense to document the new service tag.

In #2231595-96: Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend, catch said he considered a change record optional. However, I'm fine with writing one, if now that we're turning it on by default makes it more worthwhile to inform people about.

But, if I'm going to write it, would it be okay to fix the $settings (per the attached patch) thing that's been bugging me since #2231595-67: Add a cache backend that checks an inconsistent cache, then falls back to a consistent cache backend? Because I don't want to add those settings that I disagree with to the change record :)

Added draft change record: https://www.drupal.org/node/2327507.

Wonderful! The changes in #131 make a lot of sense; they make it significantly easier to understand. And one of the best change records I've seen yet!

I would love to RTBC, but #131 has been stuck on testbot for 19 hours and 40 minutes :(

#131 was canceled on testbot at my request (since it was stuck for >20 hours). Now re-tested, came back green. RTBC per #133.

This one seems catch-ish.

This looks great to me now, OK with #2327013: Add a TTL to ChainedFastBackend to prevent long running requests from using data that is too stale being in a follow-up.

Committed/pushed to 8.0.x, thanks!