We have been profiling our code base, using xhprof, as part of a D7 upgrade and are finding that Views is not performing as well as it did in D6. I don't have all the stats together but one thing that is bubbling up is the calls to the cache_get method in the views_plugin_cache* class(es). That method call is rendering somewhere around 14X slower in D7 compared to D6 (please note were talking about the difference of around .007 seconds so this is not an issue unless you are calling views a lot on a page, which in our case we are on our home page :( ).

The major difference in cache_get is the get_results_key() and restore_headers() methods which rely on core APIs, specifically DBTNG and the Library APIs respectively, which appear to be the bottlenecks. While I don't think there is much we can do about those Core API's at this point (though I'm looking :) ). It does seem like there ought to be some way to get Views to perform faster in respect to cache_get calls. Namely by avoiding the calls to DBTNG's preExecute() method (and consequently __toString()) in get_results_key(). The call to restore_header() is obviously a little trickier as the Library API is actually brought into play by drupal_add_js and drupal_add_css functions which don't seem like they can be avoided, unless I'm mistaken.

Note we have memcahe and apc set up and the Views are caching as expected so we know this isn't a DB issue, this is all happening before the cache gets hit.

Files: 
CommentFileSizeAuthor
#26 1509980-26.patch754 bytesdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 1,555 pass(es).
[ View ]
#22 1509980-22.patch668 bytesdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 1,464 pass(es).
[ View ]
#21 1509980-21.patch1.48 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 1,464 pass(es).
[ View ]
#19 1509980-19.patch1.47 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 1,464 pass(es), 0 fail(s), and 16 exception(s).
[ View ]
#12 cache_get_optimization-1509980-12.patch1.59 KBbigjim
PASSED: [[SimpleTest]]: [MySQL] 1,430 pass(es).
[ View ]
#6 cache_get_optimization-1509980-6.patch1.52 KBbigjim
Test request sent.
[ View ]

Comments

Namely by avoiding the calls to DBTNG's preExecute() method (and consequently __toString()) in get_results_key().

I fear there is no way to fix this generic, as it's required to have a cache key which is unique to the query.
For example if you have node access this is the only way to get them into the game. This have some bugs currently but that's out of scope.

Maybe some contrib module could implement a fast cache plugin which for example ignores basically what get_results_key does currently and just implement what's important (this could be even configurable).

Yea the SQL re-write thing really is a deal killer, for the most part we have been able to avoid using node_access to this point, but I se your point.

I was wondering what the chances are of passing meta data for the various handler classes (arguments (contextual filters), fields, filters, sorts and relationships) into $view->build_info and using that to get the md5() hash to avoid the expensive calls to __toString() and preExecute, assuming you are bypassing node_access.

I guess in some way I feel like those who aren't using node_access (a large segment of the target audience for Views) shouldn't have to take a performance hit.

I guess in some way I feel like those who aren't using node_access (a large segment of the target audience for Views) shouldn't have to take a performance hit.

Didn't you told that it's "just" .007 seconds, sure that's quite a bit, but you still win a lot of time compared to the executed sql and the rendered view ...
Personally i see views as a base framework which can be adapted by custom code. If for example this performance "optimized" behavior is the default, it would be a big security problem here if it's misconfigured.

I was wondering what the chances are of passing meta data for the various handler classes (arguments (contextual filters), fields, filters, sorts and relationships) into $view->build_info and using that to get the md5() hash to avoid the expensive calls to __toString() and preExecute, assuming you are bypassing node_access.

There is no problem to access all the data, even it's not part of $view->build_info at the moment. The sql string is somehow unique enough. You could alternative look at $view->query->where etc. to find all the things you need.

Didn't you told that it's "just" .007 seconds, sure that's quite a bit, but you still win a lot of time compared to the executed sql and the rendered view ...

Yes though the more important point here is Views is causing a 50% increase in our home page load time, 1/2 of which is this issue.

If for example this performance "optimized" behavior is the default, it would be a big security problem here if it's misconfigured.

I would propose this performance optimized behavior only be activated when node_access is bypassed in the View settings.

I'm gonna try and put a patch together using the following elements of $view->query

tables
relationships
where
orderby
groupby
fields

StatusFileSize
new1.52 KB
Test request sent.
[ View ]

While it's no magic bullet this patch took 5% out of the processing time for a view that had disable sql rewrite = true.

passed testing as far as setting cache keys appropriately

Status:Active» Needs review

set to needs review

Status:Needs review» Needs work

+++ b/plugins/views_plugin_cache.incundefined
@@ -260,17 +260,22 @@ class views_plugin_cache extends views_plugin {
+        $build_info = $this->view->query->where;

To support groupby queries you might need having as well.

good point I'll re-roll

<?php
-          $query = clone $build_info[$index];
-         
$query->preExecute();
-         
$build_info[$index] = (string)$query;
?>

Cloning and calling ->preExecute() is just completely unnecessary now (since we fixed some query building issues in DBTNG last year). Just use (string) $query and it will work just as good. Wouldn't that fix the performance issue you are seeing?

@Damien Tournoud It would certainly be a good step but even casting the $query object is not the most efficient process. Though I believe the reason for the preExecute is to get the SQL string after hook_node_access etc... has been run, no? md5() an array alone is significantly more efficient.

Status:Needs work» Needs review
StatusFileSize
new1.59 KB
PASSED: [[SimpleTest]]: [MySQL] 1,430 pass(es).
[ View ]

patch from #6 re-rolled , this time to include having.

Version:7.x-3.3» 7.x-3.x-dev

Moving for the testbot.

Status:Needs review» Needs work

The last submitted patch, cache_get_optimization-1509980-12.patch, failed testing.

Status:Needs review» Needs work

Marked for re-rest. Locally the same 4 test fail that fail with or w/o this patch. All 4 for views_handler_sort_date.test line 192 ViewsHandlerSortDateTest->testDateOrdering().

The messages are:
- Result is returned correctly when ordering by granularity day, forward.
- Result is returned correctly when ordering by granularity day, reverse.
- Result is returned correctly when ordering by granularity month, reverse.
- Result is returned correctly when ordering by granularity year, reverse.

I'm gonna check the tests

Status:Needs work» Needs review

Status:Needs work» Needs review

@bigjim
Any updates on your failing tests?

@dawehner according to #12 is passed on re-submit for testing. Also, in #15 locally it failed those 4 test with our without this patch. I'm not a testing guru but I believe those 4 test were intended to fail?

StatusFileSize
new1.47 KB
FAILED: [[SimpleTest]]: [MySQL] 1,464 pass(es), 0 fail(s), and 16 exception(s).
[ View ]

I have added the advice of Damien Tournoud in #10; we don't need to clone and preExecute() anymore. This brings down the execution time noticeably.

We can also just use !empty() instead of isset() and checking if 'disable_sql_rewrite' is true?

Status:Needs review» Needs work

The last submitted patch, 1509980-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 1,464 pass(es).
[ View ]

Sorry...

StatusFileSize
new668 bytes
PASSED: [[SimpleTest]]: [MySQL] 1,464 pass(es).
[ View ]

Or, keep it much simpler for now, as I think we may have more things to worry about than the current patch suggests. Just improving query casting part will make a good improvement.

Issue tags:+Needs tests

As this is quite an important place i would personally vote for a test as well as manual testing that it solves the problem for the people.

+1 for #22, I like simpler solutions :)

Version:7.x-3.x-dev» 8.x-3.x-dev
Status:Needs review» Patch (to be ported)

Let's get it in. The technique in #12 seems to be a bit fuzzy, so i suggested to require tests

Committed to 7.x-3.x

Status:Patch (to be ported)» Needs review
StatusFileSize
new754 bytes
PASSED: [[SimpleTest]]: [MySQL] 1,555 pass(es).
[ View ]

Here is an 8.x version. If we want to have 8.x tests for this first, let me know.

Status:Needs review» Fixed

No, that's fine. Committed to 8.x-3.x

If this was reverted with 3.5, should this issue stay open?
Ref. #1748526: 7.x-3.5 upgrade affecting some Views with time-based cache enabled, comment #2.

Status:Fixed» Active

I guess so. This time needs tests is really a requirement.

Issue tags:+VDC

.

Title:cache_get performance significantly worse in D7 (D6 -> D7 regression)cache_get() performance regression
Project:Views» Drupal core
Version:8.x-3.x-dev» 8.x-dev
Component:Miscellaneous» views.module

Is this still valid?