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.

CommentFileSizeAuthor
#22 643984-menu-get-item-cache.patch978 bytesdgtlmoon
#19 menu_get_item_caching_drupal6_backport.patch1011 bytescsavio
#5 menu_get_item_caching.patch1.38 KBcatch
#4 menu_get_item_caching.patch1.23 KBcatch
#4 menu_get_item.png35.97 KBcatch
#1 menu_get_item_caching.patch1.3 KBcatch
menu_get_item_caching.patch1.25 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new menu_get_item_caching.patch1.3 KB

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

chx’s picture

chx commented
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

Anonymous (not verified) commented

subscribe, what chx said.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new menu_get_item.png35.97 KB
new menu_get_item_caching.patch1.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

catch
he/him
Primary language English
commented
StatusFileSize
new menu_get_item_caching.patch1.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

Anonymous (not verified) commented

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

Anonymous (not verified) commented

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

catch
he/him
Primary language English
commented

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

chx commented
Status: Needs review » Reviewed & tested by the community

Veeeeeeeery nice.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ 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

catch
he/him
Primary language English
commented

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

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Passed, back to rtbc.

cburschka’s picture

cburschka
they
commented

What is the effect on memory usage?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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

moshe weitzman commented
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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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

csavio commented
Status: Closed (fixed) » Needs review
StatusFileSize
new menu_get_item_caching_drupal6_backport.patch1011 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

csavio commented
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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

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

dgtlmoon commented
StatusFileSize
new 643984-menu-get-item-cache.patch978 bytes

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