Improve performance of ViewsDataCache

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -38,39 +45,40 @@ class ViewsDataCache implements DestructableInterface {
+   * Whether all table cache should be rebuilt. This is set when getData() is
+   * called.
...
+  protected $rebuildAll = FALSE;

This comment about getData() doesn't seem to be true anymore with the updated code, as it actually matters whether get() was called without a specific table.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -81,44 +89,48 @@ public function __construct(CacheBackendInterface $cache_backend, ConfigFactory
+          $from_cache = TRUE;
...
         else {
           // No cache entry, rebuild.
           $this->storage = $this->getData();
-          $this->fullyLoaded = TRUE;
+          $from_cache = FALSE;
...
+        if (!$from_cache) {
+          // Add this table to a list of requested tables, as it's table cache
+          // entry was not found.
+          array_push($this->requestedTables, $key);

Couldn't we just push directly in the else?

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -128,19 +140,17 @@ public function get($key = NULL) {
+  public function set($cid, $value) {

I'm wondering whether we should have one set() method for a single table and one for all, as set() is public, it shouldn't care about internal implementation details(the fact that we have a cache).

Tagging.

This looks really really good!

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -31,6 +31,13 @@ class ViewsDataCache implements DestructableInterface {
+   * The configuration factory object.
+   *
+   * @var \Drupal\Core\Config\ConfigFactory
+   */
+  protected $config;

The config is justed used in the constructor, so what about not storing it at all? (Probably a follow up)

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -38,39 +45,39 @@ class ViewsDataCache implements DestructableInterface {
+   * Whether views data has been rebuilt. This is set when getData() doesn't return anything from cache.

Over 80 chars

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -81,44 +88,48 @@ public function __construct(CacheBackendInterface $cache_backend, ConfigFactory
+          array_push($this->requestedTables, $key);

Why array_push() and not a simple $this->requestedTables[] = $key?

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -128,19 +139,17 @@ public function get($key = NULL) {
-  public function set($key, $value) {
+  protected function set($cid, $value) {

How useful are the set and cacheGet() methods actually with this? The language suffix handling is a separate helper method now, the only thing they do is the skipCache check, which could just as well be directly in destruct() because it would save us from looping over the requested tables and then not actually saving them.

I have a separate issue that adds a setMultiple() where I already killed the set method so that I can do a single set for all requested cache tables.

Other than that, agreed that this looks nice.

--- /dev/null
+++ b/core/.idea/.name

Oops.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.php
@@ -31,6 +31,13 @@ class ViewsDataCache implements DestructableInterface {
+  protected $config;

Not used anymore.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.php
@@ -128,19 +139,17 @@ public function get($key = NULL) {
+  protected function set($cid, $value) {

This could be named cacheSet.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.php
@@ -128,19 +139,17 @@ public function get($key = NULL) {
+    $this->cacheBackend->set($this->prepareCid($key), $value);

$key is undefined here.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.php
@@ -158,26 +167,47 @@ protected function cacheGet($cid) {
+    return $cid .= ':' . $this->langcode;

This does not need to assign $cid.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.php
@@ -158,26 +167,47 @@ protected function cacheGet($cid) {
+      // Set as TRUE, so all table data will be cached.
+      $this->rebuildAll = TRUE;

Would it make sense to do $this->cacheSet($data) here so parallel requests would read the data from cache?

.

Tested this manually and it works lovely. Only a handful of views_data:$table:en cache entries are written (node, node_revision, taxonomy_term, users, file_managed, comments), clicking around in the views UI relies on the full cache entry and does write any additional cache entries except the one for views itself. Awesome!

That said, I still think the set() could be inlined in destroy() and the skipCache checked moved into the existing !empty($this->storage) if. Will be easier for me to re-roll the cache()->setMultiple() patch afterwards.

Yeah it's not public method anymore, so it seems okay to not overabstract that.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -268,16 +252,16 @@ public function fetchBaseTables() {
-        $this->cacheSet($cid, $this->storage[$table]);
+        $this->cacheBackend->set($this->prepareCid($cid), $this->storage);

Should be $this->storage[$table] in the new set() call.

Good catch!

hehe

Ok, this looks great to me. Waiting for the promised tests by @damiankloip but this looks RTBC to me otherwise. One important thing to test would be to make sure that multiple requests for a missing table definition does not result in multiple views data rebuilds/writes, a thing that is currently plaguing 7.x-3.x

Also marking as needs backport, while the API itself is completely different, this would be extremely valuable thing to have in 7.x-3.x

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.phpundefined
@@ -42,35 +50,98 @@ protected function setUp() {
+    // Expected data is that invoked directly from hook_views_data.

Can't really parse that sentence.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.phpundefined
@@ -42,35 +50,98 @@ protected function setUp() {
+  protected function assertCountIncrement($equal = FALSE) {

Minor but when reading how it was used, I was wondering if the argument should be inverted to $increment = TRUE. I think that would make more sense, reading something like assertCountIncrement(TRUE) sounds like it would actually test that it was incremented.

Great, thanks for the quick re-roll. Great work, I think this is ready to go. Very happy to have this resolved for 8.x, now we just need to find a way to backport what we can :)

#27: 1944674-27.patch queued for re-testing.

Looks much better! Committed/pushed to 8.x.

Moving to 7.x for backport.

Seeing the per-table cache in action on a big, message site, I think this is major.

Here's a start to implement this in 7.x-3.x. No tests yet, sadly, but seems to be working quite fine.

Grml, same patch with correct prefix.

Had to update the if statement to not ignore empty tables.

Yes, we did :)

Thanks for even provide tests!

+++ b/includes/cache.incundefined
@@ -23,16 +23,32 @@ function _views_fetch_data($table = NULL, $move = TRUE, $reset = FALSE) {
+          $data = views_cache_get('views_data', TRUE);
+          if (!empty($data->data)) {

Could we also merge these two lines?

Sure.

Thank you very much.

+++ b/includes/cache.inc
@@ -82,11 +97,6 @@ function _views_fetch_data_build() {
-  // Save data in seperate cache entries.
-  foreach ($cache as $key => $data) {
-    $cid = 'views_data:' . $key;
-    views_cache_set($cid, $data, TRUE);
-  }

Removing this seems to have broken views cache with memcache: https://www.drupal.org/node/2153517#comment-9337341

The patch in #40, since committed and included in 7-3.8, has been causing trouble for my site and others. See here for more:

#2153517: Randomly getting broken/missing handler
#2305905: Massive query after cloning aggregator views crashes site
and potentially #1954348: Fields not available anymore in fields add (cache issue)

As nicholasruunu points out in #40, the removal of this code seems to be causing the problem:

foreach ($cache as $key => $data) {
  $cid = 'views_data:' . $key;
  views_cache_set($cid, $data, TRUE);
}

Adding that code back to cache.inc seems to solve the problem. Without it, my site crashes and 7-3.8 becomes unusable and we have to revert to 7-3.7, which throws constant security warnings. Please help.

Here is a patch that puts that code back into the latest dev version.

#45 @damiankloip,

Seems like it, patch has been working for us the last 17 days.

@damiankloip, @nicholasruunu:

Seems like a lot of the reports include memcache - but we don't use memcache (neither do a few of the users in some of the above linked issues), so that can't be it. The fix in #40/41 seems to work though, but I don't know the implications (why it was removed, etc).

For those of us who it was memcache related, they just released memcache-7.x-1.4 today which includes the patch for #435694: >1M data writes incompatible with memcached (memcached -I 32M -m32) and has resolved this issue for us.

My site also heavily depends on views but doesn't use memcache. Since upgrading to 3.8 random view data would go missing and the admin view config screen would display the field as broken handler until the cache is cleared.

I have applied the patch in #42 and will see how it goes but I would also be interested in why this was removed.

MERCI ! :-)
Patch #42 worked for me,
for errors like "This view has been automatically updated to fix missing relationships" on editing views
with D 7.34 + PHP 5.6.5 and MariaDB 5.5.5-10.0.15

+++ b/includes/cache.inc
@@ -82,11 +97,6 @@ function _views_fetch_data_build() {
-  // Save data in seperate cache entries.
-  foreach ($cache as $key => $data) {
-    $cid = 'views_data:' . $key;
-    views_cache_set($cid, $data, TRUE);
-  }

This seemed like a much more scalable approach, may I ask why was it removed? Seems from reading the Issue Summary removing the line above that hunk would be a much better approach?

+++ b/includes/cache.inc
@@ -97,6 +97,11 @@ function _views_fetch_data_build() {
   // Keep a record with all data.
   views_cache_set('views_data', $cache, TRUE);

Right now the views_data cid on a commerce site, with many fields and modules is > 2.5MB

To make #42 work maybe we need to also remove the save all data, and if all the keys are needed maybe just save the keys that are saved to help get 'views_data:' . $key.

The patch in #42 solved the broken/missing handler problem for me, thanks mxwright!

We are seeing a similar issue where Views Blocks would suddenly vanish. We rely on Views for a vast majority of the site, so I would very much like to see this resolved. I'll test out the patch in #42, but the comments in #51 make me think this patch may still need some work? Would love to get a maintainer's thoughts.

Memcache / Redis users: There's another issue with _views_fetch_data() that affects you: #2475669: _views_fetch_data() prone to inconsistency (Especially with redis / memcache)

Some thoughts

Imagine this scenario:

1. Initial Request - cold cache:
   _views_fetch_data() triggers a full rebuild -> _views_fetch_data_build()
   We end up with something like:
   views_data:en = array('table_a' => array(), 'table_b' => array(), 'table_c' => array());
   views_data:table_a:en = array()
   views_data:table_b:en = array()
   views_data:table_c:en = array()
2. Lots of traffic, cache is filled:
   Redis (LRU config) / memcache will start to evict items from cache to make space for new stuff.
   Imagine views_data:table_b:en and views_data:table_b:en are deleted from the cache.
3. Next request _views_fetch_data() runs
   views_data:table_b:en is a miss, and triggers the full load of the views data cache, which fills the static cache.
   As this is registered as a lost views_data:table_b:en - the cache entry is restored.
   But views_data:table_c:en is a hit now! Since the static cache is in charge now. Thus views_data:table_c:en isn't considered "lost".
4. Next request _views_fetch_data() runs
   Now views_data:table_b:en is a hit but views_data:table_c:en is a miss, triggers the full load of the views data cache and its dedicated cache item is restored too.

First this means we would need quite long to "restore" the missing table specific cache items - one per request.
Second, if you're running memcache with the 1M item limit, absolutely every call would lead to a rebuild of the full cache.

So in perspective of this it would make sense to re-add the code that sets the table specific cache items whenever the full cache is rebuild.
However, for "Second" I guess in a real world scenario you'll always end up with a call to fetch the whole cache, which negates the adjustment again. And I really don't see another solution to this than adjust the memcache configuration (Think of it as the size of the table column data in the db based caching, you can't just change this to varchar - it has to be a longblob to work.)

Second, if you're running memcache with the 1M item limit, absolutely every call would lead to a rebuild of the full cache.

Well, at least with version 1.5 of memcache module that particular problem is solved, as big entries will be split up automatically.

@dawehner Well I guess then it's mostly about if we want to have a ton of extra cache_set() calls when the full cache is rebuild. Which translates to a ton of db queries when the standard caching backend is used (in my case this would be around 530 calls for the project I work on atm ;) ). As I try to always use an in-memory cache backend I wouldn't mind to have the additional sets.

I've updated to Views 3.11 and tested it at length against the trouble I reported above. Everything now seems resolved. Without further information I'll have to attribute this to various seemingly-related things mentioned in the release notes:

• by das-peter: Ensure that we do not end up with broken views data cache entries.
• #2018737 by oriol_e9g, johnv, david_garcia, klausi, forgenator: Notice: Undefined index: data in views_plugin_cache->restore_headers()
• #1376400 by fietserwin: Cache views plugin data

And return this issue back to 'Fixed' status.