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;
}
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff-2497017-44.txt | 761 bytes | damiankloip |
#44 | 2497017-44.patch | 24.56 KB | damiankloip |
#43 | interdiff-2497017-43.txt | 668 bytes | damiankloip |
#38 | interdiff-2497017-36.txt | 5.14 KB | damiankloip |
#23 | Screen Shot 2015-06-05 at 1.51.33 PM.png | 165.42 KB | catch |
Comments
Comment #1
dawehner+1 to that issue
I think we should really be able to not even initialize the View executable objects
Comment #2
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented+1000 to the issue, this slows installation way down, too. (and some memory requirements)
Comment #3
dawehnerWe should then also see whether there is a usecase to bring that to D7
Comment #4
catchComment #5
damiankloip CreditAttribution: damiankloip commentedIn 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
Comment #6
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #7
dawehnerWhich 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.Comment #8
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #9
catchHere'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.
Comment #10
catchSince this is an API change, and it's more than 100ms saving on a cold cache, bumping to critical.
Comment #11
dawehnerLooks great in general. This allows us to require much less initialized view objects, given that not all views has a pager display ...
Let's quickly fix that doc bit.
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.
Comment #13
damiankloip CreditAttribution: damiankloip commentedFixing and addressing dawehner's feedback.
Comment #14
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #16
dawehnerinterdiff looks great!
Comment #17
jmolivas CreditAttribution: jmolivas at FFW commentedDisabling 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:
To assertNotEqual to "test_disabled_display":
This is the result of running the simple-test
Comment #18
damiankloip CreditAttribution: damiankloip commentedIsn'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.
Comment #19
dawehnerYeah changing tests is always wrong ...
Comment #20
damiankloip CreditAttribution: damiankloip commentedThis is the fix we want.
Interdiff based on #14.
Comment #21
dawehnerYeah, as written in IRC some days ago :) Let's get unit test coverage, it would have caught that ...
Comment #22
dawehnerAddded API change record and adapted the issue summary
Comment #23
catchUploading 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.
Comment #24
jmolivas CreditAttribution: jmolivas at FFW commentedBased 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
Comment #25
dawehnerDisabled 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.
Comment #26
damiankloip CreditAttribution: damiankloip commented#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.
Comment #27
damiankloip CreditAttribution: damiankloip commentedCatch, those numbers look good to me!
Comment #28
dawehnerAh 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!
Comment #29
jmolivas CreditAttribution: jmolivas at FFW commented@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?.
Comment #30
damiankloip CreditAttribution: damiankloip commentedYes, 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.
Comment #31
damiankloip CreditAttribution: damiankloip commentedSo, as before, patch in #20 is the one.
Comment #32
dawehnerYeah, your change there looks pretty neat. Saving code in the test is cool!
Please create one, thank you.
#20 is still the patch which lacks of a test :)
Comment #33
larowlanIs $name the same as $view_id?
Comment #34
dawehnerYeah
Comment #35
damiankloip CreditAttribution: damiankloip commentedGood spot. Might as well add some tests whilst I'm fixing that.
Comment #36
dawehnermeh! I think there is almost never a good idea for private. Here is certainly not one
Comment #37
chx CreditAttribution: chx commentedComment #38
damiankloip CreditAttribution: damiankloip commentedComment #41
dawehnerThe 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!
Comment #42
catchMinor, 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?
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.
Comment #43
damiankloip CreditAttribution: damiankloip commented1. 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.
Comment #44
damiankloip CreditAttribution: damiankloip commentedok, mis understood catch before, just wanted the order switched on that conditional too.
Comment #45
dawehnerAgreed, those versions are easier to read
Comment #46
catchCommitted/pushed to 8.0.x, thanks!
Comment #49
quietone CreditAttribution: quietone at PreviousNext commentedPublish the CR