Lazy load display plugins

+1

It doesn't make sense to me that initializeDisplay has a return value.

public function offsetGet($offset) {
  if (!isset($this->display[$offset])) {
    $this->initializeDisplay($offset);
  }
  return $this->display[$offset];
}

Then initializeDisplay() could also be simplified.

This is what I had in mind.

YESSSSS!

+++ b/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+  /**
+   * Stores the actual display instances in an array.
+   *
+   * @var array
+   */
+  protected $display = array();

Would it be better if this was $displays or $display_storage or something instead?

+++ b/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+   * Initializes a single display and stores the result in self::display.

Does this make you think of static properties?

+++ b/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+    $this->display[$display_id] = views_get_plugin('display', $this->view->storage->display[$display_id]['display_plugin']);
+    $this->display[$display_id]->init($this->view, $this->view->storage->display[$display_id]);

Would be nice if there was a nicer way to get this stuff. Not really a part of this issue though :)

+++ b/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+  /**
+   * Implements \Iterator::current().
+   */
+  public function next() {

next()

This looks great!

Looks great, I guess this will pass but you never know... :)

+++ b/lib/Drupal/views/DisplayArray.phpundefined
@@ -42,31 +42,31 @@
+    foreach ($this->$displayHandlers as $display_id => $display) {

$this->$displayHandlers has an extra $

Just to be clear; I only removed a '$' character.

+++ b/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+    return current($this->display);
...
+    return next($this->display);
...
+    return key($this->display);
...
+    $display_id = key($this->display);
...
+    reset($this->display);

These are all still the old variable

+++ b/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+  public function valid() {
+    $display_id = key($this->display);
+    return isset($this->displayHandlers[$display_id]);

This looks weird. Wouldn't you just need to do return key($this->displayHandlers) !== NULL;

A few other instances of $this->display weren't changed.

OK one question though:

+  /**
+   * Implements \Iterator::current().
+   */
+  public function next() {
+    // If we do an actual iteration over the elements, let's initialize all
+    // displays.
+    $this->initializeAllDisplays();
+    return next($this->display);
+  }

When views is figuring out the display for a view, it iterates over the displays until one passes an access check, then returns from within the foreach() - that seems like one of the main places that'd benefit from his but initializing all displays won't help with it.

We talked about this briefly on IRC; It's a good point. One idea is to store a list of the displays on the class, maybe in the conststructor so we don't have to get them from $this->view->storage->display each time. Not sure if that's a good trade off or not?

Something like :

// ...

  /**
   * Stores an array of display ID's
   *
   * @var array
   */
  protected $displayIds = array();

// ...

  public function __construct(ViewExecutable $view) {
    $this->view = $view;

    $this->displayIds = array_keys($this->view->storage->display);

    $this->initializeDisplay('default');
  }

// ...

Just an idea.

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,160 @@
+    return $this->offsetGet(    $display_id = next($this->displayIDs);

This is broken copy/paste?

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,160 @@
+    return $display_id !== FALSE;

return ($display_id !== NULL && $display_id !== FALSE);

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+    return $display_id !== NULL && $display_id !== FALSE;

Wont !empty() give you the same result? empty will evaluate TRUE for both NULL and FALSE.

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,156 @@
+    return isset($this->displayHandlers[$offset]) || isset($this->view->storage->display[$offset]);

return isset($this->displayHandlers[$offset]) || in_array($offset, $this->displayIDs); Instead?

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,156 @@
+    return count($this->view->storage->display);

count($this->displayIDs) ?

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -130,7 +130,8 @@
-    return key($this->displayIDs);
+    $key = key($this->displayIDs);
+    return $key;

Change this back, and this is RTBC.

This looks pretty awesome now.

#44: drpal-1817582-43.patch queued for re-testing.

Rerolled after #1763974: Convert entity type info into plugins

It'd be good to profile this before it goes in on a view with say 5 or 10 displays, to see how much difference it actually makes.

@catch We are actually in the process of doing that already. :)

Hi

I did benchmark this a lot of times with different settings. The point is there is no real difference. The lazy loading script might be a bit slower. For the test I created a view with > 20 displays that use 14 fields. Than I profiled the call of views_get_view_result(). dawehner told me that we should get a real view that is known for having performance problem. Otherwise I can't show any performance problem or gain.

Removing from RTBC until we improve it.

Just fixed the fatal from DisplayPluginBase.

I think this is perilously close now ...

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+    // If the display was initialize before just return.

Maybe 'If the display was initialized before, just return.'?

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+    $display = &$this->view->storage->getDisplay($display_id);

It's always slightly annoying that we have to declare by reference on the method itself and the call to it, but hey, that's php :)

+++ b/core/modules/views/lib/Drupal/views/DisplayArray.phpundefined
@@ -0,0 +1,159 @@
+    $this->displayHandlers[$display_id] = views_get_plugin('display', $display['display_plugin']);
+    $this->displayHandlers[$display_id]->init($this->view, $display);

Is there any sense in checking we have a display before continuing here? Not sure.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -2204,7 +2185,7 @@ public function setItemOption($display_id, $type, $id, $option, $value) {
    * @return Drupal\views\Plugin\views\display\DisplayPluginBase
    *   A reference to the new handler object.
    */
-  public function &newDisplay($id) {
+  public function newDisplay($id) {

The doc still says we are returning a reference. Hang on, why do we want to change this again? :)

The attached display change looks good too, I think that is the only way we can do this without actually loading displays.

#71: drupal-1817582-71.patch queued for re-testing.

Had a quick glance over the patch... I think the file containing the DisplayArray class is not included. Testbot will probably confirm, unless I totally missed something :-)

This looks good now, the attach code and tests look fine too.

Looks like there hasn't been profiling since #51, could we do that again to see the impact on a view with a few displays?

That's a good idea.

Also, rerolled.

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayTest.phpundefined
+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayTest.phpundefined
@@ -19,7 +19,7 @@ class DisplayTest extends PluginTestBase {

@@ -19,7 +19,7 @@ class DisplayTest extends PluginTestBase {
    *
    * @var array
    */
-  public static $testViews = array('test_filter_groups');
+  public static $testViews = array('test_filter_groups', 'test_get_attach_displays');
 
   /**
    * Modules to enable.

Yes, this did it mostly.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1489,13 +1475,11 @@ public function attachDisplays() {
     $this->is_attachment = TRUE;
-    // Give other displays an opportunity to attach to the view.
-    foreach ($this->displayHandlers as $id => $display) {
-      if (!empty($this->displayHandlers[$id])) {
-        // Create a clone for the attachments to manipulate. 'static' refers to the current class name.
-        $cloned_view = new static($this->storage);
-        $this->displayHandlers[$id]->attachTo($cloned_view, $this->current_display);
-      }
+    // Find out which other displays attach to the current one.
+    foreach ($this->display_handler->getAttachedDisplays() as $id) {
+      // Create a clone for the attachments to manipulate. 'static' refers to the current class name.
+      $cloned_view = new static($this->storage);
+      $this->displayHandlers[$id]->attachTo($cloned_view, $this->current_display);
     }
     $this->is_attachment = FALSE;

And this

It is hard to create a view that really shows any difference to get some numbers. I created a really big view with many displays. And if you load it uncached you can find this:

With the patch added an uncached load shows only this:

I'd say it works and only loads the one needed display.

In the .gz you can find the view_from_hell.yml and the xhprof results from the runs.

+1 for the patch.

OK that's great. Committed/pushed to 8.x.

Wondering if this could ever be backported to 7.x-3.x, but marking 'fixed' for now.

I'm happy to test this with my 10-display view.

Back to original status after triggering testbot.

NW would be the correct status now that there's a start at a patch. :)

Okay, different rules from core then. ;)

Hi,

I'm not 100% sure if this fatal error I have it's related with this issue, but I find this issue cause the qa drupal test. In the test it have the same fatal error.

Fatal error: Call to a member function ensure_my_table() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/views/handlers/views_handler_filter_combine.inc on line 60

I get this fatal error when I have an exposed filter with combined fields. I'm trying to figure out what cause this error, but it's hard to find.

I tried to implement and test this patch on D7. I keep getting "reference to non-object" errors.

In the end, I have the following additions to patch #88

   function destroy() {
     foreach (array_keys($this->display) as $display_id) {
-      if (isset($this->display[$display_id]->handler)) {
+      if ($this->display[$display_id]->handler_inited) {
         $this->display[$display_id]->handler->destroy();
         unset($this->display[$display_id]->handler);
+        $this->display[$display_id]->handler_inited = FALSE;
       }
     }

The variable handle_inited must be reset, too.

+  /**
+   * Overrides the magic __get function to lazy initialize the display handler.
+   */
+  function &__get($name) {
+
+    // Lazy initialize the handler.
+    if ($name == 'handler') {
+//      if ($this->handler_inited) {
+      if (!$this->handler_inited) {
+        $this->handler = views_get_plugin('display', $this->display_plugin);
+        $this->handler_inited = TRUE;

The check must be inversed.

+  function __isset($name) {
+    if ($name == 'handler') {
+      if (!$this->handler_inited) {
+       return FALSE;
+       $foo = 123;
+      }
+      return TRUE;
+    }
+    else {
+      return TRUE;
+    }
+  }

Added the 'return FALSE' line.

Also, function fix_missing_relationships() seems to load all handlers and objects. It is already called when building the menu, not when building the page itself.

88: views-1817582-88.patch queued for re-testing.

Is this abandoned? Or just dormant?