Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Opening this as a follow-up to #1175054: Add a storage (API) for persistent non-configuration state.
For this I think it'd make sense to me to use the same logic as CacheArray (but not actually using CacheArray, we can have a storage controller with explicit methods that implements the same logic) for state, but we'll need to see how much it's used and what it's used for to know if that's really going to work or not.
Comment | File | Size | Author |
---|---|---|---|
#167 | interdiff.txt | 971 bytes | Wim Leers |
#167 | 1786490-167.patch | 49.5 KB | Wim Leers |
#138 | interdiff.txt | 2.09 KB | amateescu |
#138 | static-cache-state-1786490-138.patch | 59.38 KB | amateescu |
#137 | 1786490-137-xhprof.txt | 638 bytes | damiankloip |
Comments
Comment #1
BerdirClosed #1800538: Database KeyStorage implementation is not cached and called repeatedly as a duplicate, there is currently a query to the keyvalue table for every different menu item that is loaded.
Comment #2
BerdirComment #3
catchComment #4
BerdirNot sure if this needs to be critical, it's part of the performance meta issue now, so we need to deal with it to be able to close that one.
Comment #5
catchMakes sense, I'll calm down a bit.
Comment #6
BerdirSuggestions on how to do caching?
I think it shouldn't happen within the DatabaseStorage implementation, should that be wrapped in another class?
Do we want do cache all calls? I'm not sure if getAll() should be cached, for example.
Comment #7
BerdirAttaching a first proof of concept. Installation seems to work and xhprof shows that the state() queries are all nicely cached now.
As cache suggested in IRC as well, I think we should extract a generic CacheCollector or similar out of CacheArray that is not specific to being a ArrayAccess thing, that could have a proper interface and a get() method (and with the path alias, a clear). Because in this case, there's no point to go through offset(). Another example is my whitelist patch, no reason to treat it as an array if we don't have to due to backwards compatibility.
Not sure if it should happen here, would certainly make the implementation nicer and this isn't blocking anything else...
Had to rename set() to setData() in CacheArray() to not result in a naming clash.
Wondering if we should have a limited interface specifically for state, there's no need to support stuff like getAll(), getCollectionName(), and so on.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedplus 1 million for this approach! would be really glad to see us move away from ArrayAccess for D8.
Comment #10
BerdirLooks like we're missing delete/unset support in CacheArray. This was never necessary before but state needs it. This seems to fix most of the exceptions and fails. Still ugly, but making it nice will follow after I made it working ;)
Comment #12
Berdirset() needs to update the cache immediately.
Comment #14
BerdirUh, that didn't make any sense.
This should be better. Also had to use array_merge() in setData() as it didn't support updating existing data.
Comment #16
BerdirThis should fix the DUTP tests, haven't figured out the undefined key notice yet.
Comment #18
BerdirOpened #1858616: Extract generic CacheCollector implementation and interface from CacheArray to work on that separately.
Comment #19
BerdirRe-roll, included the cache collector class.
Still broken, especially when running tests.
Comment #21
BerdirRe-roll, seems to be working better now, let's see what the testbot thinks.
Comment #23
olli CreditAttribution: olli commentedtypo it->ti
$this->get($key)
- Does this update cache on set/delete*?
- Do we need something like refreshVariables for tests?
- Latest patch does not have the array_merge which I think is needed to be able to set*() anything.
Comment #24
BerdirThanks. re-roll that fixes the typos and the missing array_merge(). That might be the reason the install failed.
Comment #26
jthorson CreditAttribution: jthorson commentedCan't guarantee that this was actually this patch that threw the error below, but the timing lines up well. It's going to be a challenge for me to interrupt an install failure on a bot to verify, as long as we've got a patch backlog ... but first impressions suggest this is what's causing the install failure.
PHP Fatal error: Can't inherit abstract function Drupal\\Core\\KeyValueStore\\KeyValueStoreInterface::get() (previously declared abstract in Drupal\\Core\\Cache\\CacheCollectorInterface) in core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.php on line 17
Comment #27
BerdirGrr, sounds like a php version problem then. Let's try this.
Comment #29
BerdirFixed DUBT tests and also changed set/delete to immediately clear the cache, which fixes a lot of tests that set states in the test code and then exeucte a request.
The other way will still be broken. I haven't yet decided if we want to always clear the (local) state cache after a request, if there are not too many fails then I'd like to avoid that but that would mean that we need a simple way to clear the state cache.
Comment #31
BerdirAdded a reset() to the cache collector and a call to that from resetVariables(). This should fix most if not all of those fails.
Comment #33
Berdir#31: state-cache-decorator-1786490-31.patch queued for re-testing.
Comment #35
Berdir#31: state-cache-decorator-1786490-31.patch queued for re-testing.
Comment #37
BerdirHm, nasty, once again problems related to __destruct().
Comment #38
BerdirOh well....
Comment #39
BerdirOk, added proper documentation to the cache decorator class and added test coverage for it. Noticed a bunch of things that I fixed.
One thing to explore would be to use cache invalidation instead of deletion in the set and delete methods. Then parallel requests would immediately get the new value *if* they request it but we could get the invalidated cache entry in updateCache() if it still exists and update it.
Comment #41
Berdir#39: state-cache-decorator-1786490-39.patch queued for re-testing.
Comment #43
Berdir#39: state-cache-decorator-1786490-39.patch queued for re-testing.
Comment #45
BerdirUpdated patch to include the service destructor and use it, to see if this fixes the test fails.
Comment #46
olli CreditAttribution: olli commentedNice work, @Berdir!
Should this somehow check here if $data is still valid so that we don't override/add values that were set/deleted by parallel requests?
Comment #47
Berdir@olli: Yes, concurrency is a bit a problem. We could possibly also just drop the cache in case of set() and delete() calls (or invalidate and set back the cache entry hasn't changed). Need to think about that.
Anyway, here is a rerolled patch now that the service destructor is in. Yay!
Comment #48
ParisLiakos CreditAttribution: ParisLiakos commentedPatch needs a reroll:)
I have to say, very clean and straight forward implementation..some really minor notes:
why not
use
it?you can uncomment this now
\Drupal::service and since you are there maybe convert the one above;)
I ll try to find more after i can apply it locally
Comment #49
BerdirHere's a re-roll :)
Comment #50
BerdirNetbeans did too much there.
Comment #52
BerdirOk, so we learned today that we have 160 DUBT tests ;)
This should fix them, going back a simple non-cached state there, we're using the memory backend anyway.
Comment #53
dawehnerNice work, here are some small points.
This docblock seems to need new line-breaking for 80 chars.
I'm wondering whether we should request for the entry on demand? Additional: Needs one space more :)
Just wondering whether this is already "automatically" documentation by marking resolveCacheMiss an abstract method.
This is named DesctructableInterface now, so should the CacheCollector also implement this interface?
All this different parameters should be better typehinted, sorry for the nitpick!
Should we cleanup keysToPersist after updating the cache, because the values are already written?
missing empty line.
Do you have a use-case for delete? As it's just a caching wrapper it maybe is not needed.
Should be key_value_factory for codestyle reasons.
That's actually an Implementation of the key value store interface method.
Neither the interface nore this class has deleteMultiple ..., so is this still needed?
Can we use the container on $this->container directly?
Seems redundant to store $cache.
Comment #54
BerdirThanks for the review.
- Not sure about on-demand, we'd have to make sure everywhere that it's loaded before it's used. Also, we're talking about a class that's supposed to be used as a service. It is only constructed if it will be used (Not 100% true when being injected into another class that only maybe uses it).
- Removed the part about the abstract method, good point.
- I added delete() because it also exists (has to exist) in CacheArray. Doesn't hurt to have it, IMHO
- "Neither the interface nore this class has deleteMultiple ..., so is this still needed?" not sure what you mean here, as you said yourself, KeyValueStoreInterface has that method, maybe you added that later?
- "Can we use the container on $this->container directly?" There are some issues with $this->container in tests, it sometimes doesn't point to the right container anymore. See the critical cache flush issue about this.
All other points should be addressed. Thanks again for the great review!
Comment #56
Berdir#54: state-cache-decorator-1786490-54.patch queued for re-testing.
Comment #58
dawehner"ptional" has be to a swizz-german word :p
Maybe we could open a follow up to figure this out.
Ups right. I often forget to post reviews immediately and rereview parts, sorry.
Comment #59
dawehnerDue to #1886448: Rewrite the theme registry into a proper service I'm looking forward to get this issue resolved. Opened a follow up for Converting CacheArray to CacheCollector: http://drupal.org/node/1957092
I can't reproduce the failure locally, so this is just a try whether it passes.
Comment #60
damiankloip CreditAttribution: damiankloip commentedShould that say state?
getMultiple()
Could this just use array_key_exists? Not sure if isset() will help? Sure it will be true for non NULL values, but NULL will need the second check anyway?
Comment #61
dawehnerThis isset || array_key_exists fragment is some performance optimization. We want to have the functionality of array_key_exists, so 'foo' => NULL, should return TRUE, though we want to have the performance of isset() if possible, because isset(array('foo' => NULL)['foo']) is FALSE.
Comment #62
damiankloip CreditAttribution: damiankloip commentedI did wonder, thanks for clearing it up.
Comment #63
BerdirUpdated the patch, removed the state reference and fixed the getMultiple() docblock.
Also started to use cache invalidation to deal with set & get by default. It works basically like this:
I'm storing the cache creation time when loading the cache and when a value is set or deleted, I'm invalidating the cache entry and I'm also setting a flag that I did invalidate the cache. Now, when attempting to write the cache back, in case the flag was set, I'm also loading possibly invalidated cache entries. If the creation time doesn't match, then I know that someone else has started a new cache entry in the meantime. In that case, I don't know if a value was overriden again in another request and don't write those values back. To be extra sure, I also delete the current cache entry. Starting with the next request, the cache will start to build up gradually again.
The existing unit tests are green but I need to write explicit tests for this behavior but I wanted to see what the testbot thinks first. And I need some sleep ;)
Comment #65
pounardIt seems like premature optimization, if you have more isset() than !isset() you loose time instead of winning some of it.
Comment #66
dawehnerIt's a pattern which is used in a lot of places in core. As we expect the cache entry to be there, (which will be TRUE in most cases) this isset will be the way to go, at least from my perspective.
Comment #67
BerdirOh well, that was a nice theory with the conditional cache invalidate but what happened was this:
- Test started, was not able to load a cache, so no cacheCreated
- Value was set
- Page request accessed it, added it to the cache and stored it.
- Value was set again, test env still has no cacheCreated, so didn't bother to invalidate
- New request gets state cache with old value.
So, back to always invalidate that cache. Also had to fix a bug in cache memory backend, didn't like invalidate on non-existing cache entries very much.
Comment #69
Berdir#67: cache-state-invalidate-1786490-67.patch queued for re-testing.
Comment #70
BerdirThe test fail looked related but I can't reproduce locally...
Comment #72
Berdir#67: cache-state-invalidate-1786490-67.patch queued for re-testing.
Comment #74
BerdirThis seems to fix the twig settings patch, can't reproduce the file download test locally yet.
Comment #75
Berdir#74: cache-state-invalidate-1786490-74.patch queued for re-testing.
Comment #76
dawehnerJust in general I'm wondering whether it would make sense to implement the change in ViewsDataCache, which sets keyed storage data
directly on get(), so other requests can already profit from it.
Shouldn't we use \Drupal::state() if we change this line?
Ptional :)
Just wondering whether we use mixed in these cases.
Clever trick!
"\"
Why is it not worth to cache this? I guess it would help to just explain why here.
Drupal::state() could be used here
Comment #78
BerdirThanks for the review. ViewsDataCache set()'s each row as a separate cache entry, so setting on get() results in exactly as many cache sets as doing it combined. Here, we save all values as a single cache entry. We'd have to re-set that multiple times as the cache is initially built up.
Comment #79
BerdirAnd hm, that FileDownloadTest passed on the first attempt. Now it failed. I tried 10 times locally and it worked fine. Can someone reproduce this?
Comment #80
Berdirstate() is already deprecated and will removed, don't care about drupal_container() in there :) Can change but it seems useless to me.
Fixed the coding style stuff, improved the comments, used @inheritdocs (except in one instance, as that contains additional information where @inheritdoc afaik must not be used).
Comment #81
BerdirComment #83
BerdirTagging.
Comment #84
BerdirRe-roll. Let's see if the testbot learned how to behave.
Comment #86
BerdirC'mon, testbot, talk with me. We're friends, aren't we?
Comment #87
Berdir#84: cache-state-invalidate-1786490-84.patch queued for re-testing.
Comment #88
ParisLiakos CreditAttribution: ParisLiakos commentedquick coding review on #84
not sure why Interface and not interface
unnecessary;)
\Drupal::getContainer would be better since we are removing drupal_container()
Comment #89
Berdir#84: cache-state-invalidate-1786490-84.patch queued for re-testing.
Comment #90
BerdirFYI: I re-opened #1858616: Extract generic CacheCollector implementation and interface from CacheArray so that we can get the cache collector in. Could use some phpunit tests :)
Comment #91
BerdirCache state is in, so here's a re-roll.
Comment #93
BerdirFixed the search config and key value cache decorator tests, I wasn't able to reproduce the xml rpc one.
Comment #95
plach#93: cache-state-1786490-93.patch queued for re-testing.
Comment #97
BerdirRe-roll, some of the form problems were fixed in other issue, two small fixes.
Comment #99
BerdirRe-roll.
This is pretty ugly. the url_generator is persisted, which means that everything that is injected into that and is not persisted gets out of sync and then scary things start to happen. In this case, it is the language path processor that calls the language manager with isMultilingual() and then that language manager with that state object (which is a different than $container->get('state') at this point, hasn't been updated yet and still says 1 language.
Any service that has state and is injected into a persisted service needs to be persisted too..
Let's see if this works.
Comment #100
Crell CreditAttribution: Crell commentedEr. Any service that depends on the "state" service, or a service that itself has state? Because a stateful service is a design flaw on its face, with the sometimes exception of caching.
This looks like it's going to result in double traffic on setting. setMultiple() calls the parent, which sets the value, and then individually calls set(), which will set the value again.
We're not using drupal_container() anymore. It's deprecated. Use \Drupal::getContainer() if you really really need the container object directly.
String::formatPlural().
Comment #102
Berdir1. I don't follow. setMultiple() calls setMultiple() on the key value store and parent::set(). set() calls set() on the key value store and parent::set(). parent:set() isn't calling set(), that would be a loop ;)
2. & 3. That hunk is just moved above the table deletion, and that was done initially long before we even had a Drupal class :) And yes I do, Drupal doesn't have a hasService method. Fixed now.
This will still fail.
Comment #104
BerdirAs discussed today, here's a purely static cache using new class and Interface. Doing that requires quite a few changes in other services, because they type hint on the old interface.
Assuming this will install, this will fail on every test that does stuff and then expects updated state in the test code. Which I think will be a lot.
Comment #106
BerdirComment #108
Berdir#106: static-cache-state-1786490-106.patch queued for re-testing.
Comment #110
BerdirAdded resetCache() and added manual cache clear calls to all failing tests.
Comment #112
BerdirAh, I need to persist the state service again, for when the kernel is rebuild and persistent services have the state injected...
Comment #114
Crell CreditAttribution: Crell commentedresetCache() is the wrong approach. If a test is assuming state will be uncached, then we need to refactor the test, possibly including dropping and reinitializing the service. (That's the better way to clear the cache.) If normal code is, then IMO the code is broken in the first place.
Comment #115
BerdirJust a re-roll for now.
Comment #117
BerdirA few simple fixes.
Comment #119
amateescu CreditAttribution: amateescu commentedRerolled and fixed all the instances where the new StateInterface wasn't used. Let's see where we are with tests..
Comment #121
amateescu CreditAttribution: amateescu commentedThis gets us past the installer.
Comment #123
amateescu CreditAttribution: amateescu commentedSome more progress.
Comment #125
amateescu CreditAttribution: amateescu commentedThis takes care of the remaining ones, but there's an interesting regression in simpletest UI which is not reflected in any test: When you click 'Run tests' from the simpletest results page, you're no longer redirected to a batch page and then back to the results page, everything runs in the background and when the test results arrive, it reports a duration of 0 seconds.
Comment #127
amateescu CreditAttribution: amateescu commentedIt took 4 re-tests to get that patch to green :( Also, forget about the simpletest comment above, @dawehner pointed out that's already in HEAD so it can't be caused by this patch.
Comment #128
damiankloip CreditAttribution: damiankloip commentedMaybe we should document this is for performance? I guess that's the idea here.
This should take the default parameter, like KeyValueInterface::get()
Isn't this going to cause a bit of a loop? Should just be calling $this->set() inside the loop or $this->keyValueStore->setMultiple()? I guess that's not tested :)
Comment #129
dawehnerBeside from that it would be great to document in the issue summary why we went with an additional state interface which assumes caching alternative to
a CacheableKeyValueStoreInterface, or we just call it like that.
Here is a question: I do understand that you have to reset the state in a DUTB like test, though on a web test, doesn't that mean that this cache applies on the UI as well?
Comment #130
BerdirJust a few short comments, as I'm currently away...
1. Requiring multiple re-tests could indicate that we have random fails again (as my old patches did), we need to make sure that this isn't going to break HEAD randomly, e.g. by testing a final (hopefully) patch 10x or so.
2. I went to a static cache and an explict interface because that has been suggested in the performance/scability discussions in Prague, it's probably in the meeting notes there. The idea was to make a first step and then check if it's still a problem.
3. re DUBT vs. web test, it should be the other way round, actually. Only web tests should require manual resets because there we do page requests that could possibly change things but our static cache in the test run environment doesn't know that.
4. The main reason I didn't continue here was #114. I could convince @Crell in Prague that unsetting the container service isn't an option, think injected objects and stuff like that. We would have to re-set everyting that gets state injected and then things that would get that injected, which would probably be like 80% of our services (language manager uses state, so everything that uses language would need to be re-set). @Crell then suggested to not put the method on the interface and just assume that we're using the default backend in the test and call the method anyway, but I don't like that. resetCache() on a service might not be pure service thinking, but it's a fairly common pattern in e.g. the entity storage/render systems and we are going to be using that more not less as we add more caching.
Comment #131
amateescu CreditAttribution: amateescu commentedThanks for the reviews! This patch addresses #128, and #129 was answered by @Berdir.
Comment #135
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #136
amateescu CreditAttribution: amateescu commentedReviews from #128 and #129 were addressed, are there any other concerns here?
This is part of the critical "performance" meta so it would be nice get it in so we can start useful profiling again.
Comment #137
damiankloip CreditAttribution: damiankloip commentedThis is a really nitpicky point, but the order of the methods in the state class are pretty random. It would be nice to have them in order like StateInterface, so e.g. get/getMultiple are together. This is just so much easier when scanning the class.
Sorry in advance for that one :)
This seems a bit strong. We also don't know other people's use cases. Can we say something like 'This is mainly used for testing' or something? rather than ONLY and SIMPLETEST?
I also did a little bit of profiling comparing before and after the patch to hopefully give people an idea of the savings here, there is an increase in memory usage, but that is to be expected :) :
Comment #138
amateescu CreditAttribution: amateescu commentedWow, the order of the methods in State was really random, thanks for catching that. Also, people need to stop being sorry for reviews :)
Comment #139
Wim LeersWow, those profiling numbers look promising! It seems like that was in the context of a unit test running though? 66 KiB of memory usage seems pretty low for Drupal 8 :P
Patch looks good. A few nitpicks:
KeyValueStoreInterface
should go away, it's not used anymore.s/array/string[]/ is the latest trend I think?
I dislike all these
resetCache()
calls; this further complicates our already complex integration tests. But it sounds like there's no other way, so alas…Comment #140
amateescu CreditAttribution: amateescu commentedThanks for reviewing :)
Stuff[]
notation only Stuff is class, so an IDE can do something useful with it.Comment #141
damiankloip CreditAttribution: damiankloip commentedNot in a unit test, just only enabling xhprof for the code I wanted to see...
I don't see the problem with isolating calls like this tbh, why do we need the context of the whole page/bootstrap of all the other cruft that's running? It seems that muddies up the impact of this issue slightly.. This issue is about caching for the state system, So that profiled just that :)
This is RTBC now I think.
Comment #142
damiankloip CreditAttribution: damiankloip commentedComment #143
olli CreditAttribution: olli commentedWas there a problem with having one resetCache() in WebTestBase::refreshVariables() instead?
Comment #144
amateescu CreditAttribution: amateescu commentedThat's a fine point you have there.
Comment #145
BerdirI think what would be more interesting is an overview of the total amount of queries executed by the state system on some typical pages (e.g. frontpage with 20 nodes) before and after this patch. That should also tell us how many additional queries we could save with a persistent cache.
Not exactly sure what the profiling compares? Also note that you should remove the CPU flag as that results in considerable overhead and more function calls are more expensive than the actually are, so the results might look better than they are.
Comment #146
Wim Leers#140.2: aha, makes sense :)
#141: that's fair, I was just wondering — thanks for providing background info!
#143: Awesome. So much better now!
Comment #147
catchYep would be good to get the query before/after before commit so there's a clear record of what the known saving is.
We should also open a follow-up to discuss persistent caching.
Comment #148
damiankloip CreditAttribution: damiankloip commentedrerolled.
I also did a count of the # of queries run on the front page (with 50 nodes created, 10 showing in view) and the query count went from 222 to 173 after this patch was applied, so that's a saving of about 22% for the # of queries.
Comment #149
amateescu CreditAttribution: amateescu commentedHoping that the patch comes back green and #148 is good enough for "stats" :)
Comment #150
Wim Leers#148: is that the total number of queries or just those for the state system? I think it's the former?
It'd be useful to know the reduction in state system-only queries, too.
Comment #151
damiankloip CreditAttribution: damiankloip commented#150, yes that is the total queries. Surely the reduction is going to basically give you the decrease in calls to the state system anyway? as it is just without then with this patch applied.
If you want to get the isolated counts, feel free :)
Comment #152
Wim Leers#151: Yes, it shows the decrease, but it doesn't show what the total number of state system queries per page was before and after the patch. So we don't know if it's a 50% or 90% decrease.
It's a clear improvement, so this is not meant to hold back the patch, I'm merely asking out of interest :)
Comment #153
sunSince this is called very often, wouldn't it be faster and better to use native PHP functions here, like this?
I hope I'm not mistaken, but especially regarding the last pass of populating $this->cache with NULL values of unknown keys, there appears to be a difference in behavior in the currently proposed implementation when you call
getMultiple(array('unknown'))
more than once: The first time $values does not contain the key; if you call it again, then $values contains the key with a NULL value.Comment #154
amateescu CreditAttribution: amateescu commentedI measured the time spent executing only that piece of code on the frontpage with a bunch of nodes, making an average out of 5 runs. With the current code from the patch it's 47.896 ms and with the code from #153 it's 48.984 ms .. doesn't look like a gain to me.
Comment #155
webchickThis feels catch-ish. Also, major-ish.
Comment #156
webchickOne more.
Comment #157
olli CreditAttribution: olli commentedIs it easy to fix the second point in #153?
Comment #158
catchLet's sort that out either way before commit.
The rest of #153 doesn't look worth it to me unless there's a real performance issue there that's at least measurably in profiling.
I'd be more interest in adding CacheCollector on top of the static caching in a follow-up since that's likely to have a real impact.
Comment #159
BerdirOk, so here are the query statistics:
Tested on a frontpage with 10 n nodes being displayed.
HEAD:
Total queries: 223
state/keyvalue queries: 49 (~4200ms, 20+% of all queries)
(BTW, 111 of those queries are from the cache backend)
With the patch:
Total queries: 181
state/keyvalue queries: 7 ( execution time down to ~600ms, 4% wall time of all queries)
=> diff on both sums is 42, surprise ;)
Which means 7 different state keys that are being requested, which are:
- system.css_js_query_string
- language_count
- system.maintenance_mode
- menu_rebuild_needed
- menu.masks
- node.node_access_needs_rebuild
- system.private_key
So, there's still quite a bit that we can improve I think, especially given that the amount of state requests will increase on real sites I guess. But it's a considerable improvement already and as mentioned before, it's not the ms we save that's important here, it really is the amount of queries.
Back to @catch, or is there something else that needs to be done? Persistent cache will be a follow-up, as discussed.
Comment #160
webchickWow, that is damn impressive. :D Nice investigative work, Berdir!
Comment #161
catchI meant sun/olli's comment in #157, but that's so minor let's open a (minor) issue for it if we care.
@Berdir thanks for the breakdown of actual state queries, good to know where that stands atm. Agreed we can likely improve, so I've opened #2154461: Consider persistent caching and/or CacheCollector for state system to discuss.
For the number of cache queries, #2114319: Lots of cache requests from plugin discovery is one cause of those, relative to Drupal 7.
Committed/pushed to 8.x, thanks!
Comment #162
catchHEAD was broken - patch had gone stale in 8 days, so I've reverted for now.
Comment #163
Berdir148: 1786490-148.patch queued for re-testing.
Comment #165
amateescu CreditAttribution: amateescu commentedI think this is the only problem.
Comment #167
Wim LeersComment #168
amateescu CreditAttribution: amateescu commentedBack to RTBC.
Comment #169
catchCommitted/pushed again, thanks!