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.

Files: 
CommentFileSizeAuthor
#167 interdiff.txt971 bytesWim Leers
#167 1786490-167.patch49.5 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,400 pass(es).
[ View ]
#138 interdiff.txt2.09 KBamateescu
#138 static-cache-state-1786490-138.patch59.38 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,079 pass(es).
[ View ]
#137 1786490-137-xhprof.txt638 bytesdamiankloip
#135 static-cache-state-1786490-135.patch59.37 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es).
[ View ]
#131 interdiff.txt1.95 KBamateescu
#131 static-cache-state-1786490-131.patch249.4 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch static-cache-state-1786490-131.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#125 interdiff.txt1.71 KBamateescu
#125 static-cache-state-1786490-125.patch60.58 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 58,985 pass(es).
[ View ]
#123 interdiff.txt6.99 KBamateescu
#123 static-cache-state-1786490-123.patch59.17 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 58,506 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#121 interdiff.txt1.54 KBamateescu
#121 static-cache-state-1786490-121.patch52.83 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 59,389 pass(es), 33 fail(s), and 1,055 exception(s).
[ View ]
#119 interdiff.txt12.25 KBamateescu
#119 static-cache-state-1786490-119.patch52.06 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#117 static-cache-state-1786490-117.patch42.94 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#117 static-cache-state-1786490-117-interdiff.txt2.84 KBBerdir
#115 static-cache-state-1786490-115.patch40.1 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#112 static-cache-state-1786490-112.patch40.19 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,711 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#112 static-cache-state-1786490-112-interdiff.txt1.13 KBBerdir
#110 static-cache-state-1786490-110.patch39.66 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 59,059 pass(es), 15 fail(s), and 3 exception(s).
[ View ]
#110 static-cache-state-1786490-110-interdiff.txt14.19 KBBerdir
#106 static-cache-state-1786490-106.patch26.61 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,664 pass(es), 76 fail(s), and 8 exception(s).
[ View ]
#106 static-cache-state-1786490-106-interdiff.txt726 bytesBerdir
#104 static-cache-state-1786490-104.patch26.21 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#102 cache-state-1786490-102.patch9.89 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,129 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#102 cache-state-1786490-102-interdiff.txt766 bytesBerdir
#99 cache-state-1786490-99.patch9.88 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,408 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#99 cache-state-1786490-99-interdiff.txt399 bytesBerdir
#97 cache-state-1786490-97.patch9.86 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#97 cache-state-1786490-97-interdiff.txt1.3 KBBerdir
#93 cache-state-1786490-93.patch12.61 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,062 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#93 cache-state-1786490-93-interdiff.txt3.52 KBBerdir
#91 cache-state-1786490-91.patch10.56 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,068 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#86 cache-state-invalidate-1786490-86-partial.patch23.29 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 286 pass(es).
[ View ]
#86 cache-state-invalidate-1786490-86-interdiff.txt2.16 KBBerdir
#84 cache-state-invalidate-1786490-84.patch21.13 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,312 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#80 cache-state-invalidate-1786490-80.patch21.63 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,896 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#80 cache-state-invalidate-1786490-80-interdiff.txt6 KBBerdir
#74 cache-state-invalidate-1786490-74.patch21.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,990 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#74 cache-state-invalidate-1786490-74-interdiff.txt1.92 KBBerdir
#67 cache-state-invalidate-1786490-67.patch20.07 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,349 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#67 cache-state-invalidate-1786490-67-interdiff.txt1.9 KBBerdir
#63 cache-state-invalidate-1786490-63.patch19.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,596 pass(es), 353 fail(s), and 30 exception(s).
[ View ]
#63 cache-state-invalidate-1786490-63-interdiff.txt5.66 KBBerdir
#59 drupal-1786490-59.patch19.38 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 53,722 pass(es).
[ View ]
#59 interdiff.txt1.35 KBdawehner
#54 state-cache-decorator-1786490-54.patch18.9 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,674 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#54 state-cache-decorator-1786490-54-interdiff.txt6.04 KBBerdir
#52 state-cache-decorator-1786490-52.patch18.95 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,352 pass(es).
[ View ]
#52 state-cache-decorator-1786490-52-interdiff.txt1.22 KBBerdir
#50 state-cache-decorator-1786490-50.patch19.23 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 46,646 pass(es), 161 fail(s), and 160 exception(s).
[ View ]
#50 state-cache-decorator-1786490-50-interdiff.txt427 bytesBerdir
#49 state-cache-decorator-1786490-49.patch19.21 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#49 state-cache-decorator-1786490-49-interdiff.txt2.99 KBBerdir
#47 state-cache-decorator-1786490-47.patch19.44 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 52,564 pass(es).
[ View ]
#45 state-cache-decorator-1786490-45.patch33.13 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 52,295 pass(es).
[ View ]
#45 state-cache-decorator-1786490-45-interdiff.txt1.91 KBBerdir
#39 state-cache-decorator-1786490-39.patch19.73 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,258 pass(es), 29 fail(s), and 42 exception(s).
[ View ]
#39 state-cache-decorator-1786490-39-interdiff.txt8.28 KBBerdir
#38 catch-them-exceptions-1786490-38.patch15.41 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 51,405 pass(es).
[ View ]
#38 catch-them-exceptions-1786490-38-interdiff.txt766 bytesBerdir
#31 state-cache-decorator-1786490-31.patch15.17 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 51,128 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#31 state-cache-decorator-1786490-31-interdiff.txt1.72 KBBerdir
#29 state-cache-decorator-1786490-29.patch14.34 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 50,692 pass(es), 65 fail(s), and 3 exception(s).
[ View ]
#29 state-cache-decorator-1786490-29-interdiff.txt2.78 KBBerdir
#27 state-cache-decorator-1786490-27.patch12.69 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,466 pass(es), 694 fail(s), and 121 exception(s).
[ View ]
#27 state-cache-decorator-1786490-27-interdiff.txt513 bytesBerdir
#24 state-cache-decorator-1786490-24.patch12.63 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#24 state-cache-decorator-1786490-24-interdiff.txt1.48 KBBerdir
#21 state-cache-decorator-1786490-21.patch12.63 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#19 state-cache-with-collector.patch16.07 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#16 state-caching-1786490-16.patch7.74 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,168 pass(es), 59 fail(s), and 1,015 exception(s).
[ View ]
#16 state-caching-1786490-16-interdiff.txt771 bytesBerdir
#14 state-caching-1786490-13.patch6.98 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,675 pass(es), 99 fail(s), and 987 exception(s).
[ View ]
#14 state-caching-1786490-13-interdiff.txt1.21 KBBerdir
#12 state-caching-1786490-12.patch6.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,286 pass(es), 521 fail(s), and 1,055 exception(s).
[ View ]
#12 state-caching-1786490-12-interdiff.txt608 bytesBerdir
#10 state-caching-1786490-10.patch6.93 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 46,796 pass(es), 916 fail(s), and 131 exception(s).
[ View ]
#10 state-caching-1786490-10-interdiff.txt1.55 KBBerdir
#7 state-caching-1786490-7.patch5.61 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,335 pass(es), 1,265 fail(s), and 4,095 exception(s).
[ View ]

Comments

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.

Issue tags:+Performance

Priority:Normal» Critical

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.

Priority:Critical» Normal

Makes sense, I'll calm down a bit.

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.

Status:Active» Needs review
StatusFileSize
new5.61 KB
FAILED: [[SimpleTest]]: [MySQL] 47,335 pass(es), 1,265 fail(s), and 4,095 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.55 KB
new6.93 KB
FAILED: [[SimpleTest]]: [MySQL] 46,796 pass(es), 916 fail(s), and 131 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new608 bytes
new6.99 KB
FAILED: [[SimpleTest]]: [MySQL] 47,286 pass(es), 521 fail(s), and 1,055 exception(s).
[ View ]

set() needs to update the cache immediately.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new6.98 KB
FAILED: [[SimpleTest]]: [MySQL] 47,675 pass(es), 99 fail(s), and 987 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new771 bytes
new7.74 KB
FAILED: [[SimpleTest]]: [MySQL] 49,168 pass(es), 59 fail(s), and 1,015 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new16.07 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new12.63 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.48 KB
new12.63 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new513 bytes
new12.69 KB
FAILED: [[SimpleTest]]: [MySQL] 47,466 pass(es), 694 fail(s), and 121 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.78 KB
new14.34 KB
FAILED: [[SimpleTest]]: [MySQL] 50,692 pass(es), 65 fail(s), and 3 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
new15.17 KB
FAILED: [[SimpleTest]]: [MySQL] 51,128 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new766 bytes
new15.41 KB
PASSED: [[SimpleTest]]: [MySQL] 51,405 pass(es).
[ View ]

Oh well....

StatusFileSize
new8.28 KB
new19.73 KB
FAILED: [[SimpleTest]]: [MySQL] 52,258 pass(es), 29 fail(s), and 42 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
new33.13 KB
PASSED: [[SimpleTest]]: [MySQL] 52,295 pass(es).
[ View ]

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

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?

StatusFileSize
new19.44 KB
PASSED: [[SimpleTest]]: [MySQL] 52,564 pass(es).
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
new19.21 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here's a re-roll :)

StatusFileSize
new427 bytes
new19.23 KB
FAILED: [[SimpleTest]]: [MySQL] 46,646 pass(es), 161 fail(s), and 160 exception(s).
[ View ]

Netbeans did too much there.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
new18.95 KB
PASSED: [[SimpleTest]]: [MySQL] 53,352 pass(es).
[ View ]

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.

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.

StatusFileSize
new6.04 KB
new18.9 KB
FAILED: [[SimpleTest]]: [MySQL] 53,674 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

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.

StatusFileSize
new1.35 KB
new19.38 KB
PASSED: [[SimpleTest]]: [MySQL] 53,722 pass(es).
[ View ]

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.

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

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.

I did wonder, thanks for clearing it up.

StatusFileSize
new5.66 KB
new19.99 KB
FAILED: [[SimpleTest]]: [MySQL] 53,596 pass(es), 353 fail(s), and 30 exception(s).
[ View ]

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new20.07 KB
FAILED: [[SimpleTest]]: [MySQL] 54,349 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new21.99 KB
FAILED: [[SimpleTest]]: [MySQL] 54,990 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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.

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.

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?

StatusFileSize
new6 KB
new21.63 KB
FAILED: [[SimpleTest]]: [MySQL] 54,896 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Issue tags:+D8 cacheability

Tagging.

Status:Needs work» Needs review
StatusFileSize
new21.13 KB
FAILED: [[SimpleTest]]: [MySQL] 56,312 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.16 KB
new23.29 KB
PASSED: [[SimpleTest]]: [MySQL] 286 pass(es).
[ View ]

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

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

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

StatusFileSize
new10.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,068 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.52 KB
new12.61 KB
FAILED: [[SimpleTest]]: [MySQL] 58,062 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
new9.86 KB
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new399 bytes
new9.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,408 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new766 bytes
new9.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,129 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new26.21 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new726 bytes
new26.61 KB
FAILED: [[SimpleTest]]: [MySQL] 58,664 pass(es), 76 fail(s), and 8 exception(s).
[ View ]

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new14.19 KB
new39.66 KB
FAILED: [[SimpleTest]]: [MySQL] 59,059 pass(es), 15 fail(s), and 3 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
new40.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,711 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new40.1 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Just a re-roll for now.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.84 KB
new42.94 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Assigned:Unassigned» amateescu
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new52.06 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new12.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.

Status:Needs work» Needs review
StatusFileSize
new52.83 KB
FAILED: [[SimpleTest]]: [MySQL] 59,389 pass(es), 33 fail(s), and 1,055 exception(s).
[ View ]
new1.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.

Status:Needs work» Needs review
StatusFileSize
new59.17 KB
FAILED: [[SimpleTest]]: [MySQL] 58,506 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new6.99 KB

Some more progress.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new60.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,985 pass(es).
[ View ]
new1.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.

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.

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

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?

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.

StatusFileSize
new249.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch static-cache-state-1786490-131.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.95 KB

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.

StatusFileSize
new59.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es).
[ View ]

Rerolled.

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.

StatusFileSize
new638 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%

StatusFileSize
new59.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,079 pass(es).
[ View ]
new2.09 KB

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

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…

StatusFileSize
new59.41 KB
PASSED: [[SimpleTest]]: [MySQL] 59,009 pass(es).
[ View ]
new490 bytes

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

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.

Assigned:amateescu» Unassigned

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

Parent issue:» #1744302: [meta] Resolve known performance regressions in Drupal 8
StatusFileSize
new47.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,049 pass(es).
[ View ]
new12.98 KB

That's a fine point you have there.

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.

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

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.

StatusFileSize
new47.25 KB
FAILED: [[SimpleTest]]: [MySQL] 59,645 pass(es), 44 fail(s), and 514 exception(s).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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

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

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

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

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

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

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.

Assigned:Unassigned» catch
Priority:Normal» Major

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

Issue tags:+Performance

One more.

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.

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.

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.

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

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!

Status:Fixed» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new48.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,451 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new1.42 KB

I think this is the only problem.

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

StatusFileSize
new49.5 KB
PASSED: [[SimpleTest]]: [MySQL] 59,400 pass(es).
[ View ]
new971 bytes

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

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.