Replace _views_fetch_data with cache class

Tagging

x-post

Awesome stuff!

+++ b/core/modules/views/lib/Drupal/views/Plugin/Type/DefaultWizardDeriver.phpundefined
@@ -38,7 +38,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+      $views_info = drupal_container()->get('views.views_data')->get($table);

+++ b/core/modules/views/views_ui/admin.incundefined
@@ -2298,7 +2298,7 @@ function views_ui_field_list() {
+            $data = drupal_container()->get('views.views_data')->get($item['table']);

It would be possible to just get the views data object once and then use that object.

+++ b/core/modules/views/lib/Drupal/views/ViewsBundle.phpundefined
@@ -32,6 +32,8 @@ public function build(ContainerBuilder $container) {
+    $container->register('views.views_data', 'Drupal\views\ViewsDataCache');

Inject all the things!

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -0,0 +1,195 @@
+    $this->skipCache = config('views.settings')->get('skip_cache');

Yeah if we put this into the constructor on the container this information could be used without a special config call.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -0,0 +1,195 @@
+    cache($this->bin)->set($key, $value);
...
+    return cache($this->bin)->get($cid);

What about injecting the cache object via the container?

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -0,0 +1,195 @@
+    foreach (module_implements('views_data_alter') as $module) {
+      $function = $module . '_views_data_alter';
+      $function($data);

Just in general: is there a reason to not use drupal_alter?

Great improvements!

+++ b/core/modules/views/views_ui/admin.incundefined
@@ -2286,6 +2286,7 @@ function views_ui_form_button_was_clicked($element, &$form_state) {
+  $data = drupal_container()->get('views.views_data')->get();

@@ -2298,12 +2299,12 @@ function views_ui_field_list() {
+            $table_data = $data[$item['table']];

We should better use ->get($item['table']) to not require to load all of the data all the time.

A couple nitpicks, but I just fixed them myself. I think @damiankloip said he wanted Berdir to look this over for some benchmarks, so I won't RTBC yet, but I like this patch a lot.

This looks nearly perfect so far!

function views_block_info() {
// Try to avoid instantiating all the views just to get the blocks info.
views_include('cache');

views_include('cache');
// Cache for unpackOptions, but not if we are in the ui.

So there is no need for these two calls anymore

+++ b/core/modules/views/views.moduleundefined
@@ -1154,8 +1154,8 @@ function views_library_info() {
+  $data = drupal_container()->get('views.views_data')->get($table);
...
-  $data = views_fetch_data($table);

+++ b/core/modules/views/views_ui/admin.incundefined
@@ -2298,12 +2299,12 @@ function views_ui_field_list() {
+            $table_data = $data->get($item['table']);
+            if (isset($table_data[$item['field']]) && isset($table_data[$item['field']][$type])
+              && $field_data = $table_data[$item['field']][$type]) {
...
+              if (isset($field_data['field_name'])) {
+                $fields[$field_data['field_name']][$view->get('name')] = $view->get('name');

Nitpick: The empty line seems to make more sense in the current code.

So basically tim fixed the stuff already :)

Let's get some proper benchmarking and then RTBC it.

Restoring tags.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/relationship/GroupwiseMax.phpundefined
@@ -93,7 +93,7 @@ public function buildOptionsForm(&$form, &$form_state) {
       $sort_options[$sort_id] = "$sort[group]: $sort[title]";
     }
-    $base_table_data = views_fetch_data($this->definition['base']);
+    $base_table_data = drupal_container()->get('views.views_data')->get($this->definition['base']);

We're just switching one function call with another one so it's IMHO ok, but maybe open a follow-up issue to pass this information to the plugins which need it?

+++ b/core/modules/views/lib/Drupal/views/ViewsBundle.phpundefined
@@ -32,6 +33,16 @@ public function build(ContainerBuilder $container) {
+    $container
+      ->register('cache.views_info', 'Drupal\Core\Cache\CacheBackendInterface')
+      ->setFactoryClass('Drupal\Core\Cache\CacheFactory')
+      ->setFactoryMethod('get')
+      ->addArgument('views_info');

We have a bunch of these now but note that this is not really correct, because cache() does it's own object caching, so cache('views_info') and $container->get('cache.views_info') won't return the same object, which can be a problem depending on how the backend is implemented.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -0,0 +1,220 @@
+/**
+ * Class to manage and lazy load cached views data.
+ */
+class ViewsDataCache {

I can see that this works differently* than the current design of CacheArray/CacheCollector, wondering if we could use the same interface as that, though. We'll see.

* I haven't followed the original issue which put this in closely, but I do wonder how a cache_set() for each table performs on a site with 400 fields (which means 800+ tables in the views data definition). Wondering if we could do something similar and only store keys for tables that are actually accessed. Different issue, though.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -0,0 +1,220 @@
+  /**
+   * The views cache bin to use.
+   *
+   * @var string
+   */
+  protected $bin;

It's actually a CacheBackendInterface now, so should be named $cacheBackend and it's also not views specific in anyway ("the *views* cache bin"). This is the only thing that needs to be fixed I think. Apart from the fact that it doesn't seem to apply anymore :)

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -0,0 +1,220 @@
+
+  /**
+   * Destructs the ViewDataCache object.
+   */
+  public function __destruct() {

__destruct() in the container is a bit problematic and can result in segfaults for frequently used stuff. It seems work in case of CacheArray or the path alias issue but did not work at all for lock/keyvalue.

Help me to get #512026: Move $form_state storage from cache to new state/key-value system in, then we can use the service terminator for this :)

I'll do some profiling once there is a patch that applies :)

#27: vdc-1867782-26.patch queued for re-testing.

2. There is an own issue: #1764474: Make Cache interface and backends use the DIC.

Performance looks nice! A whole bunch of function calls saved and replaced with much faster ones. See screenshots. Memory usage difference can be ignored I think, not even 0,1%.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -0,0 +1,220 @@
+  /**
+   * The cache bin to use.
+   *
+   * @var string
+   */

@var definition needs to be updated as well, I'd also say cache backend instead of cache bin.

Happy to RTBC once that is fixed.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -124,7 +124,7 @@ public function get($key = NULL) {
   /**
-   * Sets the data in the cache in for a cache key.
+   * Sets the data in the cache backend in for a cache key.

Hm, was the "in" supposed to be "bin"? Because in doesn't make much sense, neither in the old nor new comment...

Ok, I think this is good to go. Nice little performance improvement and code looks nicer as well.

Testbot needs to agree of course, but the last two patches where just documentation changes.

Awesome!

One small confusing change is this

+++ /dev/null
@@ -1,158 +0,0 @@
-    }
-    return $cache;
-  }
-  // Return an empty array if there is no match.
-  return array();
-}

vs

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.php
@@ -0,0 +1,220 @@
+      }
+    }
+
+    // Return an empty array if there is no match.
+    return $this->storage;
+  }

Is that ok?

Ok, thanks. #39 looks brilliant.

I always get a sad face when we remove useful wrapper functions in favour of drupal_container()->some('crap')->that('chains')->forever('and ever') but this is consistent with what we have done elsewhere.

Committed and pushed to 8.x. Thanks!

This will need a change notice, so moving to the Views queue to track that, per Jess.

Title change.

Yeah: http://drupal.org/node/1872588

I always get a sad face when we remove useful wrapper functions in favour of drupal_container()->some('crap')->that('chains')->forever('and ever') but this is consistent with what we have done elsewhere.

Me too. My main problem is that those are relatively hard to find (maybe we can fix that with some sort of list of what services core provides, e.g. as a new topic in the api documentation about that whole thing?) and you have absolutely no type hinting in IDE's.

Crell's answer to that is that all that stuff should be injected, but we can only inject when we have services/classes/factories where the calling code knows what will actually be required. While that helps and was my suggestion here as well (which has been slightly misunderstood I think, will comment in the follow-up issue), it is not going to happen in 8.x for most of the code.

Change notice looks ok to me.

It seems ViewsDataCache::get($key) will return the full array of $this->storage when $this->storage[$key] could not be found ?
It that the intended behavior ? Seems odd DX wise (you get a result in a different format when the key you requested doesn't exist / is invalid ?)

[discoverability of drupal_container()->some('crap')->that('chains')->forever('and ever')]
Crell's answer to that is that all that stuff should be injected, but we can only inject when we have services/classes/factories where the calling code knows what will actually be required. While that helps and was my suggestion here as well (which has been slightly misunderstood I think, will comment in the follow-up issue), it is not going to happen in 8.x for most of the code.

#1863816: Allow plugins to have services injected would help a bit with making more stuff injectable.