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.

CommentFileSizeAuthor
#167 interdiff.txt971 bytesWim Leers
#167 1786490-167.patch49.5 KBWim Leers
#165 interdiff.txt1.42 KBamateescu
#165 1786490-165.patch48.67 KBamateescu
#148 1786490-148.patch47.25 KBdamiankloip
#144 interdiff.txt12.98 KBamateescu
#144 static-cache-state-1786490-144.patch47.26 KBamateescu
#140 interdiff.txt490 bytesamateescu
#140 static-cache-state-1786490-140.patch59.41 KBamateescu
#138 interdiff.txt2.09 KBamateescu
#138 static-cache-state-1786490-138.patch59.38 KBamateescu
#137 1786490-137-xhprof.txt638 bytesdamiankloip
#135 static-cache-state-1786490-135.patch59.37 KBamateescu
#131 interdiff.txt1.95 KBamateescu
#131 static-cache-state-1786490-131.patch249.4 KBamateescu
#125 interdiff.txt1.71 KBamateescu
#125 static-cache-state-1786490-125.patch60.58 KBamateescu
#123 interdiff.txt6.99 KBamateescu
#123 static-cache-state-1786490-123.patch59.17 KBamateescu
#121 interdiff.txt1.54 KBamateescu
#121 static-cache-state-1786490-121.patch52.83 KBamateescu
#119 interdiff.txt12.25 KBamateescu
#119 static-cache-state-1786490-119.patch52.06 KBamateescu
#117 static-cache-state-1786490-117.patch42.94 KBBerdir
#117 static-cache-state-1786490-117-interdiff.txt2.84 KBBerdir
#115 static-cache-state-1786490-115.patch40.1 KBBerdir
#112 static-cache-state-1786490-112.patch40.19 KBBerdir
#112 static-cache-state-1786490-112-interdiff.txt1.13 KBBerdir
#110 static-cache-state-1786490-110.patch39.66 KBBerdir
#110 static-cache-state-1786490-110-interdiff.txt14.19 KBBerdir
#106 static-cache-state-1786490-106.patch26.61 KBBerdir
#106 static-cache-state-1786490-106-interdiff.txt726 bytesBerdir
#104 static-cache-state-1786490-104.patch26.21 KBBerdir
#102 cache-state-1786490-102.patch9.89 KBBerdir
#102 cache-state-1786490-102-interdiff.txt766 bytesBerdir
#99 cache-state-1786490-99.patch9.88 KBBerdir
#99 cache-state-1786490-99-interdiff.txt399 bytesBerdir
#97 cache-state-1786490-97.patch9.86 KBBerdir
#97 cache-state-1786490-97-interdiff.txt1.3 KBBerdir
#93 cache-state-1786490-93.patch12.61 KBBerdir
#93 cache-state-1786490-93-interdiff.txt3.52 KBBerdir
#91 cache-state-1786490-91.patch10.56 KBBerdir
#86 cache-state-invalidate-1786490-86-partial.patch23.29 KBBerdir
#86 cache-state-invalidate-1786490-86-interdiff.txt2.16 KBBerdir
#84 cache-state-invalidate-1786490-84.patch21.13 KBBerdir
#80 cache-state-invalidate-1786490-80.patch21.63 KBBerdir
#80 cache-state-invalidate-1786490-80-interdiff.txt6 KBBerdir
#74 cache-state-invalidate-1786490-74.patch21.99 KBBerdir
#74 cache-state-invalidate-1786490-74-interdiff.txt1.92 KBBerdir
#67 cache-state-invalidate-1786490-67.patch20.07 KBBerdir
#67 cache-state-invalidate-1786490-67-interdiff.txt1.9 KBBerdir
#63 cache-state-invalidate-1786490-63.patch19.99 KBBerdir
#63 cache-state-invalidate-1786490-63-interdiff.txt5.66 KBBerdir
#59 drupal-1786490-59.patch19.38 KBdawehner
#59 interdiff.txt1.35 KBdawehner
#54 state-cache-decorator-1786490-54.patch18.9 KBBerdir
#54 state-cache-decorator-1786490-54-interdiff.txt6.04 KBBerdir
#52 state-cache-decorator-1786490-52.patch18.95 KBBerdir
#52 state-cache-decorator-1786490-52-interdiff.txt1.22 KBBerdir
#50 state-cache-decorator-1786490-50.patch19.23 KBBerdir
#50 state-cache-decorator-1786490-50-interdiff.txt427 bytesBerdir
#49 state-cache-decorator-1786490-49.patch19.21 KBBerdir
#49 state-cache-decorator-1786490-49-interdiff.txt2.99 KBBerdir
#47 state-cache-decorator-1786490-47.patch19.44 KBBerdir
#45 state-cache-decorator-1786490-45.patch33.13 KBBerdir
#45 state-cache-decorator-1786490-45-interdiff.txt1.91 KBBerdir
#39 state-cache-decorator-1786490-39.patch19.73 KBBerdir
#39 state-cache-decorator-1786490-39-interdiff.txt8.28 KBBerdir
#38 catch-them-exceptions-1786490-38.patch15.41 KBBerdir
#38 catch-them-exceptions-1786490-38-interdiff.txt766 bytesBerdir
#31 state-cache-decorator-1786490-31.patch15.17 KBBerdir
#31 state-cache-decorator-1786490-31-interdiff.txt1.72 KBBerdir
#29 state-cache-decorator-1786490-29.patch14.34 KBBerdir
#29 state-cache-decorator-1786490-29-interdiff.txt2.78 KBBerdir
#27 state-cache-decorator-1786490-27.patch12.69 KBBerdir
#27 state-cache-decorator-1786490-27-interdiff.txt513 bytesBerdir
#24 state-cache-decorator-1786490-24.patch12.63 KBBerdir
#24 state-cache-decorator-1786490-24-interdiff.txt1.48 KBBerdir
#21 state-cache-decorator-1786490-21.patch12.63 KBBerdir
#19 state-cache-with-collector.patch16.07 KBBerdir
#16 state-caching-1786490-16.patch7.74 KBBerdir
#16 state-caching-1786490-16-interdiff.txt771 bytesBerdir
#14 state-caching-1786490-13.patch6.98 KBBerdir
#14 state-caching-1786490-13-interdiff.txt1.21 KBBerdir
#12 state-caching-1786490-12.patch6.99 KBBerdir
#12 state-caching-1786490-12-interdiff.txt608 bytesBerdir
#10 state-caching-1786490-10.patch6.93 KBBerdir
#10 state-caching-1786490-10-interdiff.txt1.55 KBBerdir
#7 state-caching-1786490-7.patch5.61 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Closed #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.

Berdir’s picture

Issue tags: +Performance
catch’s picture

Priority: Normal » Critical
Berdir’s picture

Not 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.

catch’s picture

Priority: Critical » Normal

Makes sense, I'll calm down a bit.

Berdir’s picture

Suggestions 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.

Berdir’s picture

Status: Active » Needs review
FileSize
5.61 KB

Attaching 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.

Status: Needs review » Needs work

The last submitted patch, state-caching-1786490-7.patch, failed testing.

Anonymous’s picture

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.

plus 1 million for this approach! would be really glad to see us move away from ArrayAccess for D8.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
6.93 KB

Looks 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 ;)

Status: Needs review » Needs work

The last submitted patch, state-caching-1786490-10.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
608 bytes
6.99 KB

set() needs to update the cache immediately.

Status: Needs review » Needs work

The last submitted patch, state-caching-1786490-12.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.21 KB
6.98 KB

Uh, 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.

Status: Needs review » Needs work

The last submitted patch, state-caching-1786490-13.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
771 bytes
7.74 KB

This should fix the DUTP tests, haven't figured out the undefined key notice yet.

Status: Needs review » Needs work

The last submitted patch, state-caching-1786490-16.patch, failed testing.

Berdir’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
16.07 KB

Re-roll, included the cache collector class.

Still broken, especially when running tests.

Status: Needs review » Needs work

The last submitted patch, state-cache-with-collector.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
12.63 KB

Re-roll, seems to be working better now, let's see what the testbot thinks.

Status: Needs review » Needs work

The last submitted patch, state-cache-decorator-1786490-21.patch, failed testing.

olli’s picture

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.php
@@ -0,0 +1,93 @@
+    $this->keyValueStore->deleteMulitple($keys);

typo it->ti

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.php
@@ -0,0 +1,93 @@
+      $values[$key] = $this->offsetGet($key);

$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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
12.63 KB

Thanks. re-roll that fixes the typos and the missing array_merge(). That might be the reason the install failed.

Status: Needs review » Needs work

The last submitted patch, state-cache-decorator-1786490-24.patch, failed testing.

jthorson’s picture

Can'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

Berdir’s picture

Status: Needs work » Needs review
FileSize
513 bytes
12.69 KB

Grr, sounds like a php version problem then. Let's try this.

Status: Needs review » Needs work

The last submitted patch, state-cache-decorator-1786490-27.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.78 KB
14.34 KB

Fixed 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.

Status: Needs review » Needs work

The last submitted patch, state-cache-decorator-1786490-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.72 KB
15.17 KB

Added a reset() to the cache collector and a call to that from resetVariables(). This should fix most if not all of those fails.

Status: Needs review » Needs work
Issue tags: -Performance

The last submitted patch, state-cache-decorator-1786490-31.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, state-cache-decorator-1786490-31.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance

The last submitted patch, state-cache-decorator-1786490-31.patch, failed testing.

Berdir’s picture

Hm, nasty, once again problems related to __destruct().

Berdir’s picture

Status: Needs work » Needs review
FileSize
766 bytes
15.41 KB

Oh well....

Berdir’s picture

Ok, 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.

Status: Needs review » Needs work
Issue tags: -Performance

The last submitted patch, state-cache-decorator-1786490-39.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, state-cache-decorator-1786490-39.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance

The last submitted patch, state-cache-decorator-1786490-39.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
33.13 KB

Updated patch to include the service destructor and use it, to see if this fixes the test fails.

olli’s picture

Nice work, @Berdir!

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.php
@@ -0,0 +1,226 @@
+      if ($cached = $this->cache->get($this->cid)) {
+        $data = array_merge($cached->data, $data);
+      }

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?

Berdir’s picture

@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!

ParisLiakos’s picture

Status: Needs review » Needs work

Patch needs a reroll:)

I have to say, very clean and straight forward implementation..some really minor notes:

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,226 @@
+  public function __construct($cid, CacheBackendInterface $cache, \Drupal\Core\Lock\LockBackendInterface $lock, $tags = array()) {

why not use it?

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorInterface.phpundefined
@@ -0,0 +1,73 @@
+  // @todo: This results in a fatal error in PHP <5.3.10.
+  //public function get($key);

you can uncomment this now

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -899,6 +899,7 @@ protected function refreshVariables() {
     drupal_container()->get('config.factory')->reset();
+    drupal_container()->get('state')->reset();

\Drupal::service and since you are there maybe convert the one above;)

I ll try to find more after i can apply it locally

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.99 KB
19.21 KB

Here's a re-roll :)

Berdir’s picture

Netbeans did too much there.

Status: Needs review » Needs work

The last submitted patch, state-cache-decorator-1786490-50.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
18.95 KB

Ok, 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.

dawehner’s picture

Nice work, here are some small points.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,229 @@
+ * By default, the class accounts for caches where calling functions might

This docblock seems to need new line-breaking for 80 chars.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,229 @@
+ * request keys that won't exist even after a cache rebuild. This
...
+    if ($cached = $this->cache->get($this->cid)) {
+     $this->storage = $cached->data;
+    }

I'm wondering whether we should request for the entry on demand? Additional: Needs one space more :)

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,229 @@
+ * Classes extending this class must override at least the
+ * CacheCollector::resolveCacheMiss() method to have a working implementation.

Just wondering whether this is already "automatically" documentation by marking resolveCacheMiss an abstract method.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,229 @@
+abstract class CacheCollector implements CacheCollectorInterface {
...
+   * Implements KernelServiceDestruction::destruct().

This is named DesctructableInterface now, so should the CacheCollector also implement this interface?

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,229 @@
+   * @param $key
...
+   * @param $persist
...
+   * @param $key

All this different parameters should be better typehinted, sorry for the nitpick!

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,229 @@
+    foreach ($this->keysToPersist as $offset => $persist) {

Should we cleanup keysToPersist after updating the cache, because the values are already written?

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,229 @@
+  }

missing empty line.

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorInterface.phpundefined
@@ -0,0 +1,72 @@
+  public function delete($key);

Do you have a use-case for delete? As it's just a caching wrapper it maybe is not needed.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.phpundefined
@@ -0,0 +1,152 @@
+   * @param \Drupal\Core\KeyValueStore\KeyValueFactory $keyValueFactory
...
+  public function __construct(CacheBackendInterface $cache, LockBackendInterface $lock, KeyValueFactory $keyValueFactory, $collection) {

Should be key_value_factory for codestyle reasons.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.phpundefined
@@ -0,0 +1,152 @@
+   * Overrides \Drupal\Core\Cache\CacheCollector::deleteMultiple().

That's actually an Implementation of the key value store interface method.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.phpundefined
@@ -0,0 +1,152 @@
+  public function deleteMultiple(array $keys) {

Neither the interface nore this class has deleteMultiple ..., so is this still needed?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -890,7 +890,8 @@ protected function refreshVariables() {
+    \Drupal::service('config.factory')->reset();
+    \Drupal::service('state')->reset();

Can we use the container on $this->container directly?

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/CacheDecoratorTest.phpundefined
@@ -0,0 +1,87 @@
+    $this->cache = $cache = new MemoryBackend('bin');

Seems redundant to store $cache.

Berdir’s picture

Thanks 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!

Status: Needs review » Needs work
Issue tags: -Performance

The last submitted patch, state-cache-decorator-1786490-54.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance

The last submitted patch, state-cache-decorator-1786490-54.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -148,12 +146,12 @@ public function delete($key) {
+   *   (ptional) Whether the offset should be persisted or not, defaults to

"ptional" has be to a swizz-german word :p

- I blockquote delete() because it also exists (has to exist) in CacheArray. Doesn't hurt to have it, IMHO

Maybe we could open a follow up to figure this out.

- "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?

Ups right. I often forget to post reviews immediately and rereview parts, sorry.

dawehner’s picture

FileSize
1.35 KB
19.38 KB

Due 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.

damiankloip’s picture

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.phpundefined
@@ -0,0 +1,152 @@
+   * The key value store to use as state.

Should that say state?

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.phpundefined
@@ -0,0 +1,152 @@
+   * Implements KeyValueStoreInterface::getAll().

getMultiple()

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,230 @@
+    if (isset($this->storage[$key]) || array_key_exists($key, $this->storage)) {

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?

dawehner’s picture

This 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.

damiankloip’s picture

I did wonder, thanks for clearing it up.

Berdir’s picture

Updated 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 ;)

Status: Needs review » Needs work

The last submitted patch, cache-state-invalidate-1786490-63.patch, failed testing.

pounard’s picture

This 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.

It seems like premature optimization, if you have more isset() than !isset() you loose time instead of winning some of it.

dawehner’s picture

It'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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
20.07 KB

Oh 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.

Status: Needs review » Needs work
Issue tags: -Performance

The last submitted patch, cache-state-invalidate-1786490-67.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Performance
Berdir’s picture

The test fail looked related but I can't reproduce locally...

Status: Needs review » Needs work
Issue tags: -Performance

The last submitted patch, cache-state-invalidate-1786490-67.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance

The last submitted patch, cache-state-invalidate-1786490-67.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
21.99 KB

This seems to fix the twig settings patch, can't reproduce the file download test locally yet.

Berdir’s picture

dawehner’s picture

Just 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.

+++ b/core/includes/bootstrap.incundefined
@@ -2517,7 +2517,7 @@ function module_hook($module, $hook) {
+  return drupal_container()->get('state');

Shouldn't we use \Drupal::state() if we change this line?

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,268 @@
+   *   (ptional) Whether the offset should be persisted or not, defaults to

Ptional :)

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,268 @@
+   * @return

Just wondering whether we use mixed in these cases.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,268 @@
+    if (!$lock || $this->lock->acquire($lock_name)) {

Clever trick!

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorInterface.phpundefined
@@ -0,0 +1,72 @@
+ * Contains Drupal\Core\Cache\CacheCollectorInterface.

"\"

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.phpundefined
@@ -0,0 +1,140 @@
+    // Don't cache this.

Why is it not worth to cache this? I guess it would help to just explain why here.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -926,7 +926,8 @@ protected function refreshVariables() {
+    \Drupal::service('state')->reset();

Drupal::state() could be used here

Status: Needs review » Needs work

The last submitted patch, cache-state-invalidate-1786490-74.patch, failed testing.

Berdir’s picture

Thanks 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.

Berdir’s picture

And hm, that FileDownloadTest passed on the first attempt. Now it failed. I tried 10 times locally and it worked fine. Can someone reproduce this?

Berdir’s picture

state() 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).

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, cache-state-invalidate-1786490-80.patch, failed testing.

Berdir’s picture

Issue tags: +D8 cacheability

Tagging.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.13 KB

Re-roll. Let's see if the testbot learned how to behave.

Status: Needs review » Needs work

The last submitted patch, cache-state-invalidate-1786490-84.patch, failed testing.

Berdir’s picture

C'mon, testbot, talk with me. We're friends, aren't we?

Berdir’s picture

ParisLiakos’s picture

quick coding review on #84

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorInterface.phpundefined
@@ -0,0 +1,72 @@
+Interface CacheCollectorInterface {

not sure why Interface and not interface

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -172,7 +172,7 @@ public function containerBuild(ContainerBuilder $container) {
-        ->addArgument('state');
+       ->addArgument('state');

unnecessary;)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1001,6 +1001,15 @@ protected function tearDown() {
+    if (($container = drupal_container()) && $container->has('state')) {

\Drupal::getContainer would be better since we are removing drupal_container()

Berdir’s picture

Berdir’s picture

FYI: 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 :)

Berdir’s picture

FileSize
10.56 KB

Cache state is in, so here's a re-roll.

Status: Needs review » Needs work

The last submitted patch, cache-state-1786490-91.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.52 KB
12.61 KB

Fixed the search config and key value cache decorator tests, I wasn't able to reproduce the xml rpc one.

Status: Needs review » Needs work
Issue tags: -Performance, -D8 cacheability

The last submitted patch, cache-state-1786490-93.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#93: cache-state-1786490-93.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +D8 cacheability

The last submitted patch, cache-state-1786490-93.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
9.86 KB

Re-roll, some of the form problems were fixed in other issue, two small fixes.

Status: Needs review » Needs work

The last submitted patch, cache-state-1786490-97.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
399 bytes
9.88 KB

Re-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.

Crell’s picture

Any service that has state and is injected into a persisted service needs to be persisted too..

Er. 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.

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueCacheDecorator.php
    @@ -0,0 +1,142 @@
    +  public function setMultiple(array $data) {
    +    $this->keyValueStore->setMultiple($data);
    +    foreach ($data as $key => $value) {
    +      parent::set($key, $value);
    +      $this->persist($key);
    +    }
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function set($key, $value) {
    +    $this->keyValueStore->set($key, $value);
    +    parent::set($key, $value);
    +    $this->persist($key);
    +  }
    

    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.

  2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -1028,6 +1028,15 @@ protected function tearDown() {
    +    if (($container = drupal_container()) && $container->has('state')) {
    

    We're not using drupal_container() anymore. It's deprecated. Use \Drupal::getContainer() if you really really need the container object directly.

  3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -1028,6 +1028,15 @@ protected function tearDown() {
    +        $message = format_plural($emailCount, '1 e-mail was sent during this test.', '@count e-mails were sent during this test.');
    

    String::formatPlural().

Status: Needs review » Needs work

The last submitted patch, cache-state-1786490-99.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
766 bytes
9.89 KB

1. 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.

Status: Needs review » Needs work

The last submitted patch, cache-state-1786490-102.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.21 KB

As 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.

Status: Needs review » Needs work

The last submitted patch, static-cache-state-1786490-104.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
726 bytes
26.61 KB

Status: Needs review » Needs work
Issue tags: -Performance, -D8 cacheability

The last submitted patch, static-cache-state-1786490-106.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Performance, +D8 cacheability

The last submitted patch, static-cache-state-1786490-106.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
14.19 KB
39.66 KB

Added resetCache() and added manual cache clear calls to all failing tests.

Status: Needs review » Needs work

The last submitted patch, static-cache-state-1786490-110.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
40.19 KB

Ah, I need to persist the state service again, for when the kernel is rebuild and persistent services have the state injected...

Status: Needs review » Needs work

The last submitted patch, static-cache-state-1786490-112.patch, failed testing.

Crell’s picture

resetCache() 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
40.1 KB

Just a re-roll for now.

Status: Needs review » Needs work

The last submitted patch, static-cache-state-1786490-115.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
42.94 KB

A few simple fixes.

Status: Needs review » Needs work
Issue tags: -Performance, -D8 cacheability

The last submitted patch, static-cache-state-1786490-117.patch, failed testing.

amateescu’s picture

Assigned: Unassigned » amateescu
Issue summary: View changes
Status: Needs work » Needs review
FileSize
52.06 KB
12.25 KB

Rerolled and fixed all the instances where the new StateInterface wasn't used. Let's see where we are with tests..

Status: Needs review » Needs work

The last submitted patch, 119: static-cache-state-1786490-119.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
52.83 KB
1.54 KB

This gets us past the installer.

Status: Needs review » Needs work

The last submitted patch, 121: static-cache-state-1786490-121.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
59.17 KB
6.99 KB

Some more progress.

Status: Needs review » Needs work

The last submitted patch, 123: static-cache-state-1786490-123.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
60.58 KB
1.71 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, 125: static-cache-state-1786490-125.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

It 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.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/State.php
    @@ -0,0 +1,123 @@
    +        if (isset($loaded_values[$key]) || array_key_exists($key, $loaded_values)) {
    

    Maybe we should document this is for performance? I guess that's the idea here.

  2. +++ b/core/lib/Drupal/Core/KeyValueStore/State.php
    @@ -0,0 +1,123 @@
    +  public function get($key) {
    

    This should take the default parameter, like KeyValueInterface::get()

  3. +++ b/core/lib/Drupal/Core/KeyValueStore/State.php
    @@ -0,0 +1,123 @@
    +  public function setMultiple(array $data) {
    +    foreach ($data as $key => $value) {
    +      $this->cache[$key] = $value;
    +    }
    +    $this->setMultiple($data);
    

    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 :)

dawehner’s picture

Beside 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.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageListTest.php
@@ -96,6 +97,7 @@ function testLanguageList() {
     $this->drupalPostForm($path, $edit, t('Save configuration'));
+    \Drupal::state()->resetCache();
     // Ensure 'delete' link works.
     $this->drupalGet('admin/config/regional/language');

@@ -109,6 +111,7 @@ function testLanguageList() {
     // fields changed.
...
+    \Drupal::state()->resetCache();
     // We need raw here because %language and %langcode will add HTML.

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?

Berdir’s picture

Just 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.

amateescu’s picture

Thanks for the reviews! This patch addresses #128, and #129 was answered by @Berdir.

The last submitted patch, 131: static-cache-state-1786490-131.patch, failed testing.

The last submitted patch, 131: static-cache-state-1786490-131.patch, failed testing.

The last submitted patch, 131: static-cache-state-1786490-131.patch, failed testing.

amateescu’s picture

Rerolled.

amateescu’s picture

Reviews 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.

damiankloip’s picture

FileSize
638 bytes
  1. +++ b/core/lib/Drupal/Core/KeyValueStore/State.php
    @@ -0,0 +1,125 @@
    +  public function getMultiple(array $keys) {
    ...
    +  public function set($key, $value) {
    ...
    +  public function deleteMultiple(array $keys) {
    ...
    +  public function get($key, $default = NULL) {
    ...
    +  public function setMultiple(array $data) {
    ...
    +  public function delete($key) {
    ...
    +  public function resetCache() {
    

    This 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 :)

  2. +++ b/core/lib/Drupal/Core/KeyValueStore/StateInterface.php
    @@ -0,0 +1,80 @@
    +   * This is only needed for Simpletest.
    

    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 :) :

Run #5298579dcc37c Run #5298576b82f7b Diff Diff%
Number of Function Calls 322 46 -276 -85.7%
Incl. Wall Time (microsec) 5,273 725 -4,548 -86.3%
Incl. CPU (microsecs) 3,485 507 -2,978 -85.5%
Incl. MemUse (bytes) 28,432 66,072 37,640 132.4%
Incl. PeakMemUse (bytes) 60,184 57,088 -3,096 -5.1%
amateescu’s picture

Wow, the order of the methods in State was really random, thanks for catching that. Also, people need to stop being sorry for reviews :)

Wim Leers’s picture

Wow, 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:

  1. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
    @@ -8,6 +8,7 @@
     use Drupal\Core\KeyValueStore\KeyValueStoreInterface;
    +use Drupal\Core\KeyValueStore\StateInterface;
    

    KeyValueStoreInterface should go away, it's not used anymore.

  2. +++ b/core/lib/Drupal/Core/KeyValueStore/StateInterface.php
    @@ -0,0 +1,80 @@
    +   * @param array $keys
    ...
    +   * @param array $keys
    

    s/array/string[]/ is the latest trend I think?

  3. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.php
    @@ -72,6 +72,8 @@ function testLanguageConfiguration() {
    +    \Drupal::state()->resetCache();
    

    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…

amateescu’s picture

Thanks for reviewing :)

  1. Fixed.
  2. We use the Stuff[] notation only Stuff is class, so an IDE can do something useful with it.
  3. Méh
damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Wow, 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

Not 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.

damiankloip’s picture

Assigned: amateescu » Unassigned
olli’s picture

Was there a problem with having one resetCache() in WebTestBase::refreshVariables() instead?

amateescu’s picture

That's a fine point you have there.

Berdir’s picture

I 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.

Wim Leers’s picture

#140.2: aha, makes sense :)
#141: that's fair, I was just wondering — thanks for providing background info!
#143: Awesome. So much better now!

catch’s picture

Status: Reviewed & tested by the community » Needs review

Yep 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.

damiankloip’s picture

FileSize
47.25 KB

rerolled.

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.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Hoping that the patch comes back green and #148 is good enough for "stats" :)

Wim Leers’s picture

#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.

damiankloip’s picture

#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 :)

Wim Leers’s picture

#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 :)

sun’s picture

+  public function getMultiple(array $keys) {

Since this is called very often, wouldn't it be faster and better to use native PHP functions here, like this?

$keys = array_fill_keys($keys, NULL);

// Retrieve all statically cached values, regardless of value (including NULL).
$values = array_intersect_key($this->cache, $keys);

// Load and statically cache missing values.
if ($load = array_diff_key($keys, $values)) {
  $loaded = $this->keyValueStore->getMultiple(array_keys($load));
  $values += $loaded;
  $this->cache += $loaded;
}

// Explicitly cache unknown keys with NULL values.
if ($null = array_diff_key($keys, $values)) {
  $values += $null;
  $this->cache += $null;
}
return $values;

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.

amateescu’s picture

I 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.

webchick’s picture

Assigned: Unassigned » catch
Priority: Normal » Major

This feels catch-ish. Also, major-ish.

webchick’s picture

Issue tags: +Performance

One more.

olli’s picture

Is it easy to fix the second point in #153?

The first time $values does not contain the key; if you call it again, then $values contains the key with a NULL value.
catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs review

Let'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.

Berdir’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community

Ok, 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.

webchick’s picture

Wow, that is damn impressive. :D Nice investigative work, Berdir!

catch’s picture

Status: Reviewed & tested by the community » Fixed

I 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!

catch’s picture

Status: Fixed » Needs work

HEAD was broken - patch had gone stale in 8 days, so I've reverted for now.

Berdir’s picture

Status: Needs work » Needs review

148: 1786490-148.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 148: 1786490-148.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
48.67 KB
1.42 KB

I think this is the only problem.

The last submitted patch, 165: 1786490-165.patch, failed testing.

Wim Leers’s picture

FileSize
49.5 KB
971 bytes
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed again, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.