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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Helps when you set the cache as well as get it :(

chx’s picture

Status: Needs review » Needs work

If you are not using the query tag there is no point in using the slower query builder either.

Anonymous’s picture

subscribe, what chx said.

catch’s picture

Status: Needs work » Needs review
FileSize
35.97 KB
1.23 KB

Fixed $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.

catch’s picture

FileSize
1.38 KB

Need to allow for paths longer than 255 characters - discussed this with beejeebus and chx in irc and we agreed on md5().

Anonymous’s picture

woo, "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.

Anonymous’s picture

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

catch’s picture

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

Veeeeeeeery nice.

sun’s picture

+++ includes/menu.inc	26 Nov 2009 16:08:53 -0000
@@ -403,13 +403,17 @@ function menu_get_item($path = NULL, $ro
+    if ($cached = cache_get($cid, 'cache_menu')) {
+      $router_item = $cached->data;
+    }
+    else {
+      $router_item = db_query_range('SELECT * FROM {menu_router} WHERE path IN (:ancestors) ORDER BY fit DESC', 0, 1, array(':ancestors' => $ancestors))->fetchAssoc();
+      cache_set($cid, $router_item, 'cache_menu');
+    }

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

catch’s picture

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

Status: Reviewed & tested by the community » Needs review

Re-test of menu_get_item_caching.patch from comment #5 was requested by webchick.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Passed, back to rtbc.

cburschka’s picture

What is the effect on memory usage?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Could someone look at Arancaytar's question? Otherwise this looks good to go from my side.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. Committed to HEAD!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

csavio’s picture

Status: Closed (fixed) » Needs review
FileSize
1011 bytes

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

csavio’s picture

Status: Needs review » Closed (fixed)

Actually I realized this is going to be tested against 7x and will fail. I think I need to open a different issue for this.

webchick’s picture

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

dgtlmoon’s picture

Not 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