Client-side caching of administration menu

smk-ka pointed out that we could simply use a cookie that holds the CRC of the rendered menu string. If the client-side cached menu does not has the same CRC, it needs to be rebuilt. Using a cookie also allows us to determine it on the server-side already.

Note that I already talked with members of the security team about this issue. They did not see anything that would have to be secured. This means we will not even encrypt the menu on the client-side.

However, we will still need a CRC or "version counter" to trigger an update of the client-side cached menu when there is new data.

VERY interesting. I did not think about this option before. I don't know whether it is better or not, but it obviously would be an alternative approach.

I want to see the client-side caching directly in admin_menu. Your approach looks very, very appealing. Though I still wonder whether sessionStorage would provide us some yet-unknown additional benefits and/or possibilities. At the very least, I think both approaches could even co-exist, depending on the browser being used (and of course, if there is a difference in possible functionality at all).

The most important thing when (or before) working on a patch is to flesh out the proper logic.

- For clients with JS support only the very first request should build and output the menu.

- For clients without JS support the behavior does not change at all.

- The client must cache per uid, language, site.

- Whenever the menu is rebuilt (not the JS), the client needs to fetch the newly built menu.

- On all pages using the client-side cache, admin_menu must not generate, build, or output the menu at all, which means zero impact on performance. Note, however, that we will still generate Drupal.settings, and will also generate any dynamic data (bells & whistles) for the menu, e.g. the menu item value for the user counter and similar stuff.

- If retrieval of the cached menu fails for any reason, there either must be an automatic and/or manual fallback logic. (In my original test, the default output of admin_menu for clients with JS support was a link titled "Click here if the administration menu does not show up.", which was replaced by the cached menu. Clicking on this link would have removed the has_js cookie, because obviously something went wrong or the browser failed somewhere [think mobile browsers aso.])

The more I think about this approach, the more I like it - because it's bad-ass simple and is based on HTTP standards.

Let's keep this on topic. 3.x already caches the entire menu output. The output is only rebuilt if the menu is rebuilt. The implied issues resulting of this caching are already known, and yes, also the non-client-side-cached version will use JavaScript to update the data for specific menu items (one of them being the user counter) - but it will use Drupal.settings, because we do not really want to hit Drupal once again for this data (which would have the opposite result in terms of performance in the end). Admittedly, this dynamic update issue is not yet mentioned in #402058: Requirements for Administration menu in Drupal core though... ;)

So, a very rough description of what we want to achieve in this issue is: The client-side caching performs the same caching as the server-side caching. Both caches use the same cache key and also their caching behavior is identical. Only the cache storage differs.

The additional difference and challenge is that the menu is not visible nor functional when the client-side cache fails.

Please, can we please stay on topic? I'm really amazed that you're working on this, but if we do not focus on atomic issues, this won't result in anything that works. Dynamic update/replacement of data in the cached menu output is a completely separate issue, for which I would already have a lot to say, but not here. I really do not want to see any further word about that in this issue. If you really want to work in parallel on both issues, then we can create a new issue for it and discuss "over there".

Using a hash of the output sounded interesting in the first place. However, it would require that the menu output for multiple users on the same (multi-)site and in the same language is identical. That is the case for users sharing the same user roles / permissions. But admin_menu also contains user-specific items, such as the user's name - which I am not too sure we want to pass on and replace dynamically via Drupal.settings. Additionally, this would not really allow that any module or admin_menu itself could output user-specific tools and tweaks in the menu; for example, users might configure in their account settings that they want to see a Finder-alike search bar. Of course, your logic would still work. But I fear that we would extract too much user-specific data into dynamic replacements in the long run.

That said, I'm not completely sure about what I've said in the last paragraph. And because I'm not sure, I'd prefer us to work in smaller steps, so we can get a better idea of what is possible or makes sense and what not (similar to an agile development approach). Can we agree on that? For example, I've great success using this strategy in Wysiwyg API thus far.

For now we should focus on implementing the client-side cache exactly as the server-side cache, ensure that no user gets the wrong menu, the menu is correctly updated if there is a new version, the fallback works, and the ordinary menu is properly displayed for users without JavaScript. That's already a lot of functionality, and it's the core functionality for client-side caching, so we have to make sure that it is stable and fail-proof.

Regarding #9: Did you already take everything of that into account? Or do you have any amendments? If not, I'd be highly interested in a patch against 6.x-3.x or HEAD - which should not be hard to do.

Just to make sure we're speaking of the same version: All 3.x versions of admin_menu no longer cache structured data. They cache the final output of the menu; a simple HTML string. Which means that if there is a cached menu, it is output directly without any further processing. This was done as a prerequisite for this issue. See #349505: Performance: Cache menu tree for details (though the title of that issue could lead to wrong assumptions).

At least that's why my primary goal is get both caches working identically. Are we on the same line, do I miss something, or do we just talk at cross-purposes? ;)

Ok. Migrating this into admin_menu is not so trivial (just like I predicted).

I'll recap here now, primarily to get it sorted out for myself.

Client-side caching process:

- User logs in, gets a page with admin menu already contained. (Dunno when has_js cookie is really defined first)
- On the very same page, we already compute the hash for the menu. [STOP]
- User visits next page. Due to has_js cookie, admin menu is NOT output; it's not even loaded from server-side cache. Just the hash in Drupal.settings. [STOP]
- Client takes hash, prepends the cache URL, gets the menu. Due to our headers, the browser caches this request's content.
- User visits next page. Client gets hash; cache headers prevent from fetching the same URL again.
- User visits next page, but menu was rebuilt. Different hash in Drupal.settings makes the client fetch the new menu.

STOP == Where do we store the hash? $_SESSION? $user? $conf? $user for now.

We also want to prepare for http://drupal.org/project/js

Additional benefit: Regular Drupal behaviors are not applied to admin menu. We had issues with External Links and Google Analytics modules in the past, because both modules parse each and every link on a page (and admin menu can contain *many* links on large-scale sites).

Remaining todos:

- Menu stays the same when changing language/locale. We need to store hashes per language (and site?) in $user, i.e. using an array, keyed by xyz.

- Firebug tells me that each AJAX request still takes 128ms. Is this due to the bad performance of my local Apache server?

- admin menu behaviors are not applied on all pages. Seems like we need to disable async request for $.ajax.

Note that I won't be able to work on this for the next few days due to other obligations. However, I'd really, really, really like to see this getting RTBC. This is incredible!

Regarding the client-side cache key:

The solution is to use the very same cache key as the server-side.

$uid:$language->language:$hash

- We don't need $site, because the browser will query a different host anyway.
- (I think) we need $uid, because we have to support stuff like Devel's switch user.

That way, the first issue should be resolved.

Thanks for that ahah.js pointer - I wasn't aware of that.

Regarding storage, I already started brainstorming about this whole new thing. Long story short: I believe that we will be able to also cache on-demand loaded sub-menus by making the very same approach more generic. There was a feature in admin menu 5.x that listed existing views and panels below their respective "List" items in the menu, which will be resurrected in 3.x in a more generic way (see #293768: Allow to enable/disable menu additions about details). Also, the aforementioned search bar might use it. In all cases, it boils down to the logic: If the system data is still the same (e.g. no view was created/renamed/deleted), then the additional dynamic data is the same - here even for all users. Whether all of this will happen, is certainly a different question.

However, because of those preliminary thoughts, I'm rather leaning towards an own db table in the meantime, because sessions can expire very often (but the menu would still be the same).

After talking to smk-ka about the page request/generation times, it seems we can't decrease them any further. On regular requests, they take between 130-300ms. By implementing JS/AJAX callback handler, I was able to get them down to 90-120ms. Still too much.

The reason for those long request times is that Drupal is still invoked. jQuery's $.ajax() does not seem to hit the browser cache. We tinkered about implementing the ImageCache approach, i.e. dynamically creating and storing the cached output as a file. That way, Apache would directly return "Not Modified" and request times would decrease to ~5ms. The only "caveat" would be that those cache files /could/ be accessed by other users if they find a way to determine a user's hash. But then again, the security team said this would not matter...

The current implementation breaks admin_menu_suppress(), which means other modules can no longer suppress admin_menu on certain pages. It seems admin_menu_suppress() needs to add a JS setting instead now.

Fixed the suppression issue.

Attaching current state of what is working so far for me.

@markus_petrux: Good point, fixed in attached patch.

Anyway, that's a bit too nitpicky for the current situation.

Where do we go from here? The current implementation _does not work_ as intended. As stated above, jQuery's ajax() does not account for the browser's cache and leads to a full, second Drupal bootstrap as it stands. Possible reasons/options:

a) jQuery's ajax() is buggy. Try to fix it, so it incorporates the local browser cache. When it is fixed, require jquery_update + latest version of jQuery for admin_menu 3.x.

b) Our returned HTTP headers are buggy, so the behavior of ajax() is right.

c) Our options for ajax() are buggy, so it properly requests a new version.

d) Current idea won't work and Drupal will always bootstrap. Implement an approach like ImageCache using file-based caching, so Apache handles the cache requests instead of Drupal.

e) Revert to the original idea of using Thomas Frank's sessionvars and/or sessionStorage.

I don't care whether the current implementation might work with a squid or any other HTTP proxy server in the middle. For 99.99% of all users, it does not work.

On b) As mentioned before, I already investigated this a bit, so I would say that our returned headers should be right.

On d) Caching the menu will probably be ok from a security perspective. But when it comes to caching sub-menu links (i.e. lists of existing system objects), I'm not sure whether that would expose too much.

So, where do we go from here?

Went ahead and

- tested on D7 with jQuery 1.3 - no difference.
- checked server access logs - lists 1 (actually 2) request(s) for js/admin_menu/cache/[hash]
- commented out the gzip compression part - no difference.

Offtopic, but interesting: Those requests take 450-500ms each on D7; that's even more than D5 and D6.

I'll test your admin_menu_cache module now, but I don't think it will behave differently. Also, could you try this patch yourself?

Hrm. Actually, I checked the access log when testing jQuery 1.3 in D7. In D6, I see no requests in the access log. So it somehow seems to work - but I don't understand why it takes so much time to load the content from the browser cache.... I mean, when surfing in the system, I can see that admin menu is injected after some time - so the time reported by Firebug's Console seems to be the actual time.

I've tweaked the implementation even further now.

- Page output no longer flickers/jumps.

- Added Drupal.admin.getCache() as a preparing step for #293768: Allow to enable/disable menu additions

- Added initialization of settings to reduce the amount of conditional code.

Remaining todos:

+    // @todo Clean URLs required for JS callback handler support.
+    url: Drupal.settings.basePath + 'index.php?q=js/admin_menu/cache/' + hash,

+  // Do nothing at all here if the client supports client-side caching, no
+  // rebuild is needed, the user has a hash, and is NOT requesting the cache
+  // update path.
+  // @todo Implement a sanity-check to prevent permanent double requests; i.e.
+  //   what if the client-side cache fails for any reason and performs a second
+  //   request on every page?
+  if (!empty($_COOKIE['has_js']) && !$rebuild && !empty($user->admin_menu_hash) && strpos($_GET['q'], 'js/admin_menu/cache') !== 0) {
+    return;
+  }

+  // Store the new hash for this user.
+  // @todo Use an own storage for our hashs, so we can cache sub-menus as well.
+  if (!empty($_COOKIE['has_js'])) {
+    // @todo Hash needs to contain uid and language (just like cid).
+    user_save($user, array('admin_menu_hash' => md5($content)));
+  }

+  // @todo Find a proper cache lifetime.  Infinite, i.e. 1 year?
+  header('Expires: ' . gmdate('D, d M Y H:i:s', time() + 86400) . ' GMT');
+  header('Last-Modified: ' . gmdate('D, d M Y H:i:s') . ' GMT');
+  header('Cache-Control: max-age=' . 86400);
+  header('Content-Length: ' . strlen($content));

But also: Make it work in D7.

We are almost done. I want to hear proposals for the remaining todos. Of course, please also test.

@markus_petrux:

1) So you get the same reponse times I mentioned earlier or do they differ for you?

2) I did not adapt the code for the way ahah.js works, because I want to see a bug report first. Looking at ahah.js tells me that it is performing a different request, using a different dataType. Also note that I changed the dataType to 'text' in my latest patch, because 'html' would automatically search for containing SCRIPT tags in the response.

3) There is no difference in processing time between prepend() and append(). I changed it to prepend, because the entire menu was positioned at the bottom of the window in IE. Not sure whether that still happens now though.

4) Dynamic replacements and on-demand loading of additions is still a different issue. Currently, the menu is not updated at all, even in cases where it should (menu rebuild). That is another issue I forgot to mention in my last follow-up.

Eliminated some of the todos. The new basePath JS setting is what I actually want to see in core. Drupal.settings.basePath is almost always wrong in its current state.

Remaining todos:

1) Use an own storage for hashs. As visible in this patch, hashs need to be keyed by uid, language, and possibly site (on the server-side). We could use an own table 'admin_menu_hash' using optimized integer columns for those keys, or we could use a cache table, which would allow for flexible key names and would also make sure that users are not sharing this table across multi-sites (i.e. like any other cache table).

2) Avoid separate AJAX requests for clients that don't have a cache (or disabled cache).

3) Ensure the hash properly changes when the menu is rebuilt. (Not sure if this is still an issue though.)

4) Ensure proper styling and behavior in IE. (Not sure if this is still an issue though.)

5) Ensure this works in D7.

quicksketch requested to commit the clean-ups separately.

Help. I'm a bit lost with D7.

access log shows for each request:

"GET / HTTP/1.1" 200 6243
"GET /sites/all/modules/admin_menu/admin_menu.js?7 HTTP/1.1" 304 -
"GET /js/admin_menu/cache/52995beb65c2f73c3299dd3e441bd2ec HTTP/1.1" 200 1394
"GET /js/admin_menu/cache/52995beb65c2f73c3299dd3e441bd2ec HTTP/1.1" 200 1394

Note that there are even 2 requests for the cached menu per request.

The only difference to D6 seems to be those two lines in the response header:

Keep-Alive	timeout=5, max=98
Connection	Keep-Alive

I have no idea why this happens. Attached patch is for D7.

Related, but not tied to this D7 issue:

- http://www.mnot.net/blog/2006/05/11/browser_caching mentions that IE(6) only caches very certain documents.

- http://betterexplained.com/articles/how-to-optimize-your-site-with-http-... mentions using an ETag as further possibility.

New patch for D6, fixing a basePath issue on sites using clean URLs, language path prefixes, and/or having Drupal installed in a subdirectory.

@domesticat: That is possible if you upgraded from 6.x-1.1. This bug was fixed in 6.x-1.2. You just need to flush the "Administration menu" cache to remove the duplicates (once).

Thanks for testing! Can you concurrently look into your web server's access log to make sure that subsequent requests do NOT hit the web server? (i.e. the menu is served from your browser's cache)

New patch for D7, accounting for the new settings argument for Drupal behaviors.

Double requests only happen occasionally now, and, even with Drupal core JavaScripts, such as the timezone auto-detection on user/1/edit. This means this is a core bug, so we just don't care (in this issue).

Created an issue for Drupal core: #418304: AJAX/AHAH requests are hitting server twice

How to test this patch:

1) Apply the patch.
2) Clear your site's cache.
3) Clear the cache of your browser(s).
4) Visit a bunch of pages on your Drupal site, just "surf" around - but avoid admin/build/modules ;). If you have Firebug or similar add-ons installed, you should see a separate AJAX request to js/admin_menu/cache/... on each page you visit.
5) Open your web server's access log. Confirm that there is only *one* entry containing js/admin_menu/cache/...

So far, I was able to test Firefox 3.0, IE6, and Opera; all on WinXP.

Great! Thanks for testing.

Well, I'm silently using this nice thang on a few production sites already (because I know what I need to clean up later) - so, yes, we are very close to completion. :)

Remaining todos:

1) Add a cache_admin_menu table to store hashes. Cache keys consist of uid and language. (Cache schema is re-usable, see system.install)

Deferred to separate issues:

- Avoid separate AJAX requests for clients that don't have a cache (or disabled cache). (Failsafe measures)

- Ensure the hash properly changes when the menu is rebuilt. I am absolutely not happy with the current implementation, and after tinkering intensively about it, we should really rebuild whenever hook_menu() is called. AFAIK, menu_rebuild() is (almost?) always called at the end of system-changing request, so we just need to ensure the _next_ request uses a new hash (i.e. truncate the cache table).

- Create a core patch for the new basePath JS setting. (IMHO, this is a bug fix)

This time with new cache_admin_menu table and low-level functions.

Hopefully last time "needs review". If anyone has any considerations or objections then speak up - now.

#345984 by markus_petrux, sun: Added client-side caching of administration menu.
  Attention: A new era of Drupal user experience starts here.  This is the very
  first issue of a series of improvements targeting plain awesomeness.

Now pick your favorites and create patches:

#420802: Prevent separate AJAX request for clients without client-side cache
#420806: Ensure proper rebuilding of hash and menu
#420808: Remove obsolete caching
#420812: Implement hook_js()
#293768: Allow to enable/disable menu additions
#420828: Allow dynamic replacements in the menu
#420816: On-demand loading of dynamic paths and local tasks #1
#420836: Move module functions into include file
#420840: Ensure Drupal.behaviors.adminMenu is only executed once

Interesting! I'd definitely be (very) open to re-visit other client side storage methods at some point in the future.