Large performance impact with nice menu

I personally don't notice any slowdown, but I suppose it will vary by site, how big your menus are, etc. Yes it gets the whole menu tree even if not expanded, since it needs to build the full menu in HTML in order to function.

The problem with caching menus is that they can change per user or per page or both, depending on the use. In Drupal 5 there is no core block cache, so you would need to use the block cache module to cache the NM blocks. A related Drupal 6 issue (where core block caching does exist) is here: #284294: Attribute 'cache' should be set in blocks definition

No response - closing.

Yes there is a HUGE performance issue with Nice Menus. Most noticeable on sites that use node access and thus cannot use block caching. The problem being that Nice Menus loads the whole menu tree. This in turn causes any Views that are menu items to load. Nice menus added 500+ queries and > 3 seconds to my page generation times.

I'm guessing that there may be a cleaner way to do this (or maybe not in D6). The problem being that the menu system is not well documented.

As a workaround I did added the following theme override:

/**
 * Override of theme_nice_menu_primary_links().
 */
function YOURTHEME_nice_menu_primary_links($direction = 'down', $menu = NULL) {
  $cache_key = 'nice_menu:primary_links:'. $direction .':'. md5(json_encode($menu));
  $cache = cache_get($cache_key);
  if ($cache->data) {
    return $cache->data;
  }
  $output = theme_nice_menu_primary_links($direction, $menu);
  cache_set($cache_key, $output, 'cache', CACHE_TEMPORARY);
  return $output;
}

This will cache the whole thing for all users. It will get regenerated on each cron run. You may need to alter accordingly for your site.

I recently started analyzing my site with the XHProf PHP profiler. Guess what I found? The number one resource hog (in cpu ms) was - much to my surprise - Nice Menus. It seems a bit silly that there is no built-in caching options for a module that has the potential to add seconds on to page-load times. The sacrifice for caching should really just be the inability to know which menu option is the 'active' menu. But, quite honestly, I wasn't relying on this feature anyway as its a bit awkward to try and note the active menu item when dealing with DHTML drop down / out menus.

This is my vote for a config option to enable caching with the caveat that you lose any accurate identification of the 'current' menu item. It would seem to me that caching should be performed on a menu+direction+uid basis. I've seen some mention that caching would have to be per-user per-page but I honestly do not understand the need for the per-page piece if we are rendering the entire menu tree anyway.

Right now we are running a variation on dalin's theme override code (above) as follows:

/**
 * Implementation of theme_nice_menus()
 */
function mytheme_nice_menus($id, $menu_name, $direction = 'right', $dept$
  global $user;

  $key = 'nice_menu:' . $menu_name . ':' . $user->uid . ':' . $direction;
  $output = cache_get($key, 'cache_menu')->data;

  if (!$output) {
    $output = theme_nice_menus($id, $menu_name, $direction, $depth, $menu);
    cache_set($key, $output, 'cache_menu', CACHE_TEMPORARY);
  }

  return $output;
}

This currently lives in our theme's template.php file -- though would be nice to have it added to the module. If there is a willingness to commit it, I would be more than happy to submit a patch.

The code in my posting above has a few omissions - should be:

/**
 * Theme override for theme_nice_menus
 */
function mytheme_nice_menus($id, $menu_name, $mlid, $direction = 'right', $depth = -1, $menu = NULL) {
  global $user;
  
  // Attempt to retrieve the menu from the cache
  $key = 'nice_menu:' . $menu_name . ':' . $user->uid . ':' . $direction;
  $output = cache_get($key, 'cache_menu')->data;

  // Otherwise regenerate the menu and store it in the cache
  if (!$output) {
    $output = theme_nice_menus($id, $menu_name, $mlid, $direction, $depth, $menu);
    cache_set($key, $output, 'cache_menu', CACHE_TEMPORARY);
  }

  return $output;
}

Subscribe

If someone wants to supply a patch against Drupal 6 V2, I'll give it a look. I don't need this myself and I am certainly not going to do this for the D5 version. If no supplies a patch, I'll move this to a docs issue so folks can have it as a reference when searching.

OK, I think this patch results in the lowest number of cache entries while still being as flexible as possible.

++++++++++++++++++Ship it!!!
We have a large menu structure and complicated access controls. The patch in #8 alone reduced an example page load time from 3018.07 ms to 1405.6 ms.

If you place your primary links into the header region via a block (and remove $primary) from your page.tpl.php then you can cache the block smarter using Block Cache Alter (set by role, by user, by page, etc.)

Also I would recommend breaking it into two menu block so that one can be globally cached and another can be based on the user, then just place the two menus side by side.

subscribe
waiting for release

1.7% speedup on /user page for anonymous user on a large site

w/o patch

ns2# ab -c5 -n100 http://nicemenutesting/user
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking nicemenutesting (be patient).....done


Server Software:
Server Hostname:        nicemenutesting
Server Port:            80

Document Path:          /user
Document Length:        44444 bytes

Concurrency Level:      5
Time taken for tests:   23.805 seconds
Complete requests:      100
Failed requests:        98
   (Connect: 0, Receive: 0, Length: 98, Exceptions: 0)
Write errors:           0
Total transferred:      4518260 bytes
HTML transferred:       4470960 bytes
Requests per second:    4.20 [#/sec] (mean)
Time per request:       1190.230 [ms] (mean)
Time per request:       238.046 [ms] (mean, across all concurrent requests)
Transfer rate:          185.36 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:  1006 1172 221.2   1105    2180
Waiting:      941 1093 215.4   1029    2049
Total:       1006 1172 221.2   1105    2180

Percentage of the requests served within a certain time (ms)
  50%   1105
  66%   1120
  75%   1154
  80%   1162
  90%   1391
  95%   1853
  98%   2060
  99%   2180
 100%   2180 (longest request)

 

w patch

 
 ns2# ab -c5 -n100 http://nicemenutesting/user
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking nicemenutesting (be patient).....done


Server Software:
Server Hostname:        nicemenutesting
Server Port:            80

Document Path:          /user
Document Length:        45072 bytes

Concurrency Level:      5
Time taken for tests:   23.409 seconds
Complete requests:      100
Failed requests:        99
   (Connect: 0, Receive: 0, Length: 99, Exceptions: 0)
Write errors:           0
Total transferred:      4521481 bytes
HTML transferred:       4474181 bytes
Requests per second:    4.27 [#/sec] (mean)
Time per request:       1170.456 [ms] (mean)
Time per request:       234.091 [ms] (mean, across all concurrent requests)
Transfer rate:          188.62 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    3  15.8      0      98
Processing:  1008 1166 151.1   1113    1710
Waiting:      940 1074 141.7   1019    1563
Total:       1009 1169 150.1   1118    1710

Percentage of the requests served within a certain time (ms)
  50%   1118
  66%   1151
  75%   1183
  80%   1214
  90%   1437
  95%   1561
  98%   1607
  99%   1710
 100%   1710 (longest request)

Thanks for you work on this caching.

I appreciate the benchmarks, What's with all of the failed requests? I'm not ready to make a call based on that just yet.

Also the patch is making a cache key times every menu * every user id, that will add a ton of writes to a busy site.

How about handling this at the Block level? Currently it's set to BLOCK_NO_CACHE, and there may be an easy win to be had there.

Based on the code comments, this was avoided in the past because menus can be so different for different users.

I appreciate the benchmarks, What's with all of the failed requests? I'm not ready to make a call based on that just yet.

The failed requests in the benchmark are _probably_ fine - note the cause - length. This will happen if either the HTTP header or HTML document differs by one byte. To confirm run wget twice on the page and diff the two.

Also the patch is making a cache key times every menu * every user id, that will add a ton of writes to a busy site.

See Add1sun's remarks from #1 "The problem with caching menus is that they can change per user or per page or both, depending on the use." Also any busy site will either be already experiencing this issue, or be using block caching and thus this would not experience tonnes of writes. For a menu of even moderate size, writing to the cache once per user per menu is faaaar cheaper than rebuilding several views on each page load. Putting this patch into place will greatly improve the situation for 99% of sites. For the 1% that may be adversely effected by high cache writes I see two options:

1) Include a setting at admin/settings/nice_menus to allow the admin to choose caching options. Flexible, but challenging to do in a way that is not confusing to the hobbyist, while still useful to the enterprise site.
2) Same as #1, but don't have a setting on the admin page. Just use variable_get() and make a note in README.txt to direct advanced users where to go.

I'm kinda partial to #2.

different size in benchmarks are result due some profiling code from xhprof, so - nevermind
Menu is small in my website version, so possibly we need to test huge menus with many items against this patch

ajax it.

+1 the caching patch was quite helpful to me. I'm running with it now in prod.

Fixing tag.

Just ran across this thread again. We started running with this patch originally; however, this cached information doesn't have the ability to get cleared on node actions (I.e. add/edit/delete). We switched to using block caching and receive similar performance benefits.

HTH.

Yes I suppose if you are linking to nodes in the menu that would be the case. In that case switching from cache_menu to cache_block would do the trick. I'm not sure that we can assume that the cache_block cache bin exists though.

To clarify, I meant to say that we reverted this patch and are only using block caching (block cache alter) at the moment.

So, it sounds like we can avoid having to change the module and just point to Block Cache Alter in the README. Sound good?

Block Cache Alter won't help you if you rely on $primary_links. We still need a better solution.

And what about block-level caching? What is wrong with *not* hardcoding it? I am not sure I understand the problem with $primary_links vs block_cache_alter...

This simple patch may help people a lot, but may break other sites.

diff --git a/nice_menus.module b/nice_menus.module
index 743db51..eae0746 100644
--- a/nice_menus.module
+++ b/nice_menus.module
@@ -184,7 +184,7 @@ function nice_menus_block_info() {
   for ($i = 1; $i <= variable_get('nice_menus_number', '2'); $i++) {
     $blocks[$i]['info'] = variable_get('nice_menus_name_' . $i, 'Nice menu ' . $i) . ' (Nice menu)';
     // We have too many things changing per user, per page to cache.
-    $blocks[$i]['cache'] = DRUPAL_NO_CACHE;
+    $blocks[$i]['cache'] = DRUPAL_CACHE_PER_ROLE;
   }
   return $blocks;
 }

Maybe it should be part of the nice menu configuration to choose how it is cached? That would site well in nice_menus_block_configure...

@dalin Will your patch in #8 work for D7. I'm still seeing bad performance from nice menus.

I am closing this issue, since Drupal 4.x, 5.x, and 6.x are now not supported.