Once #626688: Add caching for system_list() is in, the only required database query to serve a page from Drupal will be menu_get_item() - since everything after that will be determined by the page callback. If the page callback had a good caching strategy - not that hard to do with panels, then it's theoretically possible to serve pages to authenticated users without a single database hit, which would be a lovely thing for D7 to be able to do.
Attaching untested patch. There's a teensy-tiny API change compared to head - we no longer have a query tag there, but this was never alterable in D6.
Comment | File | Size | Author |
---|---|---|---|
#22 | 643984-menu-get-item-cache.patch | 978 bytes | dgtlmoon |
#19 | menu_get_item_caching_drupal6_backport.patch | 1011 bytes | csavio |
#5 | menu_get_item_caching.patch | 1.38 KB | catch |
#4 | menu_get_item_caching.patch | 1.23 KB | catch |
#4 | menu_get_item.png | 35.97 KB | catch |
Comments
Comment #1
catchHelps when you set the cache as well as get it :(
Comment #2
chx CreditAttribution: chx commentedIf you are not using the query tag there is no point in using the slower query builder either.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe, what chx said.
Comment #4
catchFixed $cid to not be longer than the schema definition, no longer using query builder.
Screenshot shows this as the last query (apart from system_list() executed before the menu handler gets going.
Comment #5
catchNeed to allow for paths longer than 255 characters - discussed this with beejeebus and chx in irc and we agreed on md5().
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedwoo, "then it's theoretically possible to serve pages to authenticated users without a single database hit" makes me smile.
patch looks fine to me, its very straight forward.
tried to test this with the D7 port of APC, and hit this: #644034: can't load alternate cache implementations.
edit: thats a dead end, as DamZ explained.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedhere are some results with the D7 APC module swapped in as the cache backend, ab -c 2 -n400, hitting front page, no content:
HEAD
Requests per second: 18.22 [#/sec] (mean)
Requests per second: 18.20 [#/sec] (mean)
Requests per second: 18.27 [#/sec] (mean)
patch
Requests per second: 18.89 [#/sec] (mean)
Requests per second: 18.98 [#/sec] (mean)
Requests per second: 18.96 [#/sec] (mean)
Comment #8
catchSo speaking to Earl, Panels apparently takes four queries to get from router item to serving a display (which can potentially contain all cached content), that's not 0, but removing this would cut 1/5 of the queries absolutely necessary to serve a full page.
At a minimum, apachesolr_autocomplete and similar would benefit from this - since that has a real menu callback which needs to be called, but everything is requests to Solr.
Comment #9
chx CreditAttribution: chx commentedVeeeeeeeery nice.
Comment #10
sunI don't get this.
This is replacing one range-limited query with two queries (select/insert) on many pages.
And if there is a cached result, then we just replace one query by another.
So what is the deal here?
I'm on crack. Are you, too?
Comment #11
catchOn a cache miss you'll get two queries, however the results should be cached for quite a while since it's rare that there's a menu_rebuild().
However if you run a non-database caching backend, then you can serve entire pages without hitting the database - for example an apache solr autocomplete path. So this is a scalability patch rather than performance - although the cache query is a bit faster than menu_get_item() because it doesn't have to resolve that big IN() with long strings, so it's still a small improvement even with the 1-1 exchange.
Comment #13
catchPassed, back to rtbc.
Comment #14
cburschkaWhat is the effect on memory usage?
Comment #15
webchickCould someone look at Arancaytar's question? Otherwise this looks good to go from my side.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedThese cache entries write to the DB, not to drupal_static(). There is no memory increase. If you choose to replace your cache.inc for APC or something, you are already quite aware about memory needs.
Comment #17
webchickThanks. Committed to HEAD!
Comment #19
csavio CreditAttribution: csavio commentedMy team needed this in 6 for performance reasons based on the last load tests we did. Here is a potential back-port patch for Drupal 6 based on what was done for Drupal 7.
Comment #20
csavio CreditAttribution: csavio commentedActually I realized this is going to be tested against 7x and will fail. I think I need to open a different issue for this.
Comment #21
webchickCross-linking #1234830: Revert cache_menu patch removal, add a $conf setting instead (was: cache_menu: huge table size). Turns out this patch is a little over-zealous in its caching. :)
Comment #22
dgtlmoon CreditAttribution: dgtlmoon commentedNot sure if this is really closed or not, however menu_get_item() for me is still adding +20 or so queries to the page, I've rerolled the patch from #19, maybe this patch will be of use to someone out there