Add a cache collector style backend to reduce the number of calls to cache backends

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
@@ -0,0 +1,398 @@
+  /**
+   * Constructs a CacheCollectorBackend.
+   */
+  public function __construct($cid, CacheBackendInterface $cache_backend, LockBackendInterface $lock, array $default_tags = array()) {

Let's start with a nitpick, document it.

On top of that I am not convinced that you should store all the entries in the collection but rather manually configure which one you want to load.

1. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,398 @@
   +  public function delete($cid) {
   +    $this->lazyLoadCache();
   +    unset($this->storage[$cid]);
   +    $this->keysToRemove[$cid] = $cid;
   +    // The key might have been marked for persisting.
   +    unset($this->keysToPersist[$cid]);
   +    $this->invalidateCache();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function deleteMultiple(array $cids) {
   +    $this->lazyLoadCache();
   +
   +    foreach ($cids as $cid) {
   +      unset($this->storage[$cid]);
   +      $this->keysToRemove[$cid] = $cid;
   +      // The key might have been marked for persisting.
   +      unset($this->keysToPersist[$cid]);
   +    }
   +
   +    $this->invalidateCache();
   +  }
   

   You could save some code here.

2. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,398 @@
   +    $this->reset();
   ...
   +  public function deleteTags(array $tags) {
   ...
   +    $this->reset();
   ...
   +  public function invalidate($cid) {
   ...
   +    $this->reset();
   ...
   +    $this->reset();
   

   It also feels odd to reset all of them, as you could also just invalidate some of them (not for tags.)

3. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,398 @@
   +  protected function lazyLoadCache() {
   ...
   +
   +    if ($cache = $this->cacheBackend->get($this->getCid())) {
   

   It is odd that you request things, even if you just ask for just other CIDs.

4. +++ b/core/modules/views/views.services.yml
   @@ -1,61 +1,66 @@
        class: Drupal\views\Plugin\ViewsPluginManager
   -    arguments: [access, '@container.namespaces', '@cache.discovery', '@module_handler']
   +    arguments: [access, '@container.namespaces', '@cache.views_plugin_collector', '@module_handler']
   

   did we considered to use the parent functionality of containers here? Note: we could move the plugin ID to the end, and be done.

Here are some minor doc issues. Do we need profiling here?

1. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   + * This backend acts as a proxy to aggregate all cache items into a single cache
   + * entry. This results in a single get and set call to the actual cache backend.
   

   more then 80 chars.

2. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   +   *   (optional) Whether to acquire a lock before writing to cache. Defaults to
   ...
   +      // an eventually invalidated cache entry, only update an invalidated cache
   

   more then 8 chars.

3. +++ b/core/modules/views/src/Plugin/ViewsPluginManager.php
   @@ -31,8 +29,11 @@ class ViewsPluginManager extends DefaultPluginManager {
   +   *
   

   Extra space.

4. +++ b/core/modules/views/src/Plugin/ViewsPluginManager.php
   @@ -31,8 +29,11 @@ class ViewsPluginManager extends DefaultPluginManager {
   +   *   The plugin type. For example, 'display'.
   

   space missing after this.

It'll fix an exception and #9.

1. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   +    $this->lazyLoadCache();
   +    if (isset($this->storage[$cid]) || array_key_exists($cid, $this->storage)) {
   +      $cache = $this->createCacheObject($this->storage[$cid]);
   

   I'm curious whether the individual cache objects should be stored in $this->storage directly?

2. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   +    $cache = [];
   +
   +    foreach ($cids as $cid) {
   +      $cache[$cid] = $this->get($cid, $allow_invalid);
   +    }
   +
   +    $cids = array_diff($cids, array_keys($cache));
   

   When we store the actual objects we could reduce this to array_intersect_keys

3. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   +    $deleted_tags = &drupal_static('Drupal\Core\Cache\DatabaseBackend::deletedTags', []);
   +    $invalidated_tags = &drupal_static('Drupal\Core\Cache\DatabaseBackend::invalidatedTags', []);
   +
   

   We no longer have deleted && invalidated tags, but just the later one.

4. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   +      Cache::mergeTags($this->tags, array_unique($item['tags']));
   

   What is the reason for this array_unique call? Why should an item ever have multiple times the same? At least \Drupal\Core\Cache\DatabaseBackend::doSet does not care about that.

5. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   +  public function delete($cid) {
   +    $this->lazyLoadCache();
   

   Do we need the lazyLoadCache call here?

6. +++ b/core/lib/Drupal/Core/Cache/CacheCollectorBackend.php
   @@ -0,0 +1,433 @@
   +    // Check expire time.
   +    $cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= REQUEST_TIME;
   +
   

   Ah, so we can't cache the object itself.

7. +++ b/core/modules/system/src/Tests/Cache/CacheCollectorBackendUnitTest.php
   @@ -0,0 +1,37 @@
   + * Definition of Drupal\system\Tests\Cache\CacheCollectorBackendUnitTest.
   

   Nitpick

8. +++ b/core/modules/views/src/Plugin/ViewsPluginManager.php
   @@ -31,8 +29,11 @@ class ViewsPluginManager extends DefaultPluginManager {
   -  public function __construct($type, \Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
   +  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, $type) {
   
   +++ b/core/modules/views/views.services.yml
   @@ -1,61 +1,74 @@
   +    parent: views.plugin.base
   +    arguments: [access]
   

   I'd not expected this to work, so arguments are appended?

9. +++ b/core/modules/views/views.services.yml
   @@ -1,61 +1,74 @@
   +    parent: views.plugin.base
   

   <3

So a question:

What is the difference between \Drupal\Core\Cache\CacheCollectorBackend and \Drupal\Core\Cache\CacheCollector then?

Can / should we consolidate those?

Overall I am +1 to making the Cache Collector a backend instead of a class (CacheArray before).

reroll first, will look at failures and #12 after

Ugh, wrong patch

We can't merge the two, the have a different purpose.

CacheCollector is a generic concept and an abstract class, everything that uses it needs to subclass it to implement certain methods.

Maybe we need to find a better name for this, but not sure what that would be.

We could only do this in a minor release I think given it's an API addition. Since it will save a lot of i/o, bumping to major.

#19: Yes and no, the core CacheCollector class could use - as discussed in IRC - use the CacheCollectorBackend class added here as backend instead and remove a lot of duplicate code.

@Damian also will open a CacheMultipleBackend.php issue, which might be better suited, than a cache collector backend here.

IMHO we can mark that as a duplicate of #2473205: Create a cache backend that pre loads multiple items in one getMultiple() call now

I agree to close this as duplicate.