Cache filters per page load for massive performance boost
mr.j - April 8, 2009 - 03:14
| Project: | Custom filter |
| Version: | 5.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Currently we have about 250 custom filters and we never use subfilters.
Attempts by the module to select subfilters individually for every single filter results in a significant performance degradation.
On our homepage Devel module reports:
Executed 3846 queries in 2815.98 milliseconds.
Now comment out this line (#95):
// $filter['sub'] = customfilter_get_filters($sid, $filter['fid'], $sortby, $cols);
Executed 1012 queries in 1579.4 milliseconds.
The difference of 2800+ queries suggests that subfilter queries are being executed repeatedly - perhaps for separate blocks or views on the page? Maybe the filter query results could be cached in session per page load (or something similar) to avoid being selected so many times.
#1
Not sure that I made it clear that I would like a setting which allows me to disable the use of subfilters altogether.
#2
Here's a patch that fetches the filters and filter sets once per page load (using statics). Makes a big difference. Won't have an effect on php4, but in php5, wow. What a difference.
#3
Thanks, but the patch has a bug. customfilter_get_filters() is recursive so after this patch it will never select subfilters, even for people who want them (because the static array is already set).
I fixed it by changing the if condition on line #90 to this, so that it always queries for subfilters the first time the recursive call is made:
if (!isset($filters[$sid]) || $root > 0) {Re-rolled patch attached to this post.
Testing the various scenarios:
Default code:
Executed 3852 queries in 3847.09 milliseconds.
Memory used at devel_init(): 16.94 MB
Memory used at devel_shutdown(): 24.99 MB
Subfilters disabled (as per original post):
Executed 1018 queries in 1437.31 milliseconds.
Memory used at devel_init(): 16.94 MB
Memory used at devel_shutdown(): 23.1 MB
acstewart patch + my fix (patch attached to this post):
Executed 1159 queries in 1427.18 milliseconds.
Memory used at devel_init(): 16.94 MB
Memory used at devel_shutdown(): 24.4 MB
acstewart patch + my fix + subfilters disabled:
Executed 941 queries in 1344.13 milliseconds.
Memory used at devel_init(): 16.94 MB
Memory used at devel_shutdown(): 23.3 MB
So it looks like a combination of the two approaches yields the best result in terms of reducing the number of queries and almost as little memory used as just disabling the subfilters.
#4
Excellent.. Will take a look at the patch and apply it. Stay tuned.
#5
mr.j, can you export your filterset to an XML file and attach it to an issue here?
I have another patch that assembles the filter sets and caches the resulting array so that devs can take advantage of drupal's caching layer (for things like memcache), but I'd like to be certain that it behaves nicely with your implementation.
-=A
#6
I am changing the status of this report, which has not gotten a feedback in the last three months.
#7
Apologies I never saw the request for more information.
I have been using the patch in #3 for months now and there are no problems with it.
It is pretty crazy to simply "won't fix" this one given the numbers I posted earlier. Most people would be interested in potentially saving thousands of database calls and several seconds of waiting per page load.
I assume it is due to you being a new maintainer of this module and not wanting to maintain the 5.x branch, but you would be silly to ignore it for 6.x. Put it this way - if/when we upgrade to Drupal 6.x I will not be using this module without the attached fix at #3.
I have only set this back to "needs review" so that you will hopefully see and read this comment as the default issues list filters it out otherwise. Regards
#8
I am changing back the status as set by acstewart.
About implementing the same code for Drupal 6 version, I would like to see any comparison between sub-replacement rules disabled, and sub-replacement rules enable for the Drupal 6 version of the module.
#9
I am not running D6, but here is a patch that uses acstewart's static arrays code against the latest 5.x-1.4.
It does not disable subfilters.
#10
- // Prepare columns to select- if (! is_array($cols)) $cols = array($cols);
- $columns = join(', ', $cols);
I am not sure why that part of the code is being removed, as it could break other code, and it doesn't influence the performance of the code.
I would like to see any explanation of what the code is doing too.
#11
You didn't look very closely, because it is added back in a few lines down.
Read #2 and #3 for a full explanation - it selects the filters using a static array so that the same filters are not selected multiple times on a page load. Look at #3 for stats on how big a difference this makes to performance.
#12
I mean in the code, not in this report; although, comment #1 says few about the code.
#13
Although I am not developing the version for Drupal 5, I could commit the changes if (and only if) a proper patch is going to be submitted. This means the code as I reported in comment #10 is not removed by the patch, and a setting is added in the user interface.
#14
Rather than making the sub-filters optional, it would be better to cache the filters definition.
#15
First forget about making sub-filters optional for now, the patch has nothing to do with that (as I said in #9). You are obviously confused, so I have changed the issue title accordingly.
Now please re-read the first line in #11 where I tried to explain that the code you are referring to is not removed, it is just moved a few lines down.
TADA! That is exactly what this patch does.
There is no need for a setting on the user interface. The patch operates transparently and reduces database usage significantly. Here you go - its 2.4 seconds faster per page load and uses 2700 less database queries in the example I checked:
Unpatched module:
Executed 3852 queries in 3847.09 milliseconds.
Memory used at devel_init(): 16.94 MB
Memory used at devel_shutdown(): 24.99 MB
Patch in #9:
Executed 1159 queries in 1427.18 milliseconds. <== FASTER
Memory used at devel_init(): 16.94 MB <<== SAME
Memory used at devel_shutdown(): 24.4 MB <<== LESS
KiamLaLuno commit it or don't, I give up trying to explain it any more. If you can't understand the code or don't want to commit it then please just leave this issue open so that others who will inevitably run into the same performance problems can see it. Thanks.
For anyone else using this patch, I will continue to patch it for 5.x releases of this module and will post it here in future.
[Edited by KiamLaLuno to remove the sentence all in capital case, and using bold font]
#16
I hope that, as maintainer, I can ask if I don't understand anything.
Yes, I got confused on what you were trying to do, also because you opened the report saying
That is also the reason why I was talking of a setting about the subfilters.
#17
The code has been changed, and committed in CVS for the development snapshot; in the next days, I will release a new official release.
@mr.j: If you want to become a co-maintainer for the Drupal 5 branch, you are welcome.
#18
Automatically closed -- issue fixed for 2 weeks with no activity.