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.
Comment | File | Size | Author |
---|---|---|---|
#26 | 1509980-26.patch | 754 bytes | damiankloip |
#22 | 1509980-22.patch | 668 bytes | damiankloip |
#21 | 1509980-21.patch | 1.48 KB | damiankloip |
#19 | 1509980-19.patch | 1.47 KB | damiankloip |
#12 | cache_get_optimization-1509980-12.patch | 1.59 KB | bigjim |
Comments
Comment #1
dawehnerI 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).
Comment #2
bigjim CreditAttribution: bigjim commentedYea 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.
Comment #3
dawehnerDidn'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.
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.
Comment #4
bigjim CreditAttribution: bigjim commentedDidn'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.
Comment #5
bigjim CreditAttribution: bigjim commentedI'm gonna try and put a patch together using the following elements of $view->query
tables
relationships
where
orderby
groupby
fields
Comment #6
bigjim CreditAttribution: bigjim commentedWhile 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
Comment #7
bigjim CreditAttribution: bigjim commentedset to needs review
Comment #8
dawehnerTo support groupby queries you might need having as well.
Comment #9
bigjim CreditAttribution: bigjim commentedgood point I'll re-roll
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedCloning 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?Comment #11
bigjim CreditAttribution: bigjim commented@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.
Comment #12
bigjim CreditAttribution: bigjim commentedpatch from #6 re-rolled , this time to include having.
Comment #13
tim.plunkettMoving for the testbot.
Comment #15
bigjim CreditAttribution: bigjim commentedMarked 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
Comment #16
bigjim CreditAttribution: bigjim commented#12: cache_get_optimization-1509980-12.patch queued for re-testing.
Comment #17
dawehner@bigjim
Any updates on your failing tests?
Comment #18
bigjim CreditAttribution: bigjim commented@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?
Comment #19
damiankloip CreditAttribution: damiankloip commentedI 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?
Comment #21
damiankloip CreditAttribution: damiankloip commentedSorry...
Comment #22
damiankloip CreditAttribution: damiankloip commentedOr, 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.
Comment #23
dawehnerAs 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.
Comment #24
bigjim CreditAttribution: bigjim commented+1 for #22, I like simpler solutions :)
Comment #25
dawehnerLet'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
Comment #26
damiankloip CreditAttribution: damiankloip commentedHere is an 8.x version. If we want to have 8.x tests for this first, let me know.
Comment #27
dawehnerNo, that's fine. Committed to 8.x-3.x
Comment #28
Leeteq CreditAttribution: Leeteq commentedIf 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.
Comment #29
dawehnerI guess so. This time needs tests is really a requirement.
Comment #30
dawehner.
Comment #31
xjmComment #32
jibranIs this still valid?
Comment #33
jhedstromNeeds an IS update if this is still valid.
Comment #34
dawehnerWell, the same type of code still exist, but on the other hand its a question whether its still neeeded for most cases, given that they will rely on render caching to get a proper speed up by default, which will never touch that particular piece of code.
Comment #43
larowlan@bigjim @dawehner do you think this is safe to be closed (won't fix) in that case - i.e. render caching has since been added which makes this now redundant?
Triaged as part of Bug Smash Initiative
Comment #46
quietone CreditAttribution: quietone at PreviousNext commentedThere has been no discussion of the problem in 7 years.
In #43, 2 years ago, larowlan pointed out that render caching has been added and asked for clarification.
Since we need more information to move forward with this issue, I am keeping the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #48
quietone CreditAttribution: quietone at PreviousNext commentedSince we need more information to move forward with this issue and none has been supplied, I am closing this issue.