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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

 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).

bigjim’s picture

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.

dawehner’s picture

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.

bigjim’s picture

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.

bigjim’s picture

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

tables
relationships
where
orderby
groupby
fields

bigjim’s picture

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

bigjim’s picture

Status: Active » Needs review

set to needs review

dawehner’s picture

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.

bigjim’s picture

good point I'll re-roll

Damien Tournoud’s picture

-          $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?

bigjim’s picture

@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.

bigjim’s picture

Status: Needs work » Needs review
FileSize
1.59 KB

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

tim.plunkett’s picture

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.

bigjim’s picture

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

bigjim’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs work » Needs review

@bigjim
Any updates on your failing tests?

bigjim’s picture

@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?

damiankloip’s picture

FileSize
1.47 KB

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

Sorry...

damiankloip’s picture

FileSize
668 bytes

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.

dawehner’s picture

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.

bigjim’s picture

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

dawehner’s picture

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

damiankloip’s picture

Status: Patch (to be ported) » Needs review
FileSize
754 bytes

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

dawehner’s picture

Status: Needs review » Fixed

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

Leeteq’s picture

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.

dawehner’s picture

Status: Fixed » Active

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

dawehner’s picture

Issue tags: +VDC

.

xjm’s picture

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

Is this still valid?

jhedstrom’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update

Needs an IS update if this is still valid.

dawehner’s picture

Well, 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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

larowlan’s picture

Issue tags: +Bug Smash Initiative

@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

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
quietone’s picture

There 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!

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Postponed (maintainer needs more info) » Closed (outdated)

Since we need more information to move forward with this issue and none has been supplied, I am closing this issue.