Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Performance hit was too high without menu caching.
Should this be made optional?
Comment | File | Size | Author |
---|---|---|---|
#23 | menu_12.patch | 3.18 KB | JonBob |
#21 | menu_11.patch | 93.31 KB | JonBob |
#17 | menu_10.patch | 92.99 KB | JonBob |
#11 | menu_9.patch | 89.75 KB | JonBob |
#9 | menu_8.patch | 86.64 KB | JonBob |
Comments
Comment #1
Dries CreditAttribution: Dries commentedNumbers. We need numbers.
Comment #2
TDobes CreditAttribution: TDobes commentedThis patch no longer applies... this probably needs significant updating to work with the latest version of the menu system. I'm taking this out of the patch queue.
Comment #3
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedDries wanted numbers, here is a ab run without cache.
Comment #4
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedHere is one with.
Comment #5
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedAnd here is an updated patch. The patch is probably not complete, it might some cache clearing here and there. JonBob, can you please have a look at it?
Executive summary of the ab runs:
--- menu-without-cache.txt Thu Aug 5 02:59:28 2004
+++ menu-with-cache.txt Thu Aug 5 02:59:28 2004
-Requests per second: 1.14 [#/sec] (mean)
-Time per request: 877.238 [ms] (mean)
-Transfer rate: 42.91 [Kbytes/sec] received
+Requests per second: 1.39 [#/sec] (mean)
+Time per request: 718.555 [ms] (mean)
+Transfer rate: 52.10 [Kbytes/sec] received
Requests per second goes up by 20%. Not all that bad.
The comparison has been done as user #1 with several menus enabled and thus a fairly large menu.
Comment #6
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedFor some reason some local tasks seemed to disappear after applying this patch. The "edit" tab at user pages is on of them.
Comment #7
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI still think that menu caching is a "must" in terms of performace. Unfortunately, my patch only addresses a part of the problem (and that's why the local tabs were not displayed). JonBon says it is possible to do menu caching, but that it would be complicated.
Comment #8
JonBob CreditAttribution: JonBob commentedOkay, I've taken a shot at this.
This patch reintroduces the per-user menu cache by adding a "path" parameter to hook_menu(). This parameter is present if context-sensitive menu items (such as the node edit tabs) are being requested. So most menu hooks only return menu items when the path is absent. This allows us to cache those menu items, then append the context-sensitive ones to the menu structure after the cache has been consulted.
This patch needs to be benchmarked to see if it is a significant performance gain. If so, then to finish it up I need to audit code for places where the cache needs to be invalidated (module activation, for example). I also then want to look for ways to refactor menu.inc code to avoid duplication (this was partly a copy/paste job).
Comment #9
JonBob CreditAttribution: JonBob commentedOops, cvs diff failed. Here's a manual diff; hope I remembered how to do those correctly. :-)
Comment #10
Dries CreditAttribution: Dries commentedJust benchmarked this patch on my local copy of drupal.org and it saves 190ms/request.
Comment #11
JonBob CreditAttribution: JonBob commentedOkay then, here's a version that adds cache clearing where necessary, and does some refactoring. I changed the $path parameter to a boolean, $may_cache, since using arg() is more convenient than parsing a $path string anyway.
Comment #12
Dries CreditAttribution: Dries commentedComment #13
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedIf you are concerned about an increase in the cache table's number of entries we should consider storing the diffrent types of cache (page, menu, filter, ...) in different tables. The filter cache alone will get a large number of entries in time.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedI think a more appropriate place for per user cache is in the $_SESSION object. All you have to do is add stuff to that array and PHP/Drupal handles the rest ... This should help prevent a slowdown, since no new rows are introduced.
Comment #15
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedThe idea of storing the menu structure in the session sounds intriguing, but has some problems on its own. The structure is quite big: 56k were reported. Also there were 11000 open session reported on drupal.org. So we'd need to handle anonymous users separately. registerd users only have 350 open sessions. So I think we should store the menu structure in the cache table and clear it if the user's session expires.
Comment #16
(not verified) CreditAttribution: commentedCacheing in the SESSION is problematic. How do you invalidate the caches? To do so you will have to have database / filesystem writes of a timestamp so putting it in the SESSION would result in more round trips to persistent store.
Are we sure that there are no code level ways to improve menu performance before adding complex caching? I'm not sure there is a difinitive way to know when the menu cache should be invalidated.
Comment #17
JonBob CreditAttribution: JonBob commentedHere's a new version. This one caches on roles rather than users, so cache usage is much more conservative. To do this, user-specific items like "my account" have been moved out into the non-cacheable block of their menu hooks.
I also modified the code to not produce empty "callback arguments" and "description" fields when those are omitted, and to add checks for the presence of these where they are used. This makes the code just a tiny bit uglier IMO, but it reduces the size of the serialized menu by over 10K, so I think it's worthwhile.
Please test and, if motivated, benchmark.
Comment #18
Dries CreditAttribution: Dries commentedI haven given it some thought and I might be OK with the per-user cache if the cached menus are invalidated frequently. If we invalidate the menus upon cache_clear_all(), the number of cached menus will be acceptable. Typically, the number of active users is only a fraction of the number of registered users.
Comment #19
Bart Jansens CreditAttribution: Bart Jansens commentedThis is a lot faster, reduced page execution time on one of my pages from 85ms to 35ms (and thats without updating all my custom modules, so the end result might be even better).
I did notice some weird behavior tho,
- every time i went to admin/menu, two new entries were added to the menu table even tho they already existed:
| mid | pid | path | title | weight | type | description |
+-----+-----+--------+--------------+--------+------+-------------+
| 241 | 1 | logout | uitloggen | 10 | 22 | |
| 240 | 0 | user/9 | Mijn profiel | 0 | 22 | |
- on the menu admin page the 'create content' entry is shown before the 'recent posts' but in the menu the order was reversed (the order of the other links was correct).
Comment #20
Bart Jansens CreditAttribution: Bart Jansens commented(previous post was about menu_10.patch)
killes asked me to try menu_9.patch so here are the results:
before: 85ms
menu_9: 30ms
menu_10: 35ms
Also, i didnt notice the same quirks with this patch
Comment #21
JonBob CreditAttribution: JonBob commentedOkay, here's a compromise between the last two patches.
This one goes back to per-user caching. While most users have menus that are nearly identical, this method allows less room for security problems. The worst case scenario with a user-based cache is that a user is granted access to a page she had access to in the past but doesn't now; the worst for a role-based cache is that a user is granted access to a page accessible to others in her role but never to her. Neither of these situations should ever happen, but if a bug were to creep in the latter would be more disastrous.
To compensate for the size of the caches somewhat, this uses the new filter expiry mechanism to remove cached menus after a day, so that we aren't stuck with caches for users with no activity.
This patch also includes the serialized size improvement from the previous one.
Comment #22
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #23
JonBob CreditAttribution: JonBob commentedThe legacy handlers and file upload previews were mistakenly cached when they cannot be. Attached patch fixes this as well as a reference to an undefined constant in legacy_menu().
Comment #24
Steven CreditAttribution: Steven commentedThis was applied by Dries earlier today.
Comment #25
(not verified) CreditAttribution: commented