Problem/Motivation

Steps to reproduce:

1. Load admin/modules
2. drush cr
3. Enable a module
4. Look at profiling data for the form POST and the next page render - see two screenshots attached.

Views::getApplicableViews() runs for route rebuilding, menu link plugins, and menu local task plugins. All of these are global registries/caches that completely block normal page rendering for any other request (i.e. not a single themed request can be served until all items have finished building).

With the standard profile they are not great for performance, and they get significantly worse once you have more views enabled on a site.

Note this is currently worse in HEAD due to #2495073: Views feed display plugin has to get all views data on init - but it makes sense to fix both issues independently of each other, however profiling will see more modest improvements depending on which gets fixed first.

Proposed resolution

Rather than initializing displays, check the config directly instead. This also avoids initializing the plugins which is itself expensive.

Remaining tasks

Get that working and profile before/after to see if it sufficiently fixes the issue.

User interface changes

None.

API changes

\Drupal\views\Views::getApplicableViews no longer returns view executables.

Before:

       $views = Views::getApplicableViews();
       foreach ($views as $data) {
         list($view, $display_id) = $data;
         $id = $view->storage->id();
         $this->viewsDisplayPairs[] = $id . '.' . $display_id;
      }

After:

       $views = Views::getApplicableViews();
       foreach ($views as $data) {
        list($view_id, $display_id) = $data;
        $this->viewsDisplayPairs[] = $view_id . '.' . $display_id;
     }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

+1 to that issue

I think we should really be able to not even initialize the View executable objects

Fabianx’s picture

+1000 to the issue, this slows installation way down, too. (and some memory requirements)

dawehner’s picture

We should then also see whether there is a usecase to bring that to D7

catch’s picture

damiankloip’s picture

In it's current form, this is almost impossible to do I would say, as the calling code, local tasks and local actions are depending on that executable anyway. So if we did change it here, it would take more work again for those places to create instances. Looking at things like #2301317: MenuLinkNG part4: Conversion

damiankloip’s picture

I think most of these places we could replace with config only logic, as they really just want settings. The only problem is that with a display that has been init()ed you get merged default values. Which is impossible otherwise currently.

dawehner’s picture

The only problem is that with a display that has been init()ed you get merged default values. Which is impossible otherwise currently.

Which bit of the configuration matters here? I mean the only check is pretty much if (!empty($handler->definition[$type]) && $handler->isEnabled()) { if I understand it correctly.

damiankloip’s picture

Status: Active » Needs review
FileSize
9.54 KB

Here is a relatively quick first attempt. Using the display configuration to determine the enabled state. Which is not too bad, as default display is never disabled, so we don't really need to fall back.

This means implementations using this (only a handful) have to create the instances themselves, which RouteSubscriber was already doing anyway. It's just the (grrrrrr) Local tasks and menu links.

Didn't amend any tests yet.

catch’s picture

Here's an xhprof screenshot with the patch applied, compare it to: https://www.drupal.org/files/issues/Screen%20Shot%202015-05-29%20at%2011...

Looks like this saves approximately 450ms on my machine. Will be more with more views.

Note this is valuable without needing to fully fix local tasks/actions because for example the module page form submit (and the installer) does not need to build local tasks and actions - only when we render a full page do we need to do that.

catch’s picture

Priority: Major » Critical

Since this is an API change, and it's more than 100ms saving on a cold cache, bumping to critical.

dawehner’s picture

Looks great in general. This allows us to require much less initialized view objects, given that not all views has a pager display ...

  1. +++ b/core/modules/views/src/Plugin/Derivative/ViewsMenuLink.php
    @@ -8,14 +8,47 @@
    +   * @param \Drupal\Core\Routing\RouteProviderInterface $route_provider
    +   *   The route provider.
    +   * @param \Drupal\Core\State\StateInterface $state
    +   *   The state key value store.
    +   * @param \Drupal\Core\Entity\EntityStorageInterface $view_storage
    +   *   The view storage.
    +   */
    +  public function __construct(EntityStorageInterface $view_storage) {
    

    Let's quickly fix that doc bit.

  2. +++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
    --- a/core/modules/views/src/Views.php
    +++ b/core/modules/views/src/Views.php
    
    +++ b/core/modules/views/src/Views.php
    +++ b/core/modules/views/src/Views.php
    @@ -212,26 +212,29 @@ public static function getEnabledDisplayExtenders() {
    
    @@ -212,26 +212,29 @@ public static function getEnabledDisplayExtenders() {
       public static function getApplicableViews($type) {
    

    We certainly need to update the documentation of that method. It's also a bit odd that we don't have direct test coverage for that method, but well, this is how it is.

Status: Needs review » Needs work

The last submitted patch, 8: 2497017.patch, failed testing.

damiankloip’s picture

Assigned: Unassigned » damiankloip

Fixing and addressing dawehner's feedback.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
20.41 KB
12.16 KB

Here are the comment fixes, and fixes for the two failing unit tests. Didn't look at DisabledDisplayTest yet but does imply that getApplicableViews is not dealing with disabled displays properly.

Moved some stuff around in ViewsTest and a @todo with a new method. Should be just the one failing test class now.

Status: Needs review » Needs work

The last submitted patch, 14: 2497017-14.patch, failed testing.

dawehner’s picture

interdiff looks great!

jmolivas’s picture

Status: Needs work » Needs review
FileSize
21.95 KB
1.31 KB

Disabling a ViewDisplay do not return a 404 status code, it returns a 200 status code.

I did update the test replacing, asserting to 400 status code:

$this->assertResponse(404);

To assertNotEqual to "test_disabled_display":

$result = $this->xpath('//h1');
$this->assertNotEqual($result[0], 'test_disabled_display', 'The enabled page_2 display is disabled.');

This is the result of running the simple-test

Drupal test run
---------------

Tests to be run:
  - Drupal\views\Tests\Plugin\DisabledDisplayTest::testDisabledDisplays

Test run started:
  Friday, June 5, 2015 - 10:20

Test summary
------------

Drupal\views\Tests\Plugin\DisabledDisplayTest::testDisabledD  35 passes

Test run duration: 1 min 10 sec
damiankloip’s picture

Isn't that the problem? That a 200 is being returned. Changing the tests for that is just wrong :)

This patch should not affect the current tests for the response code returned for a disabled display. You do not want people being able to access disabled displays.

dawehner’s picture

Yeah changing tests is always wrong ...

damiankloip’s picture

FileSize
20.42 KB
750 bytes

This is the fix we want.

Interdiff based on #14.

dawehner’s picture

Yeah, as written in IRC some days ago :) Let's get unit test coverage, it would have caught that ...

dawehner’s picture

Issue summary: View changes

Addded API change record and adapted the issue summary

catch’s picture

Uploading four screenshots - two before, two after.

This is xhprof of the POST form submit, then subsequent render, of admin/modules when enabling actions module. I ran drush cr before submitting the form.

Before


After

As you can see especially for route building, this takes us down from 350ms (with standard profile - much more with lots of views on a site) to 50ms, and xhprof reckons a 50% reduction in memory usage too - not reliable but right direction at least.

jmolivas’s picture

Based on #18 @damiankloip and #19 @dawehner

What must be changed is response returning the 200 and return a 404 status code.

Did you know if there is an issue for making Disabled views return a 404?, to change that and rollback the test

dawehner’s picture

Did you know if there is an issue for making Disabled views return a 404?, to change that and rollback the test

Disabled views certainly produce a 404.
Go to /glossary and you will get a 404. Also disabling a display works pretty good!

Note: damian reverted your changes, so the test actually ensures we have a 404 going on.

damiankloip’s picture

#24, to clarify what I was talking about earlier. The responses should NOT be returning 200, and they currently do not. They return 404s. The fix I added in #20 (based off of #14) means that disabled displays do not get a route registered. Which is the correct behaviour. We don't need any other issues for this.

damiankloip’s picture

Catch, those numbers look good to me!

dawehner’s picture

Ah I see, we even safe a lot of inclusive time for the callers, I think mainly because we filter out instances we don't need, which is a huge time saver in further processing,
especially the routing, yeah!

jmolivas’s picture

FileSize
23.81 KB
3.16 KB

Did you know if there is an issue for making Disabled views return a 404?, to change that and rollback the test

@dawehner I as trying to reference to ViewDisplay not Views.

I did a small refactor to the DisabledDisplayTest to take advantage and reuse the code to enable/disable displays.

Should a patch like this belong to the same issue? or a new issue should be created?.

damiankloip’s picture

Yes, a new issue please. This is out of scope here, and distracting from the actual issue. We don't need to touch that file/test here.

damiankloip’s picture

So, as before, patch in #20 is the one.

dawehner’s picture

Yes, a new issue please. This is out of scope here, and distracting from the actual issue. We don't need to touch that file/test here.

Yeah, your change there looks pretty neat. Saving code in the test is cool!

Should a patch like this belong to the same issue? or a new issue should be created?.

Please create one, thank you.

#20 is still the patch which lacks of a test :)

+  public function testGetApplicableViews() {
+    // @todo
   }
 
larowlan’s picture

+++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
@@ -107,12 +107,15 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+        $name = $view->get('id');

Is $name the same as $view_id?

dawehner’s picture

Is $name the same as $view_id?

Yeah

damiankloip’s picture

Good spot. Might as well add some tests whilst I'm fixing that.

dawehner’s picture

+++ b/core/modules/views/src/Tests/Plugin/DisabledDisplayTest.php
@@ -99,4 +84,46 @@ public function testDisabledDisplays() {
+  private function toggleDisplays($view, $type){

meh! I think there is almost never a good idea for private. Here is certainly not one

chx’s picture

damiankloip’s picture

FileSize
24.56 KB
5.14 KB

Status: Needs review » Needs work

The last submitted patch, 38: 2497017-36.patch, failed testing.

damiankloip queued 38: 2497017-36.patch for re-testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

The recent patch does not include the out of scope changes of #29. Beside the private part, they are looking fine,
but are simply not required to solve that issue.

Thank you @damiankloip to add test coverage!

catch’s picture

  1. +++ b/core/modules/views/src/Views.php
    @@ -194,44 +194,47 @@ public static function getEnabledDisplayExtenders() {
    +        $enabled = !array_key_exists('enabled', $display['display_options']) || !empty($display['display_options']['enabled']);
    

    Minor, but 1. should we do the !empty check first. 2. If not, since we know the key exists, then could just be a boolean check rather than !empty?

  2. +++ b/core/modules/views/src/Views.php
    @@ -194,44 +194,47 @@ public static function getEnabledDisplayExtenders() {
    +        if (in_array($display['display_plugin'], $plugin_ids) && $enabled) {
    

    Would also reverse the order here.

    These are micro-optimizations but I also find the simpler check first easier to scan. Or we could skip setting the variable and have a longer single conditional, not sure.

Nothing else to add though, patch looks great and this has been a site-hosing problem since Drupal 6 that's still not resolved in the contrib module: #853864: views_get_default_view() - race conditions and memory usage so great to see it comprehensively fixed here.

damiankloip’s picture

FileSize
24.56 KB
668 bytes

1. That is specifically in that order, as the logic goes something like "If there is no enabled key, assume TRUE. Otherwise, if there is a key, use that". empty() would not cover the non existing key case.
2. I was thinking about that before, and agree. So, done.

Having a D7 patch for this would be good too. I will open a separate issue in the views queue.

damiankloip’s picture

FileSize
24.56 KB
761 bytes

ok, mis understood catch before, just wanted the order switched on that conditional too.

dawehner’s picture

Agreed, those versions are easier to read

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 8214143 on 8.0.x
    Issue #2497017 by damiankloip, jmolivas: Views::getApplicableViews()...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

Publish the CR