With the default install profile, menu_load() is called three times on every request. It has no static caching, and does a query per menu - despite the custom menu table usually being less than ten-fifteen actual menus.
I'd suggest we add a menu_custom_menus() function which loads all basic info about custom menus into a single array, then have menu_load() access the results of that. We could also consider caching menu_custom_menus() at the db level to save database access on sites running memcached, since it's extremely rare that this list needs to be rebuilt.
Comment | File | Size | Author |
---|---|---|---|
#141 | 638070-user-uid-only-optional-141.patch | 7.22 KB | Gábor Hojtsy |
#134 | 638070-user-uid-only-optional-131.patch | 6.41 KB | pwolanin |
#129 | 638070-user-uid-only-optional-129.patch | 6.42 KB | pwolanin |
#122 | user-uid-only-optional.patch | 6.61 KB | Gábor Hojtsy |
#118 | user-uid-only-optional.patch | 7.02 KB | Gábor Hojtsy |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedwith navigation turned on, i get three calls for that as well (along with the three for management). nasty.
Comment #2
catchSo the reason why this happens is this:
Contextual links uses proper menu access checks to decide if links should be displayed or not.
It does this for each actual menu displayed on the page to generate links for those.
Proper menu access checks require _menu_load_objects() to be run for every router path, which calls menu_load(), user_load() etc.
Up until last week, we were also doing additional database hits for every comment displayed on a page (edit/delete) due to contextual links, and the same for blocks. However comments was rolled back, blocks are now using %/% style paths instead of %block/%delta style.
I'm slightly broadening the scope of this issue since this needs dealing with at the menu system level - even if that's only documenting the behaviour a bit more explicitly and changing core to abide by it.
The places I'm currently aware of, in HEAD at least, suffering from this, are the My Account link - this triggers a user_load() via user_uid_optional_to_arg() (up to 6% of the request time with default profile), and this menu_load() for each menu.
So we could consider moving to non-loader paths for these, although that may not be possible with user_uid_optional, or alternatively try to find a way to allow the menu system to check access without invoking the menu loader, or add a menu_load_menus() or similar to work around it.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedBumping to critical. 6% of request time for the My Account link is insane given that we already know that the user has account if he is logged in.
Comment #4
catchMoving to menu component.
Comment #5
chx CreditAttribution: chx commentedI have measured these on /user with ab using a cookie from uid 1 (and checking that the returned HTML was the same size as saved from the browser as logged in) and could measure absolutely no change.
Comment #6
catchHmm, chx's patch tries to fix an aspect of this issue, but not the one that causes the main problems here.
Let's take menu_load() as an example.
When we're generating contextual links, we check access for each menu, and this triggers a menu_load() per menu. However, the 'access callback' for that is user_access(). Therefore menu.module should be able to say "Yes, I've got a menu loader, but when you're just checking access to the menu link I'm creating, I don't need my loader function called for me thaks.". Similar the user_load() which happens in HEAD, only happens due to the 'My Account' link. Static caching doesn't do anything for us in either of these cases.
This would involve either:
1. Add an additional router key to hook_menu() - like 'skip_menu_loader_for_access_check'
2. Or if we're not allowed a schema change, have access_arguments allow for a specific array key/value like 'load_object' => FALSE.
Either of these involves _menu_translate() looking at access_arguments() before going to menu_check_access().
Comment #7
pwolanin CreditAttribution: pwolanin commented@catch - so in a sense we call the access callback before the loading callbacks IF the access arguments don't contain any integers?
Comment #8
catchpwolanin, yeah I think that'd work.
Just realised that this whole idea won't help %user_uid_optional since that takes an $account parameter - but we don't actually need that for the 'My account' link itself. Not sure if there's a way around that but it's the most expensive (3-4 database queries, about 6% of simple pages).
Comment #9
sunsubscribing
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe
Comment #11
pwolanin CreditAttribution: pwolanin commentedI'll work on a totally minimal patch - potentially suitable for D6 backport - to just address that one link issue.
Also, there is already a critical bug here that's separate - the user_menu now specifies user_build as the callback, but does NOT specify the second argument to that function. Of course, function user_user_view($account) doesn't even have a paramer for the build mode...
Who committed this broken code to user module?
Comment #12
pwolanin CreditAttribution: pwolanin commentedHere's a patch. Still, seems like viewing a node causes (for no good reason?) a user_load on the author. Maybe RDF? So it's very hard to find a page where user_load() is not running for some user. I'm not sure where that user_load is coming from though?
This patch does mean that for a user viewing a node page or the front page, user_load() is not called for their account by user module.
Comment #14
catchRDF user_load() issues are here #636992: Entity loading needs protection from infinite recursion.
But yeah this will help when the authenticated user isn't the same as the author of the node or a comment (extremely common), or if RDF isn't enabled, or on views / panels etc.
Comment #15
ksenzeeRunning this one past the bot. It removes blog and tracker modules' dependency on user_uid_optional_load(). I think.
Comment #17
ksenzeeDuh, the access functions were relying on $account too. This should take care of the blog.module failures. Still working on tracker.
I have to admit I'm not sure when it makes sense to use %user_uid_optional. What about blog/%user/feed? I left it as %user and gave it its own access function that takes an $account parameter, just because blog/%user_uid_optional/feed didn't make any sense to me (obviously it's not optional in a path like that). But maybe I'm reading too much into the "optional" part...
Comment #19
carlos8f CreditAttribution: carlos8f commentedsubscribe
Comment #20
ksenzeeTests pass locally with this one. I stuck %user_uid_optional in the middle of the blog and tracker router item paths after noticing that Peter did it with user module. :)
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedI'm not too happy with the direction of recent patches here. Up through #9, we wanted to smarten the access check in menu system to only load $account when needed. Thats the right place for the complexity IMO. Most recent patch pushes user_load() calls all over the place, which complexifies access callbacks and page callbacks. We're gonna end up with similar complexity for callbacks related to nodes and terms and comments.
Comment #22
pwolanin CreditAttribution: pwolanin commented@Moshe - the account object is loaded before the access check ever happens. That's way I took this approach as the simplest change to improve performance for the one problematic case (versus a more sweeping API change). See comments in #6. Up though #9 none of the proposed changes would actually have addressed the performance problem.
D7 also has a static cache for this object - so if we have to call it once the added calls should have little effect. Yes, this patch makes the code a bit uglier, and is not a pattern I'd generally suggest.
If we want to make this a more sweeping API change - possibly we need to somehow allow the access callback to take the name of the load functions and all arguments? The access callback could then implement logic to only load objects as needed?
Comment #23
Dries CreditAttribution: Dries commented@Moshe, it would be best to prototype what you have in mind (assuming you have something in mind). Simpler code would win over more complex code, but not until the simpler code emerges. It is unclear how we'd achieve what you have in mind.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedRecently, we have been working on avoiding a user_load() on a page just for the My Account link. I actually think thats not desireable. We *should* be loading a full $user object for the current user at the end of the bootstrap. By not doing so, many many functions which later use the object have to call user_load() to make sure that they are working with a fully loaded user object. I dislike passing around partial user object. Its time to end this premature optimization. #490108: user_load() should add session properties when loading the currently logged-in user and #361471: Global $user object should be a complete entity are just two examples of bugs that our optimization causes.
This issue about more than just avoiding the user_load(), but I'm a bit fuzzy on those aspects. Anyway, lets discuss and if folks agree with me, we can add the full user_load() at #361471: Global $user object should be a complete entity.
Comment #25
sunI agree with that suggestion and kick-started discussion in #361471-10: Global $user object should be a complete entity
However, even with that change being in place, I also feel a bit uneasy about the proposed changes here. My main consideration and question probably is: If we do this for %user, then we ought to do it for all dynamic menu arguments...?
Comment #26
pwolanin CreditAttribution: pwolanin commented@sun - This link is a bit special since it's a dynamic path that we are showing in the menu for all users by default.
Still - I think we should seriously consider (for D8) more of an on-demand loading for page or access callbacks. If I'm going to deny you access since you don't have 'access content' permission, I really never need the $node.
Comment #27
carlos8f CreditAttribution: carlos8f commentedSeems like we've got a bigger issue of whether user_load() for the current user is necessary for every request or not. Patches 12-20 wouldn't be so useful if user_load() ends up being part of the bootstrap process :) That would unfortunately be a 6% of the page load we can't get rid of.
Personally, I'm a fan of 'lazy loading', as long as it's done properly. *Partially* loading an object could be inviting trouble, as we've seen with $user. In D8 however, it could be split into $user (just the basics from bootstrap) and $user->account (full object after hooks run), so lazy-loading would be more manageable.
As it currently stands, I'm kind of +1 on a mandatory user_load() in D7. It probably won't be worse for performance, since user_load() is called anyway in some place or another. This would just make a full $user more universally available during the request.
Does this bring us back to adding caching to menu_load()? Or is there more to think about regarding some 'skip loader for access callback in some situations' logic?
Comment #28
sunSo what we are doing here is to move (IMHO essential) logic away from the menu router to speed up menu links.
I guess that's the part of this patch, which I don't like. The menu router is consulted when I try to access path/%foo, and when I try to access this router path, then the entire page request shouldn't get as far as to the page callback in the first place. Moving this essential access and lookup logic into the page callback function basically is a step backwards to D5 or 4.7.
I wonder whether we can find (or perhaps even introduce) a difference between access to the active/current menu router path and figuring out access to display a menu link.
A big part of this picture is probably that we don't have entity caching in core yet. Hence, any dynamic argument like %user leads to a heavy stack of loader functions to serve the requested argument. I can only guess that this would (mostly) decrease to a single cache lookup with entity caching - even for %user.
However, pwolanin is also right in that there is zero point in loading any dynamic argument (object), in case the current user has no access (permission) to "work" with that object - based on the menu link in question. This is something that is very worth to explore. And given that we are still horribly slower than D6, this is something we absolutely need to investigate.
Comment #29
carlos8f CreditAttribution: carlos8f commentedI understand that the goal pwolanin's patch is to use properties in $user in place of doing a full menu loader on the current user's uid. However, after looking closely at the patches starting with #12, there's something really flawed :)
The purpose of user_uid_optional_load() is that it loads the current user if not passed an argument. This change makes no effect since the uid is passed here anyway.
By removing this loader, you're invalidating the change made above to use %user_uid_optional for menu items. Instead, the loader should be removed from the paths, i.e.
user/%/view
.I can understand that the 'shortcut' here might optimize the 'My account' link, but only if user_load($user->uid) is never called by anything else during the request. Have we seen that to normally be the case? If not, all these changes are just deferring the user_load() call. This is also moot if user_load() is to be done after every bootstrap like moshe suggests.
I'm on crack. Are you, too?
Comment #30
carlos8f CreditAttribution: carlos8f commentedCross post. I still think this needs work though.
Comment #31
carlos8f CreditAttribution: carlos8f commentedAgreed with @sun on #28. This seems like a regression and is actually the domain of entity caching. (what's the status on that?)
Comment #32
catchOK so entity caching Dries has just said he'd like to explore it more, I need to find time to turn the contrib module into a proper core patch, which I'm hoping to do tomorrow or Monday, but can't guarantee. However entity caching for users in core D7 is pretty much a no-go - since we load things like $user->last_access, session data and tonnes of other stuff which can change whenever. That'd be a huge patch.
In general, while I agree it'd be nice not to have global $user be two random things, it's worth remembering that in D6 there's no static caching for user_load() - which means having it called in various places is a real waste, but we do static cache it in 7, so it's not really a premature optimization to the extent it was in D6 where it could actually be harmful.
For the more general issue of loaders - chx had some draft code which didn't yet make it to the issue, which had two ideas to it.
1. Allow 'access callback' to be an array with either an OR or AND operator
2. Allow the array keys of that array to state if they need the router loader loaded or not.
This would be an API addition, but transparent from HEAD since we'd just do is_array().
For the user situation, you'd then break the current access callback into three - one checks if you're viewing your own account and are logged in, the second checks if you're able to view profiles or not, the third would load the $account object from the loader and use the information from that, if any pass, the rest don't get called.
This would mean some more complex requirements in hook_menu() for those paths, and more complexity in menu.inc, but it'd keep everything encapsulated, and for the contextual links / menu_load() issue it'd work great since we only want user_access() for that anyway.
Comment #33
carlos8f CreditAttribution: carlos8f commentedI need to do my homework on the entity caching issues, but I could see how it wouldn't apply to users so well without 'patching' some of the properties dynamically. It would be messy, probably not worth it.
Regarding $user 'premature optimization', in D6 it was, for lack of a better word, a disaster :) With static caching we're now in the safe zone, and with entity caching we'd be trading some bugginess or loss of functionality for maximum efficiency. Until we can prove that user_load() is necessary on each page load, let's work with an incomplete $user for now. Many times all we need is $user->name, $user->uid, $user->roles, etc.
I like chx's direction, since it transparently introduces an optimization feature into the menu system, it's probably the most reasonable solution so far. I'll see if I can come up with some code independently to accomplish this without getting too complex.
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commented@catch, @chx - can we get a patch here which implements the menu changes described in #32?
Comment #35
carlos8f CreditAttribution: carlos8f commentedFor menu links, our current approach of unconditionally running _menu_load_objects() (even if there is an eventual denial of access) and using full objects for the title callback (when all we need is a title) is excessive. It makes sense for a page callback, but not for menu links. Ideally, access and title callbacks, at least when used in a menu link context should take a raw argument (such as uid), and determine if they need to do the object loading or not to get the access result or title. I think this sort of lazy-loading convention would be nice for D8, but in D7 we have some specific cases that need help, like the My Account link.
Here I've implemented a small-scale demo of this concept. My patch adds a flag available to menu items called 'skip_loader_for_link' (working title, of course). Access and title callbacks for these menu items consequently need to be capable of taking a raw argument in a menu link context, and a full object in a page context.
I don't really support multiple type possibilities in function arguments, but that seems like the easiest thing to do post-API-freeze along with an opt-in flag for that. Otherwise we'd be requiring all access callbacks to go without loaders (obviously too late) or having some convoluted system of multiple access callbacks. I think the former should be considered for D8 performance, since as @pwolanin points out, if you don't have 'access content' permission, $node goes to waste.
I benchmarked this on my (admittedly underpowered) macbook, and it did make a positive improvement. I hit the front page with a minimal install (block, menu, file, image, dblog, devel, one node with an image) logged in as user #2.
before patch:
After patch:
This shows a ~8% performance increase on the front page, where user_load() was skipped since it's unnecessary to check access or generate the 'My account' title.
I also benchmarked user/2, which had a .8% performance drop, expected since additional user_load() calls are made, which are statically cached.
Comment #36
carlos8f CreditAttribution: carlos8f commentedI've postponed #654032: Regression: 'My account' link can no longer be displayed since the patch in #35 here fixes the problem more elegantly by implementing separate logic for link/page contexts in the title callback. That allows the link to show up as 'My account' while the page it links to is titled 'USERNAME' via the same title callback.
Comment #37
pwolanin CreditAttribution: pwolanin commented#35 is probably similar is effect to what I prposed, but has a schema change that I'm not totally fond of. If we are going to go with a schema change, almost seems like we should more generally allow links to have distinct access (and other?) callbacks?
Comment #38
sunSo it seems like we can achieve heavy performance gains by dealing with the plain arguments first, and only load the arguments when we really need them.
Re-phrase that into: Heavy performance gains if we deal with plain arguments in access callbacks, and only replace the arguments in the map when actually needed.
This is not only true for %user, but also for stuff like %node. We don't need to load $node, if the current user does not have the "access content" permission. Same applies to %comment, etc.
Possible conclusion: Can we change access callbacks to be invoked with original arguments, and allow them to update the argument map in case the user has access and they load something? I.e. so that most of the code for title and page callbacks stays the same.
Comment #39
catchI can't find the pastebin chx did for this, and not sure if there was an associated menu.inc patch with it, but here's a very idea of the syntax - was over a week ago and I don't think this is exact. If we can get it working, then I think it's the best bet.
And then we serialize access callback when it's an array. This makes hook_menu() a bit more complex, but all the individual access callbacks only have to worry about one thing.
Comment #40
carlos8f CreditAttribution: carlos8f commented@pwolanin
I think the performance increase is worth a schema change, and the API change is minimal and not intrusive on development. If a schema change must be avoided, the flag could be hidden in the 'options' array, but that isn't good since 'options' is intended to affect the l() function.
Allowing links to have their own callbacks would probably be a major API change, I think too late for D7. I agree, defining a 'menu link access callback' would avoid the $arg problem, but might cause duplication of code, etc.
@sun
I'm for access and title callbacks always running with plain arguments, as long as there is static caching of *_load() functions. I think that needs to be D8, though.
Comment #41
Dries CreditAttribution: Dries commentedJust want to encourage all of you to keep moving this forward. A 6-8% performance gain seems big enough to break some APIs for prior to the first alpha release. I think sun said it best in #38 -- or even shorter; "our lazy loading is broken".
Comment #42
Dries CreditAttribution: Dries commentedTo be more clear -- when choosing between two approaches, I'd go with the bigger API change if there is a performance gain of more than 5-6%. 5-6% is significant and can be justify an API change at this stage. Shortly, we'll roll a first alpha at which point the rules will change.
Comment #43
carlos8f CreditAttribution: carlos8f commentedI think the biggest long-term improvement would be to only use menu loading for page callbacks. That is where it is unconditionally needed, and everywhere else it depends on the situation. The callbacks can use *_load() functions if really necessary, with static caching to fall back on. It's better for long-term performance if we enforce this, rather than it being only a per-case optimization on the My account link, etc.
I also dislike the prototyped syntax in #39. Mixing 'key=>value' with 'value' in an array is weird and should be avoided. Using the key as a flag is also weird. And writing 3+ functions to do one access check is heavy and won't encourage developers to use this syntax :)
Comment #44
chx CreditAttribution: chx commentedThis is what catch is talkin' about.
Comment #45
chx CreditAttribution: chx commentedOn IRC carlos8f suggested dropping the early loading and only doing loading for the page callback. This would avoid all loads for access and title callbacks. His argument -- if some rare function needs loading , most loads are static cached now -- is fairly solid. This is massive work but if he can pull it off, I am glad.
Comment #46
sun1) Note that, at some point, we also need to support class methods as access callbacks, so 'access callback' needs to support an array (which may clash with the last proposed patch).
2) Not sure whether removing the argument loading altogether (except for page callbacks) works. We have menu routers like
http://api.drupal.org/api/function/node_access/7
http://api.drupal.org/api/function/node_page_title/7
http://api.drupal.org/api/function/node_page_view/7
http://api.drupal.org/api/function/user_view_access/7
http://api.drupal.org/api/function/user_page_title/7
http://api.drupal.org/api/function/user_view/7
Both access callbacks seem to require the loaded object at some point (when simple access checks passed). Both title callbacks are only invoked when the user has access. Same applies to page callbacks. If we do not update the $map (http://api.drupal.org/api/function/_menu_check_access/7), then we basically require all code to "leverage" static caching.
Comment #47
carlos8f CreditAttribution: carlos8f commentedI'm starting to do some work on what chx mentioned in #45. It's obviously a very big task, with 30 custom access callbacks to deal with and many tests to write or alter. I'm fairly certain there will be tangible performance benefits from this, (as we've seen from the 'My account' link) and it'll be a good application of lazy loading philosophy. If certain menu items absolutely need their objects pre-loaded, perhaps we can add a menu flag to specify that.
Comment #48
int CreditAttribution: int commentedsubscribe
Comment #49
carlos8f CreditAttribution: carlos8f commentedHere's some preliminary work, involving moving _menu_load_objects() calls out of _menu_translate()/_menu_link_translate() and into menu_execute_active_handler() instead. I modified user_access(), node_access(), and node_access_grants() to alternately take uid/nid in place of $account/$node. In certain places I create a skeleton $account or $node to avoid a full *_load() call, but the skeleton is never passed to another function, since that would break consistency.
The patch seems to be at least basically functional, although Garland replaces Seven as the admin theme, for some reason.
Comment #50
carlos8f CreditAttribution: carlos8f commentedI found the theme issue to be due to menu_get_custom_theme() being called during bootstrap, before I was adding 'theme arguments' in menu_execute_active_handler(). This patch translates 'theme arguments' without loaded objects, during _menu_translate() inside menu_get_custom_theme()/_drupal_bootstrap_full(). Seven is back.
Now to address some custom access/title callbacks.
Comment #51
carlos8f CreditAttribution: carlos8f commentedRight now I'm curious what approach will be more effective: (inside an access callback)
The first just gets the minimal data to do an access check, whereas the second anticipates the $comment will get used later on for rendering, assuming static caching does its job. I'm going with the first strategy for now, and will explore the second later to see if there's an overall improvement.
Comment #52
catchJust a note that comment_load() and menu_load() aren't static cached.
Comment #53
carlos8f CreditAttribution: carlos8f commentedcatch: any reason for that?
Comment #54
carlos8f CreditAttribution: carlos8f commentedI imagine in the case of comments static caching would cause memory bloat. And caching menu_load() was the original purpose of this issue. :)
Comment #55
catchFor comments, they're very rarely loaded more than once on a page request, and we allow for loading up to 300 at a time in core. So for that case, it saves a bit of memory. We had an issue recently where contextual links was added for comments, and this ended up calling comment_load() 3-4 times per comment due to the access check, but that was rolled back.
menu_load() there's no particular reason in either direction afaik - although that's also just a direct query.
Comment #56
pwolanin CreditAttribution: pwolanin commented@carlos8f - I think this issue is heading in generally the right direction, but I'm opposed to making functions with a variable signature - they should take either an ID or an object, but trying to detect what's passed in seems like a pattern to be removed not expanded.
Comment #57
carlos8f CreditAttribution: carlos8f commentedpwolanin, I consulted with chx on the issue of variable signatures, and we decided it's a necessity on node_access() to accept both, to avoid breaking stuff. I'm doing the same thing with user_access(), to be consistent and since i've found plenty of cases that pass $account there. I may end up abstracting that detection logic, but I can't really tell yet. All the minor access callbacks i'm changing to use a raw argument only, since no one is really fond of having all these functions take multiple argument types.
Comment #58
catchOne possibility, though it might not be any better, would be to add wrapper functions around node_access() and user_access(), have these take the variable argument, then pass on to node_access() or user_access() if the object needs to be loaded. That'd restrict it to just the menu access callbacks, since those two are called in other places too.
Comment #59
carlos8f CreditAttribution: carlos8f commentedcatch, interesting suggestion. I will think on that.
This patch should take care of the access callback changes. Now for title callbacks and tests.
Comment #60
catchOne other reason to do it like that - there's a reasonable PHP overhead even just fetching objects from static cache, see http://drupal.org/node/470398 - and similar issues (we were calling node_load() up to 900 times for the same node on comment listings up until a month or so back), so the more we can enforce that people pass objects around apart from extra special cases like this the better.
Comment #61
carlos8f CreditAttribution: carlos8f commentedcatch, I think I see what you're talking about, but my reworked versions of user_access() and node_access() don't use *_load() at all--at most, one query substitutes for that. node_access() could indirectly call node_load() though, if a module calls it in hook_node_access(). Also, since the determination of whether an object needs to be loaded is situational and very procedural, I think it would be difficult to separate the logic into a wrapper function. In that case, we might need a near-duplicate function that takes a raw argument for menu callback usage.
Let's keep the idea in mind though, thanks.
Comment #62
chx CreditAttribution: chx commentedSee http://drupal.org/node/310077 on getmetadata and friends.
Comment #63
carlos8f CreditAttribution: carlos8f commentedI think the access and title callbacks are covered now; giving this a test run. Still have to address some bugs in menu.inc (undefined offset on line 717 and theme callback inheritance were some I saw).
In covering the title callbacks I came across a new feature that I implemented in the patch:
Sometimes it might be useful for access/title callbacks to know what context they are called in, since _menu_translate() is called by local tasks, contextual links, and menu tree, in addition to menu_get_item(), which may be called anywhere.
A use case I have in mind is the 'My account' link in core. The title callback doesn't know whether it is generating a link title or a page title, therefore they can't be different without a drupal_set_title() in the page callback, which introduces inconsistency, especially with local tasks. I tried to solve this problem with drupal_set_title() in #654032: Regression: 'My account' link can no longer be displayed, but this context idea would do it much more gracefully.
Access and title callbacks can take a dynamic argument %context, being one of 'page', 'link', 'local_task', 'contextual_link', or NULL if menu_get_item() is called on an arbitrary path. That allows user_page_title() to simply take a $context argument and serve different link and page titles. This approach could also theoretically work to hide certain menu links, local tasks, or contextual links based on toggles or other non-permission checks.
Comment #68
carlos8f CreditAttribution: carlos8f commentedImproved menu.inc, now experimenting with loading objects in menu_get_item(), instead of menu_execute_active_handler(). I think this is a much more solid approach, since now menu_get_object() works more reliably, and you can be assured that the results of menu_get_item() will have loaded objects as long as 'access' => TRUE.
Comment #70
carlos8f CreditAttribution: carlos8f commentedFixed lots of stuff. One problem was that 403's were being returned instead of 404's, since 404's are normally triggered by menu loaders returning FALSE, and menu loaders are skipped now if access is false. To remedy that, access callbacks can now return MENU_NOT_FOUND if they can't find the object to do an access check. That introduces problems in API functions such as node_access() which are also used as access callbacks. I created a menu-specific version of node_access(), _node_menu_access(), which returns MENU_NOT_FOUND if the item couldn't be found, and also takes raw arguments instead of full objects.
Curious what the test bot has to say. The patch is getting close. I was getting a weird bug running tests locally though, where my test environment was returning TRUE for drupal_multilingual() even though the {languages} table or locale wasn't installed. That led to user_save() not working, which led to lots of tests failing. Hmm.
Comment #71
David_Rothstein CreditAttribution: David_Rothstein commentedHm, not sure about this... I have to say that Moshe's concern in #21 seems to be coming exactly true here :) This is making callbacks a fair amount more complex, which is something that would be nice to avoid.
To me that approach looks very promising in the long run - it seems more flexible and pluggable. I agree that the
'#op' => 'or'
part isn't good, but I think we can get rid of that (for example, by moving to a three-valued return system exactly like http://api.drupal.org/api/function/hook_node_access/7 does it).Which would lead to an access callback function with the following signature:
(And this approach would ideally work for all menu system callbacks, not just access callbacks.)
Comment #72
carlos8f CreditAttribution: carlos8f commentedDavid_Rothstein,
My original thought was to make lazy loading opt-in for D7, and default with possible opt-out for D8. Dries in #42 and chx in #45 inspired me to give it a try now before alpha is released. The extra complexity would be a trade-off for better all-around performance.
Comment #73
carlos8f CreditAttribution: carlos8f commentedI benchmarked the current patch, and overall it was disappointing. My results were fluctuating with the default profile, but after uninstalling all modules except menu and devel performance, I saw a slight net slowdown versus HEAD. I think I overestimated the number of objects being loaded on a given page.
The two object loads I traced that were expendable were user_uid_optional_load() (from 'My account') and menu_load() (called 3x by contextual links for the same menu, not cached), which brings us back to our original optimizations. Something like #44 might be good, but the user menu might even be optimized further by making it more static. Basically all the menu is:
These links never really change. It's more expandable as a menu, but slows down every page load.
Comment #74
catchYeah we only load items defined with %loader - anything with % by itself doesn't get the router loader called, Those menu_load() and the user_uid_optional() are te main ones with HEAD.
There's actually an issue open for the user menu (which is currently secondary links) at #408142: Create a 'user links' menu + page template variable. Converting it into a page variable and not using the menu system would be a lot faster, and you could still theme it / hook_page_alter(). That issue is marked critical because we never intended to leave it as the default secondary menu - since this runs the risk of messing up a lot of theme upgrades. It'd then just be an optional addition to themes rather than an API change. But on the other hand it'd be nice to be able to solve this generically within the menu system too (for things like menu_load()) - I'm fairly concerned that if contextual links get used a lot, we'll see more of this in contrib).
Comment #77
carlos8f CreditAttribution: carlos8f commentedSo back to square one, I wrote a cache for menu_load() and menu_get_menus(). The other half of the issue is the user menu, but let's pursue #408142: Create a 'user links' menu + page template variable for that.
Comment #78
catchJust a note that #606608: Use proper menu router paths for comment/* is similarly in bad shape at least partly due to this issue. It's not in the critical path so much, but comment_load() eight times is a lot.
At this stage, while the user links variable would be completely optional, it might be considered more of an API change (if it's applied to Seven and Garland) than a backwards compatible change to hook_menu(), so I'm not sure about #408142: Create a 'user links' menu + page template variable for fixing this. Since we're only talking about adding to the API now rather than changing it as such, and 6% is a lot, this might sneak in past alpha too, if the patch was sufficiently tidy. However while there's a few options here which seem viable, none of them really stick out yet.
On the patch itself, there shouldn't be any need to initialize drupal_static() to NULL - can just use no default then check with isset(). Otherwise looks good though.
Comment #79
sunCan we name this menu_load_all() or similar?
Otherwise, we'll have:
menu_custom_menus()
menu_get_menus()
menu_load()
...which is... quite confusing.
As catch mentioned, we can remove the NULL here.
!isset()
I think this should use {cache_menu}
Single quotes.
Appending a drupal_alter() here would make sense, I think.
Please re-roll with diff -up, because we don't see the context of changes.
When using {cache_menu}, those are probably no longer required.
Powered by Dreditor.
Comment #80
catchThe rest of Sun's review looks good, but I disagree with "Appending a drupal_alter() here would make sense, I think.", whether it does or not, new issue for that please.
Comment #81
Dries CreditAttribution: Dries commentedI'm willing to make API changes prior to alpha if they bring significant performance improvements, where significant is (probably) 5% or more.
Comment #82
catchI thought about this some more, and realised that even if we could get the user links variable patch, this only helps if you never need to put that link in a menu for some other reason. So I decided to get #44 working, which seems the only viable option for a generic solution. This now passes all user tests, so ought to be fairly solid, and properly removes the user_load() from the default front page.
Leaving CNR for both the bot and some reviews, although there's more cleanup to do here before it's committable, as well as applying the pattern to menu and any other pain points. The main thing we need is a constant instead of a random string to indicate whether to load objects or not, and a lot more comments.
I don't have much time until Tuesday to work on any of this, so re-rolls as welcome as reviews, but I'll do my best to keep up with it where possible.
Comment #84
sunI agree.
Some thoughts:
- Multi-purposing the same item properties 'access callback' and 'access arguments' is very ugly. Ideally, we either introduce a new property 'access callbacks' that is processed during router building, or, we just keep the current properties and use them like they have been designed.
- Splitting both properties and ensuring the proper + matching order of values in both arrays is very fragile. Ideally, this info should be kept together.
- Serializing an array of functions into 'access callback', ugh.
To keep it short and to the point: how about this:
Or similar. Thoughts?
5 days to code freeze. Better review yourself.
Comment #85
Dries CreditAttribution: Dries commentedSorry, haven't ready up on the entire issue, but why do we need these OR and AND constructs? Both proposed solutions in #82 or #84 are a bit of a WTF to me.
Comment #86
catchWhile that looks a bit nicer, it will require schema changes to store properly. That means two things - 1. we have to make a schema change < 5 days before alpha 2. We have the overhead of an extra column in the {menu_router} table which will currently only be used by one router item. That really looks like more of a D8 thing to me.
Here's some rough performance data.
HEAD:
Executed 39 queries in 35.99 milliseconds.
Patch:
Executed 33 queries in 25.66 milliseconds.
Also attached kcachegrind screenshots which show a 4.2% shaved off just from menu access. We save a user_load(), a file_load(), (because user_load() calls file_load()), and due to this, also save initializing the entity and field systems altogether. The savings will obviously be more if you have contrib modules implementing hook_user_load().
However, whilst preparing this, I noticed that the menu_load() calls aren't only from the access check, but are also from menu_get_item() as called by contextual links. That means this patch will work for user_load(), and possibly for the comment tabs, but won't have a practical different on menu_load() unless you happened to be viewing a link to a menu with contextual links disabled. So we may need both this, and caching of menu_load() as in carlos8f's patch, to fix all the issues identified here.
Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedAs I said above in #71....
Then all the complicated AND/OR logic can remain inside the (single) access callback like always, and it simply chooses to arrange its internal logic to delay the call to user_load($uid) as late as it possibly is able to. No?
Comment #88
carlos8f CreditAttribution: carlos8f commentedI agree with David and Dries, the menu item is much cleaner when AND/OR logic is not shoved into it. I would support a simple way of opting out of menu loading, like specifying
'access argument mode' => MENU_NO_LOAD, 'title argument mode' => MENU_LOAD
etc.I'll work on the menu_load() cache (#77) in the mean time.
Comment #89
carlos8f CreditAttribution: carlos8f commentedThis should address everything in #79 and #80.
Comment #90
catchI think David's idea will work. I'd still want to use the array key to avoid schema changes/bloat, but that ought to be more straightforward than the current patch.
Tested #89 - we never fetch from the menu_load_all() static even if it's set. Wondering if we should split this into two sub issues and keep this for meta.
Comment #91
carlos8f CreditAttribution: carlos8f commentedcatch, I'd like to go with the cleanest solution even if it requires schema changes. Some of these proposals look like glorified hacks :-/
re: static in menu_load_all(), you'll have to elaborate. I'm pretty sure the static was working in #77, and the function is getting called 3x per page load. I'll test it out and follow up if it's broken.
I'm ok with splitting the issue, whichever is most productive. The menu_load() cache part is almost ready.
Comment #92
carlos8f CreditAttribution: carlos8f commentedcatch, I inserted some debug statements in menu_load_all() using patch #89 and got this for the front page:
string(7) "from db" string(11) "from static" string(11) "from static"
and for the second page load,
string(10) "from cache" string(11) "from static" string(11) "from static"
which indicates that the static and db caching are working there. How did you do your testing?
Comment #93
sunNote that I took a large step back in #84 - what do we really want? a) Don't break existing APIs. b) Don't load dynamic menu arguments unless necessary. c) Keep all existing page and title callback arguments (keep APIs). d) Don't introduce any weird updating of statically cached menu $map. e) Support both OR'ed and AND'ed access conditions.
I'm not sure about which schema changes was commented on, but we already have an 'access callback' string (defaulting to an already special-cased 'user_access') and a serialized 'access arguments' array. Of course, the syntax to specify AND/OR-access conditions is highly debatable.
Another (perhaps special-cased, if required) access callback? Why not?
Comment #94
catchSpoke to chx in irc. After going round on a few different options, we think this is good:
Stored in a new tinyint in the router table, this should only affect menu_link_translate() more or less, and won't impact any existing APIs at all. If you specify this, then your access and title callbacks need to be able to accept either an int or an object.
Comment #95
catchAnd a patch. User tests pass, didn't run any others though. Performance numbers should be more or less identical to #86.
Comment #96
carlos8f CreditAttribution: carlos8f commentedcatch, this looks eerily familiar. I implemented the idea in #94 30 days ago, in #35! :)
Edit: 20 days ago actually.
Comment #97
chx CreditAttribution: chx commentedLOL! The only problem there then was schema change. BAH! We can't come up with anything better, #35 contains benchmarks, FIRE.
Comment #98
carlos8f CreditAttribution: carlos8f commented#89 is probably RTBC as well, should we merge them?
Comment #99
pwolanin CreditAttribution: pwolanin commented@catch : this sounds wrong: "your access and title callbacks need to be able to accept either an int or an object. "
do you mean that your get the path component? e.g. if the access argument is array(1) for node/123 you get int 123, but for foo/bar you get 'bar'.
Comment #100
chx CreditAttribution: chx commentedpwolanin, no, they will get either 123 or node_load(123)
Comment #101
carlos8f CreditAttribution: carlos8f commentedpwolanin, it's not actually an int v.s object, it's the plain argument v.s. the load result. so yes, it could be a string.
Comment #102
webchickI don't want to really stick my fingers in here too much, because Dries looks like he has this handled, which is totally cool.
However, chx asked me to take a look. I haven't read the issue, just looked at the patch, since that's what everyone who's not in this issue will have to go on. It's not obvious to me at all what skip_load_for_links is for, and the schema description doesn't help me, nor is it documented in hook_menu() docs in menu.api.php (bad monkey! no banana! ;)).
I imagine other modules are going to need to flag this, even if user module is the only place in core. It needs to be obvious to them what this does, and when to use it.
Please add a nice description for this property in the hook_menu() docs that outlines user_menu()'s use case.
Comment #103
carlos8f CreditAttribution: carlos8f commentedAdded better docs.
Comment #104
catchSince this is actually carlos8f's patch which apparently I just re-rolled (doh!), and the extra docs look great, RTBC again. (edit: and sorry for insisting on a schema change, this is so much simpler than the other options it justifies it completely).
Comment #105
carlos8f CreditAttribution: carlos8f commentedKeep in mind we actually have 2 separate patches in this issue: #103 which adds menu link optimization (RTBC), and #89 which adds a menu_load() cache, as recommended in the original post. We could review #89 and roll it together with #103, or commit them separately, but they are both valuable.
Comment #106
catchIMO #89 is RTBC too, we can open a minor followup issue for D8 to decouple the static cache clearing, or it could hopefully be solved by something like #636454: Cache tag support, but it's pretty harmless as things go compared to the gains, and there's no way to do it cleaner unless we add a hook_menu_rebuild() or something.
Comment #107
carlos8f CreditAttribution: carlos8f commentedRolling the two together, RTBC per #104 and #106: menu link optimization flag + menu_load() cache.
Comment #108
carlos8f CreditAttribution: carlos8f commentedSo #107 is the culmination of this issue, reviewed and tested. Phew, let's move on :)
Comment #109
Gábor HojtsyWhy do we complicate things here by "skip load for links", when we can actually pretty easily achieve the same by not specifying the object type to load (ie. have "user/%" instead of "user/%user_uid_optional", or if we truly need the optionality handling in the loader, add a "%uid_optional" which would not load the user object). It will have the convenience to not require us to remember that access and title callbacks do not get the object loaded while page callbacks do have. It will also make access and title callbacks less code by not having these conditionals, we'll have cleaner signatures and no schema changes.
Why is it so important to tell the menu system we have a user to load there, if we also tell the menu system to mostly ignore this fact? I'm not clear on this one.
Comment #110
chx CreditAttribution: chx commented_to_arg
Comment #111
Gábor Hojtsy@chx: You mean http://api.drupal.org/api/function/user_uid_optional_to_arg/7? That does not seem to require a fully loaded object either. So no reason not to have a %uid_optional instead of a %user_uid_optional with the former not doing a user_load() as far as I see. Maybe if you can elaborate a bit more on this, so we'll not continue this ping-pong.
Comment #112
catchThere's a patch for that at #20 however it's about the same patch size, and spread out all over the shop, whereas #107 limits changes to menu.inc and a couple of user.module functions. #20 also has page callbacks returning MENU_NOT_FOUND themselves, more $GLOBALS['user'] etc. so I'm not sure it's any cleaner either. By removing user_uid_optional_load(), we'd also have an API change for modules which rely on that, whereas the current patch is just an API addition so won't affect upgrading.
I don't think #107 is perfect, but it's the best we've come up with (IMO including #20), very viable, and a lot better than the situation in HEAD.
edit: cross-posted with chx and Gabor. Adding a new %uid_optional would remove the API change here, but the rest still stands unless we see a super-clean patch using it.
Comment #113
Gábor Hojtsy#20 is big and scattered because it changes blog, tracker, etc. paths and callbacks as well, which #107 does not do at all. If we consider changing the single menu item as in #107 is enough, applying a non-user loading version looks simple. Also, I'm not saying we'd remove user_uid_optional_load, I'm saying we'd add uid_optional_load or user_uid_only_load or somesuch to be in the user module namespace.
I think adding a hack like "oh, I have an object type defined but most of the time just ignore that" to the menu system instead of skipping to use an object type or define an object type which takes less resources to load. With this "skip load for links", we kick out some of the loading code, which is crufty and looks confusing on the API level (sometimes you get the full object, sometimes not), so I'm for using the argument object types for what they were designed for instead.
Looking into creating a patch which would equal #107 in functionality but without the menu API cruft.
Comment #114
Gábor HojtsyThe attached patch takes all the menu.module changes from #107 intact (I did not even review this part of the patch). It also takes all the changes from #107 for user_view_access() intact (but it adds an extra comment on top about the dual nature of $account there). In testing, I found that user_view_access() is called elsewhere with an $account so we cannot make it plain dependent on a $uid only.
To achieve my main goal however, I've introduced a %user_uid_only_optional, which as its name says only loads the uid and makes it optional (versus %user_uid_optional which also loads the user). This is simply achieved by reusing %user_uid_optional's to_arg but not specifying a loader function for %user_uid_only_optional at all. So we'll get the uid only and it will default to the current user. This is what we were about to achieve in #107, but that introduces workarounds in the menu system to achieve this instead of just having a uid loader.
The title callback is now with a $uid only and the page callback needed a wrapper to convert the $uid to the $account. So in all cases, we postpone lazy loading the user as much as possible.
I've also added crucial comments to the menu item to tell we do this for performance reasons, and attempted to fix the description of user_uid_optional_to_arg() once and for all :) Also found out that this menu item has nothing to do with user.pages.inc, so removed that file reference as well.
Now I believe this does everything that #107 did and does in a cleaner way. In manual testing, it all turned out to be fine, so I'm interested in the bot and reviewers. It certainly is way smaller then #107 :)
Comment #115
Gábor HojtsyBTW for API cleanliness, I'd rather have %user_uid_optional actually only deal with a "uid" and have a %user_optional which also loads the user. So we'd keep having a clean "user" argument type and a newly identified "user_uid" argument type from the menu system's POV. I've kept %user_uid_optional as-is in #114 in the interest of backwards compatibility (avoid to make an API change this late for this).
Comment #116
catchKeeping _to_arg() without the loader is nice. Feels like we might want to rename user_uid_optional and friends in D8, but with zero API change this is a lot nicer than the new hook_menu() key and as self-contained everywhere else. Gets +1 for me, leaving CNR for chx and/or pwolanin to have a crack at it (and the test bot).
edit: cross-posted again, exactly
Comment #118
Gábor HojtsyThe tests failed due to a one line change not carried over in menu.inc, namely to clear the static cache of menu_load_all() as well when clearing menu statics.
Also discussed the access callback with chx and simplified it a bit.
Comment #119
sunCould we rename this into
%user_uid_lazy
or similar? Given that %user_uid_optional already makes zero sense when reading it and you have to look up what those callbacks do exactly, it would be great if we wouldn't add another dynamic argument that only marginally differs from %user_uid_optional and is not self-explanatory.
1 days to code freeze. Better review yourself.
Comment #120
catchI'm not sure about _lazy - it doesn't actually do any lazy loading for you. I'd rather a @todo to rename them both for D8 at this point.
Comment #121
pwolanin CreditAttribution: pwolanin commentedIndeed - lazy doesn't make sense to me. The 'optional' relates to the to_arg function.
Comment #122
Gábor HojtsyYeah, its not doing anything lazy for you. I totally agree %user_uid_optional is a crazy name though. Updated patch now with simplified access callback (in cooperation with "tha chx" :).
Comment #123
pwolanin CreditAttribution: pwolanin commentedLooks like user_load is still called a couple times for viewing a node, but this patch prevents the tool bar and menu links from invoking it.
Applied patch locally - it works. Very nice job on tightening up the changes to the access callback.
Comment #124
moshe weitzman CreditAttribution: moshe weitzman commentedIf this fails due to bad uid, we should return FALSE in order to avoid a NOTICE on next line.
should protect against user_load failure to avoid notice
should protect against user_load failure to avoid notice
I'm on crack. Are you, too?
This is the dumbest message. Can it go, please?
Comment #125
sun@moshe: Fixed a long time ago. Click the install/update link on http://drupal.org/project/dreditor
Comment #126
webchickComment #127
pwolanin CreditAttribution: pwolanin commentedfollow-up performance issue for RDF module: #683590: user_load still being called for every node view
Comment #128
carlos8f CreditAttribution: carlos8f commentedI like the simplicity of Gabor's patch. However, it only fixes user.module, and we've already spent quite a while working out a generic solution. #107 can apply to any menu links prone to this performance problem, and we tried to make it as clean as possible while minding restrictions on API changes.
Comment #129
pwolanin CreditAttribution: pwolanin commented@carlos8f is there another known problem other than the user link?
Re-roll to address moshe's concerns - note especially an admin would get a blank page for a user/$uid that was invalid (in addition to notices). This makes it a 404.
note also - chx says - is_object() is about as fast as isset()
Comment #130
catchThe other situation we have is #606608: Use proper menu router paths for comment/* although not down to this since that's tabs rather than menu links, that probably needs reverting back to anonymous placeholders though. I'm pretty sure a similar situation would happen with a link to a view - but it's likely that views needs to use anonymous placeholders too if that hasn't changed since the last time I looked.
Comment #131
chx CreditAttribution: chx commentedI like this better to a generic change this late the cycle.
Comment #132
Dries CreditAttribution: Dries commentedWould be great to get some performance numbers for the patch in #129 (if we all believe that is the way to go).
Comment #133
carlos8f CreditAttribution: carlos8f commentedIn English, please? :)
Same as above, and this function name should go in the hall of shame. Let's just hope no contrib developers ever have to figure this out :P
MENU_NOT_FOUND should be returned to trigger a 404.
1 days to code freeze. Better review yourself.
Comment #134
pwolanin CreditAttribution: pwolanin commentedfixed return value and doxygen.
@Dries - real gain is in combination with http://drupal.org/node/683590, but it should be the 5-8% gain already reported since we avoid the user_load()
Comment #135
carlos8f CreditAttribution: carlos8f commentedOne question: can we rename user_uid_only_optional_to_arg() to user_uid_to_arg()? The "optional" part is assumed here, I think. And "only" points out that the naming of user_uid_optional is confusing, since 'uid' refers to the argument rather than the return value.
In D8 let's clean all this stuff up. All these wrappers, _to_arg() and uid_optional functions look pretty damn crufty. Ideally we'd just use a $uid_optional flag in 'load arguments', something along those lines.
Comment #136
Dries CreditAttribution: Dries commentedIt might be good to add little @todo's in the code with reminders to clean things up in D8.
Comment #137
pwolanin CreditAttribution: pwolanin commented@carlos8f - the to_arg functionality is pretty powerful even if not widely used. I'm not sure your suggestion makes sense.
We added the 'optional' to the loader name in an API-breaking change for D6 because without it there were problems with people understanding the API and risking security issues.
Comment #138
carlos8f CreditAttribution: carlos8f commented@pwolanin, I'm aware of the power of to_arg, just that it's one of those things that seems confusing when learning the Drupal API. Any improvements in D8 (even to the terminology involved) would be nice.
The cruft I referred to is mainly with the "uid_optional" stuff, I think that is better suited as a flag to pass to user_load(), in the menu system via load arguments. It's confusing to developers when to use %user_uid_optional, case-in-point ksenzee in #17 :)
Comment #139
pwolanin CreditAttribution: pwolanin commentedWell - what kind of todo? "// @todo: make the Drupal APIs sane and less confusing." Maybe that could be a mission statement for D8, though in a few areas I think D7 managed to achieve that :-)
Comment #140
carlos8f CreditAttribution: carlos8f commentedJust a note that this patch is mutually exclusive with #361471: Global $user object should be a complete entity, which proposes to run user_load() for the current user after every bootstrap.
Comment #141
Gábor HojtsyAdded @todo comments to the loaders and to_args of the "user_uid_*" argument types as @Dries requested. We can debate the naming of these endlessly, as @pwolanin pointed out, we added "_uid_optional" in D6 to explicitly document that the uid is optional. I agree a full cleanup of these argument types would be nice, but given how late we are in the D7 cycle, this does not seem to be desired.
Comment #142
carlos8f CreditAttribution: carlos8f commentedLooks OK, let's do it.
Comment #144
Dries CreditAttribution: Dries commentedAlrighty! Committed to CVS HEAD. Thanks for all the hard work, folks. This should provide a nice win.