CacheArray is a neat trick that was introduced in 7.x to introduce backwards compatible partially loaded large cache arrays.

As discussed in #1786490: Add caching to the state system, there is no need for that in 8.x for at least the new implementations, so let's make this implementation with interface that we can use elsewhere too.

The long term goal is probably to get rid of CacheArray completely but that doesn't need to happen here.

Follow up: #2185015: Remove SchemaCache and CacheArray

Files: 
CommentFileSizeAuthor
#27 cache-collector-1858616-27.patch43.59 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,244 pass(es).
[ View ]
#27 cache-collector-1858616-27-interdiff.txt4.76 KBBerdir
#18 cache-collector-1858616-18.patch41.05 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache-collector-1858616-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 cache-collector-1858616-18-interdiff.txt1.95 KBBerdir
#16 cache-collector-lazyload-1858616-16.patch40.63 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,460 pass(es).
[ View ]
#16 cache-collector-lazyload-1858616-16-interdiff.txt9.42 KBBerdir
#14 cache-collector-path-whitelist-1858616-14.patch40.57 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,587 pass(es).
[ View ]
#14 cache-collector-path-whitelist-1858616-14-interdiff.txt15.48 KBBerdir
#11 cache-collector-path-whitelist-1858616-11.patch26.55 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,401 pass(es).
[ View ]
#11 cache-collector-path-whitelist-1858616-11-interdiff.txt4.07 KBBerdir
#9 cache-collector-path-whitelist-1858616-9.patch26.46 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,854 pass(es).
[ View ]
#9 cache-collector-path-whitelist-1858616-9-interdiff.txt806 bytesBerdir
#7 cache-collector-path-whitelist-1858616-7.patch26.47 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#7 cache-collector-path-whitelist-1858616-7-interdiff.txt8.11 KBBerdir
#5 cache-collector-path-whitelist-1858616-5.patch20.12 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,888 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#2 cache-collector-1858616-2.patch7.74 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 48,761 pass(es).
[ View ]

Comments

catch’s picture

Yes anything that doesn't need backwards compatibility with array syntax shouldn't be using ArrayAccess really, we should also be able to see a very small performance improvement here by skipping the magic methods.

On top of this there's the potential to have alternative implementations of the base class (like LRU instead of additive, although that's going to be hard to get right without too many writes but someone could try).

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new7.74 KB
PASSED: [[SimpleTest]]: [MySQL] 48,761 pass(es).
[ View ]

Here's a first attempt. Defined the interface and the implementation and also reworked the existing documentation in CacheArray, moved generic documentation to the interface and implementation specific stuff to the class. Likely needs more work.

I've prepared it for the ServiceTerminator pattern introduced in the form_state issue and it also gets cache and lock injected.

I initially planned to refactor CacheArray to use the implementation provided by this but this turned out to be not as easy as I hoped, mostly because ThemeRegistry contains quite some overrides that would have to be changed as well.

No tests yet. Not sure if I should re-introduce some or just merge this back into #1786490: Add caching to the state system and write coverage for that. As everything is injected now it should actually be possible to write fast unit tests for this that don't have to touch the database. But doing it here would require us to write a test implementation that implements the abstract method.

Fabianx’s picture

Issue tags:+Performance

Tagging

Berdir’s picture

Status:Needs review» Closed (duplicate)

Closing this, integrated and improved the patch here into #1786490: Add caching to the state system.

Berdir’s picture

Status:Closed (duplicate)» Needs review
StatusFileSize
new20.12 KB
FAILED: [[SimpleTest]]: [MySQL] 57,888 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Reviving this, as suggested by catch, fighting with the state cache and not making progress.

We have multiple CacheArray implementations in HEAD now that live in the container, updated path whitelist cache as a first example. Tests seem to almost pass except one case in PathLanguageTest that I haven't figured out yet.

Status:Needs review» Needs work

The last submitted patch, cache-collector-path-whitelist-1858616-5.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new8.11 KB
new26.47 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Ok, we also need a method to actually clear the cache, not just reset the currently loaded cache entry. Also converted the locale stuff over.

Status:Needs review» Needs work

The last submitted patch, cache-collector-path-whitelist-1858616-7.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new806 bytes
new26.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,854 pass(es).
[ View ]

This should be better.

ParisLiakos’s picture

Status:Needs review» Needs work

this looks great! most of them are nitpicks:)

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,276 @@
+   * Implements CacheCollectorInterface::set().

full namespace needed

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,276 @@
+    // Invalidate the cache to make sure that other requests immediately see the
+    // deletion before this request is terminated.
+    $this->cache->invalidate($this->cid);
+    $this->cacheInvalidated = TRUE;
...
+    // Invalidate the cache to make sure that other requests immediately see the
+    // deletion before this request is terminated.
+    $this->cache->invalidate($this->cid);
+    $this->cacheInvalidated = TRUE;

maybe a protected method invalidate to avoid duplication?

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,276 @@
+          // cache entry, do not attempt to overwrite data that might have been

Probably the comma here should be a dot

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorInterface.phpundefined
@@ -0,0 +1,79 @@
+ * The default implementation is CacheCollector.

we should use the full namespace to CacheCollector

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

lowercase first on Interface

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.phpundefined
@@ -39,9 +39,34 @@ class LocaleLookup extends CacheArray implements DestructableInterface {
    * Constructs a LocaleCache object.

uh just noticed we have the wrong object name here. could we fix this since we are here?

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
new26.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,401 pass(es).
[ View ]

Thanks, fixed those things and added a method.

ParisLiakos’s picture

Issue tags:+Needs tests, +phpunit

this needs some unit tests, probably on AliasWhitelist, tagging

catch’s picture

+   * A cid to pass to cache()->set() and cache()->get().
+   *
+   * @var string

Since the cache object is injected should the comment be updated as well?

Berdir’s picture

StatusFileSize
new15.48 KB
new40.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,587 pass(es).
[ View ]

Ok. Fixed those comments and wrote a ton of unit tests. The method names and documentation there needs work, but I have 100% code coverage of the CacheCollector class.

Found one issue, when you only did a delete() and no set(), then it didn't update the cache.

ParisLiakos’s picture

Issue tags:-Needs tests

not very thorough review, but they look great!

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,405 @@
+  function testResolveCacheMiss() {
...
+  function testSetAndGet() {
...
+  function testSetAndGetFalse() {
...
+  function testGetFromCache() {
...
+  function testDelete() {
...
+  function testUpdateCacheNoChanges() {
...
+  function testUpdateCache() {
...
+  function testUpdateCacheLockFail() {
...
+  function testUpdateCacheInvalidatedConflict() {
...
+  function testUpdateCacheMerge() {
...
+  function testUpdateCacheDelete() {
...
+  function testUpdateCacheReset() {
...
+  function testUpdateCacheClear() {

lets add some visibility here

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,405 @@
+    $this->constructCollector();

i see you do this on most tests..why not put it in setUp() ? and still call it when you want to override it later

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,405 @@
+    $key = $this->randomName();
+    $value = NULL;
+
+    $this->constructCollector();
+    $key = $this->randomName();
+    $value = $this->randomName();

you set $key and $value twice

Berdir’s picture

StatusFileSize
new9.42 KB
new40.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,460 pass(es).
[ View ]

Thanks for the review.

- Made them public
- Fixed the double-definition, that uncovered that the test method and has() was actually broken, fixed.
- The reason for constructCollector() was that in some cases, I set up the cache mock first. I could have re-created it but thought that to be a bit weird.
- This patches switches to lazy-loading, which allows us to not initially get the cache but only when we need it. That also fixes the reset() weirdness and it allows me to kill constructCollector().

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+ * Tests the cache CacheCollector.
+ *
+ * @group Cache

I really like to have @see \Drupal\Core\Cache\CacheCollector so you can switch between them really easy.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+class CacheCollectorTest extends UnitTestCase {

This is an amazing test!!

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+   * @var \PHPUnit_Framework_MockObject_MockObject
...
+   * @var \PHPUnit_Framework_MockObject_MockObject

All the other tests actually document on the actual interface/class which is mocked, which gives you a better understanding what is going on.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+    $this->collector->destruct();
...
+    $this->collector->destruct();

The setup method is called for each test methods, so I don't understand why this i needed.

Berdir’s picture

StatusFileSize
new1.95 KB
new41.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache-collector-1858616-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Added the @see and a comment line to explain the destruct. It's not clean-up/tear down, updateData() is a protected method called by destruct(), so the call to it triggers the actual thing we're testing in those methods.

Not sure about the @var. Documenting it as a mock object made it possible for me to have autocomplete on it for expects(), which is the only way I'm interacting with it within the test. As the tests are now written, I don't care which one it is, but using what I have in the patch makes more sense to me :)

ParisLiakos’s picture

i agree about the @var with berdir
i think i am not the right person to rtbc this, but imo its ready

dawehner’s picture

Yeah I don't care that much either.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,307 @@
+   * Implements \Drupal\Core\Cache\CacheCollectorInterface::set().

Oh i guess it should be one @inheritdoc?

Berdir’s picture

Well, then we can't have the additional documentation that's below it in the method docblock and would have to move it inline, where it's harder to find.

I know @timplunkett wants to allow partial inheritdoc and extend it, but I'm not sure if that makes sense conceptually for cases like this (I don't want to have both things merged together somehow, a reference to the interface and this information below seems like exactly what I want here).

So, I'd rather not touch that.

Berdir’s picture

Priority:Normal» Major

This is blocking major/critical performance issues, e.g. state caching and if we want to remove CacheArray, this needs to go on, asap.

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

lets get it in then

Berdir’s picture

Issue tags:-Performance, -phpunit

#18: cache-collector-1858616-18.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Performance, +phpunit

The last submitted patch, cache-collector-1858616-18.patch, failed testing.

catch’s picture

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

After this->get() the array_key_exists() will always return TRUE, because items that don't exist get stored as NULL at least with the default implementation. Shouldn't this just be an isset()?

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,307 @@
+    // Lock cache writes to help avoid stampedes.
+    // To implement locking for cache misses, override __construct().
+    $lock_name = $this->cid . ':' . __CLASS__;
+    if (!$lock || $this->lock->acquire($lock_name)) {

This comment is just from CacheArray, but it's no longer accurate now that the cache item is loaded in lazyLoadCache.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,307 @@
+          $this->cache->delete($this->cid);

Shouldn't this be deleteTags() if tags were passed in? At the moment it looks like if you have multiple cache items with tags, and a process calls delete on one of them, the rest won't be invalidated.

+++ b/core/lib/Drupal/Core/Path/AliasWhitelist.phpundefined
@@ -7,15 +7,19 @@
-class AliasWhitelist extends CacheArray implements DestructableInterface {

Yes! I was so pleased to get CacheArray into Drupal 8/7, but it's so good we can remove it from Drupal 8.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,388 @@
+  /**
+   * Tests updating the cache when there is a
+   */

Incomplete comment: there is a?

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,388 @@
+
+    // Set up mock objects for the expected calls, first a lock acquire, then
+    // cache get to look for conflicting cache entries, which does find

Really nice tests except possibly for the multiple cache + tags case.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new4.76 KB
new43.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,244 pass(es).
[ View ]

Re-roll.

Hm. The array_key_exists() check is taken from CacheArray. I'm not sure what the expected behavior should be there. The check makes sense because it allows to set values to NULL and avoid to re-trigger cache misses for those on future requests. That said, this doesn't happen by default and is the main reason why subclasses currently need to set/persist their stuff in resolveCacheMiss() themself.

I added a basic test for that. I'm happy to change it, just not quite sure what would be a sane behavior? Maybe store NULL's by default (and therefore simplify resolveCacheMiss() implementations) but keep returning FALSE in has()? Right now, subclasses can already do that but they explicitly need to chose to do so.

Added cache delete support for tags and tests for it, updated the comment.

Yes! I was so pleased to get CacheArray into Drupal 8/7, but it's so good we can remove it from Drupal 8.

Yes and yes to that :)

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

back to catch then!:)

catch’s picture

Title:Extract generic CacheCollector implementation and interface from CacheArray» Change notice: Extract generic CacheCollector implementation and interface from CacheArray
Priority:Major» Critical
Status:Reviewed & tested by the community» Active

Hmm. I guess either way is fine for has(), and it's only used in tests in the patch, so as long as we pick one it's up to implementations to figure that out anyway.

Committed/pushed to 8.x, will need a change notice.

catch’s picture

Issue tags:+Needs change record

Tagging.

catch’s picture

cosmicdreams’s picture

Soooooooooo, I was just about to open an issue about removing CacheArray because I noticed it was deprecated and still used in ModuleInfo.php and SchemaCache.php. Also it was included in code comments in AliasWhitelist.php and TestBase.php. Also the CacheArray class still exists.

So is it better to create a follow up issue for the elimination of CacheArray from Drupal or is it better to piggyback onto here?

cosmicdreams’s picture

Also, I suppose I could try to write a change notice for this one once we clean up.

cosmicdreams’s picture

xjm’s picture

Issue tags:+Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

The patch for this issue was committed on June 21, 2013, meaning that the change record has been missing for more than seven months.

Berdir’s picture

Status:Active» Needs review

Wow, I completely forgot about this one.

Tried to make up for it by providing a detailed change notice, we can probably more or less move that 1:1 to a documentation page somewhere, just no idea where ;)

https://drupal.org/node/2186501

Berdir’s picture

Title:Change notice: Extract generic CacheCollector implementation and interface from CacheArray» Extract generic CacheCollector implementation and interface from CacheArray
Status:Needs review» Fixed
Issue tags:-Needs change record, -Missing change record

@xjm verified that the change notice is ok, so closing this.

Status:Fixed» Closed (fixed)

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