Add CacheBackendInterface::setMultiple()

Subscribe. I also did this for my initial CacheArray implementation.

Looking at catch's implementation, it looks to be more intelligent and only cache the parts which are actually required but a setMultiple() might still be useful for caches where a number of items are accessed on a single request.

+    catch (Exception $e) {
+      $transaction->rollback();
+    }

The other places in cache.inc where exceptions are caught, it is done to silently ignore the database being down. But if the database is down, there is no point in doing a rollback, is there? So what is the purpose? An inline comment would be appropriate.

(I think muting these exceptions is a bit fishy - I'll consider filing a ticket about that)

In deleteMultiple() we delete in chunks of a 1,000 entries. Did you consider doing the same in setMultiple() ? The chunking was introduced in #333171-12: Optimize the field cache in order to not create SQL queries that exceed the max_allowed_packet size. I guess it is unlikely that we add that many entries at once, so personally I don't think we need chunking here.

+ $this->assertTrue($cached['cid_1']->data == $items['cid_1']['data']);
I think assertEqual() is better in this place.

I suggest changing the test so the setMultiple() call will add a new entry and overwrite an existing one.

I think this issue has a lot of merit, but the patch is now miles off being applicable on the current code base. Planning to take a look at converting it today. There are a number of backends to consider now (which I guess has to be an enviable position to be in).

Attaching an attempt at incorporating these changes into the current state of core. I've needed to make a few changes:

• added implementations for all current backends
• added tags support to the previous implementation

I was unable to find a cleaner solution for MemoryBackend::setMultiple, as it always needs a foreach to fix up the incoming items (checksum and flatten tags), so might as well do the set in the foreach as well. MemoryBackend isn't the type that would hugely benefit from avoiding the foreach+set anyway.

Wondering if we should change set() in the database backend to be a wrapper of setMultiple(), just like get() is.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -114,6 +114,26 @@ interface CacheBackendInterface {
+   *       'expire' => CacheBackendInterface::CACHE_PERMANENT,
+   *       // @see CacheBackendInterface::set().
+   *       'tags' => array(),

I'm not sure about the rules of @see in inline comments but this might not make sense Because the purpose of @see's is to be added at the end of the description on api.drupal.org, this however should stay there. methods/functions are linked anyway.

Maybe "(optional) The cache tags for this item, see Cache..."?

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.phpundefined
@@ -280,6 +280,25 @@ abstract class GenericCacheBackendUnitTestBase extends UnitTestBase {
+    $cached = $backend->getMultiple($cids);
+    $this->assertEqual($cached['cid_1']->data, $items['cid_1']['data'], t('Over-written cache item set correctly.'));
+    $this->assertEqual($cached['cid_2']->data, $items['cid_2']['data'], t('New cache item set correctly.'));

Assertion messages should not use t().

Many thanks for the review!

Attaching corrections of the smaller suggestions. I don't have anything against the idea of making set a wrapper of setMultiple, but it will need to wait until I have the time to give it the care it needs. It does seem like something that would be worth keeping consistent if there's no downside (which would be worth a little checking). There is a small difference in implementations of the 2: set is a merge query, while setMultiple is a delete + insert.

#18: 1167248-cache-setmultiple.1.patch queued for re-testing.

Attaching a re-roll.

Converted ViewsDataCache to use this. I think we should get rid of that multiple set there and instead set the separate tables on demand, similar to CacheCollector but until then, it probably makes sense to use this.

This should fix the test failures.

+++ b/core/lib/Drupal/Core/Cache/BackendChain.phpundefined
@@ -130,6 +130,15 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+   * Implements Drupal\Core\Cache\CacheBackendInterface::setMultiple().

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -106,6 +106,15 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+   * Implements Drupal\Core\Cache\CacheBackendInterface::setMultiple().

+++ b/core/lib/Drupal/Core/Cache/NullBackend.phpundefined
@@ -48,6 +48,11 @@ public function getMultiple(&$cids, $allow_invalid = FALSE) {
+   * Implements Drupal\Core\Cache\CacheBackendInterface::setMultiple().

Misses starting "\".

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -146,6 +146,26 @@ public function getMultiple(&$cids, $allow_invalid = FALSE);
+   *       // (optional) The cache tags for this item, see CacheBackendInterface::set().

Does it make sense to add an additional @see CacheBackendInterface::set()

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -146,6 +146,26 @@ public function getMultiple(&$cids, $allow_invalid = FALSE);
+  function setMultiple(array $items);

Should should be probably be marked as a public function.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -149,6 +149,35 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+    catch (Exception $e) {
+      // The database may not be available, so we'll ignore errors.
+     }

Is there a reason why catch exceptions even we don't do it in all the other functions?

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -106,6 +106,15 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+      $this->set($cid, $item['data'], isset($item['expire']) ? $item['expire'] : CacheBackendInterface::CACHE_PERMANENT, isset($item['tags']) ? $item['tags'] : array());

Could we use $item += array('expire' => CacheBackend..., 'tags' => array()) , this would make it a bit easier to read.

+++ b/core/lib/Drupal/Core/Cache/NullBackend.phpundefined
@@ -48,6 +48,11 @@ public function getMultiple(&$cids, $allow_invalid = FALSE) {
+  function setMultiple(array $items = array()) {}

public ...

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.phpundefined
@@ -298,6 +298,25 @@ public function testGetMultiple() {
+   * Test Drupal\Core\Cache\CacheBackendInterface::setMultiple().

"\"

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -245,14 +227,15 @@ public function fetchBaseTables() {
   public function __destruct() {

As berdir mentioned in IRC this could have scalibility issues if you have for example 100 fields you end up with 200 potential cache tables and so inserts here.

Thanks for the great review.

- Fixed missing \
- Removed Exception, that's a left-over
- Added public
- Added defaults to $item, nice idea!

Unlike deleteMultiple(), there is no reliable way to make sure that the resulting string is not too large, a single item could be. So I think we shouldn't worry about that too much. And I want to get rid of that in case of views anyway once CacheCollector is in.

This looks really good now, though I don't feel to be in the position to RTBC it.

...

#29: cache-set-multiple-views-1167248-29.patch queued for re-testing.

#29: cache-set-multiple-views-1167248-29.patch queued for re-testing.

#29: cache-set-multiple-views-1167248-29.patch queued for re-testing.

#29: cache-set-multiple-views-1167248-29.patch queued for re-testing.

We actually want the transaction here (and in deleteMultiple), if only for performance. Without the transaction, you force the database to commit after *each* statement. With the transaction, the database can do a more efficient commit operation at the end.

Re-rolled, ViewsDataCache no longer needs this as it has been refactored quite a bit. Also using the injected database now and starting a transaction. I think we don't need to catch exceptions/do a rollback if something goes wrong. If the delete query fails then nothing happened anyway and if the insert fails then we actually want to delete, to not have old caches lying around?

#39: cache-set-multiple-1167248-39.patch queued for re-testing.

#39: cache-set-multiple-1167248-39.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Cache/BackendChain.phpundefined
@@ -130,6 +130,15 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+   * Implements \Drupal\Core\Cache\CacheBackendInterface::setMultiple().

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -160,6 +160,39 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+   * Implements \Drupal\Core\Cache\CacheBackendInterface::setMultiple().

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.phpundefined
@@ -106,6 +106,15 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+   * Implements \Drupal\Core\Cache\CacheBackendInterface::setMultiple().

+++ b/core/lib/Drupal/Core/Cache/NullBackend.phpundefined
@@ -48,6 +48,11 @@ public function getMultiple(&$cids, $allow_invalid = FALSE) {
+   * Implements \Drupal\Core\Cache\CacheBackendInterface::setMultiple().

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.phpundefined
@@ -298,6 +298,25 @@ public function testGetMultiple() {
+   * Tests \Drupal\Core\Cache\CacheBackendInterface::setMultiple().

Should be @inheritdoc now.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -149,6 +149,26 @@ public function getMultiple(&$cids, $allow_invalid = FALSE);
+   * @param $items

So it should be type hinted as array

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -160,6 +160,39 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+  function setMultiple(array $items) {

should be explicit marked as public.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -160,6 +160,39 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+    // Use a transaction so that the database can write the changes in a single
+    // commit.
+    $transaction = $this->connection->startTransaction();

We do care about creating the bin, if it did not exist before. The chance is low that you start with a setMultiple but you never know.

Yeah, I'm pretty sure the last patch predates @inheritdoc and dynamic table creation, didn't expect it to still apply ;)

#39: cache-set-multiple-1167248-39.patch queued for re-testing.

Straight reroll for now (patch succeeded with default fuzz), let's see how it goes and then probably still needs corrections from #43 ...

(Marking for review just for testing - most likely should be "needs work")

Style corrections from #43. Last comment still needs dealing with.

Looks like there may be a little extra from the set() method that needs pulling into setMultiple also.

This should fix that fail - was reliant on the fixed discrepancy in the return from flattenTags in MemoryBackend. Also converted a usage for this in \Drupal\Core\Config\CachedStorage.

Looks great! :)

Some simple remarks, but this is almost ready.

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -202,6 +202,76 @@ protected function doSet($cid, $data, $expire, $tags) {
   +      $this->deleteMultiple(array_keys($items));
   

   It would be valuable to have a comment here that explains the rationale for using multi delete + multi insert rather than many merge queries. (See #9, #11)

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -202,6 +202,76 @@ protected function doSet($cid, $data, $expire, $tags) {
   +        $flat_tags = $this->flattenTags($item['tags']);
   +
   +        // Remove tags that were already deleted or invalidated during this request
   +        // from the static caches so that another deletion or invalidation can
   +        // occur.
   +        foreach ($flat_tags as $tag) {
   +          if (isset($deleted_tags[$tag])) {
   +            unset($deleted_tags[$tag]);
   +          }
   +          if (isset($invalidated_tags[$tag])) {
   +            unset($invalidated_tags[$tag]);
   +          }
   +        }
   +
   +        $checksum = $this->checksumTags($flat_tags);
   +
   +        $fields = array(
   +          'cid' => $cid,
   +          'expire' => $item['expire'],
   +          'created' => REQUEST_TIME,
   +          'tags' => implode(' ', $flat_tags),
   +          'checksum_invalidations' => $checksum['invalidations'],
   +          'checksum_deletions' => $checksum['deletions'],
   +        );
   +
   +        if (!is_string($item['data'])) {
   +          $fields['data'] = serialize($item['data']);
   +          $fields['serialized'] = 1;
   +        }
   +        else {
   +          $fields['data'] = $item['data'];
   +          $fields['serialized'] = 0;
   +        }
   

   This entire chunk of code is *identical* to what's in DatabaseBackend::doSet(). Let's just split that into a protected ::prepareItemSet() method, and rename the existing ::prepareItem() to ::prepareItemGet()?

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.php
   @@ -295,6 +296,43 @@ public function testGetMultiple() {
   +   /**
   

   One leading space too many.

Thanks for the review.

Fixed the docs points. I think they are subtly different, although they reuse alot of the same code for sure. Plus we would have to call drupal_static alot more if this was extracted out? Otherwise I agree it might be nice to consider. What do you think about kicking that to a follow up?

Sounds good.

But then:

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -202,6 +201,78 @@ protected function doSet($cid, $data, $expire, $tags) {
+        // Remove tags that were already deleted or invalidated during this request

80 col.

And in your interdiff:

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -179,7 +179,6 @@ protected function doSet($cid, $data, $expire, $tags) {
-      'serialized' => 0,

Nice subtle clean-up :)

Fixed the comment and reverted that change in doSet() I don't see an issue doing that one line here, it will go into a followup anyway... :)

Looks good to me. I agree that adding setMultiple() makes sense. Also looks like we found an actual first use-case for it, which is even better :-)

That said, the associative array values are bit poor/strange, but we can't do anything about that here without introducing proper CacheItem objects first, and that should not hold up this patch.

Thanks, and agreed. If we add a proper classed cache item. We can change it then.

#62: ooooh! Yes! Following!