Posted by Michelangelo on May 31, 2004 at 5:52pm
8 followers
| Project: | Drupal core |
| Component: | menu system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | JonBob |
| Status: | closed (fixed) |
Issue Summary
Performance hit was too high without menu caching.
Should this be made optional?
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| reintroduce-menu-cache.patch | 648 bytes | Ignored: Check issue status. | None | None |
Comments
#1
Numbers. We need numbers.
#2
This 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.
#3
Dries wanted numbers, here is a ab run without cache.
#4
Here is one with.
#5
And 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.
#6
For some reason some local tasks seemed to disappear after applying this patch. The "edit" tab at user pages is on of them.
#7
I 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.
#8
Okay, 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).
#9
Oops, cvs diff failed. Here's a manual diff; hope I remembered how to do those correctly. :-)
#10
Just benchmarked this patch on my local copy of drupal.org and it saves 190ms/request.
#11
Okay 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.
#12
#13
If 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.
#14
I 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.
#15
The 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.
#16
Cacheing 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.
#17
Here'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.
#18
I 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.
#19
This 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).
#20
(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
#21
Okay, 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.
#22
Committed to HEAD. Thanks.
#23
The 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().
#24
This was applied by Dries earlier today.
#25