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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

with navigation turned on, i get three calls for that as well (along with the three for management). nasty.

catch’s picture

Title: Reduce database hits from menu_load() » Router loaders causing a lot of database hits for access checks

So 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.

moshe weitzman’s picture

Priority: Normal » Critical

Bumping 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.

catch’s picture

Component: menu.module » menu system

Moving to menu component.

chx’s picture

Status: Active » Needs work
FileSize
2.98 KB
2.59 KB

I 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.

catch’s picture

Hmm, 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().

pwolanin’s picture

@catch - so in a sense we call the access callback before the loading callbacks IF the access arguments don't contain any integers?

catch’s picture

pwolanin, 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).

sun’s picture

Anonymous’s picture

subscribe

pwolanin’s picture

I'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?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.37 KB

Here'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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

RDF 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.

ksenzee’s picture

Status: Needs work » Needs review
FileSize
4.44 KB

Running this one past the bot. It removes blog and tracker modules' dependency on user_uid_optional_load(). I think.

Status: Needs review » Needs work

The last submitted patch failed testing.

ksenzee’s picture

Status: Needs work » Needs review
FileSize
6.85 KB

Duh, 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...

Status: Needs review » Needs work

The last submitted patch failed testing.

carlos8f’s picture

subscribe

ksenzee’s picture

Status: Needs work » Needs review
FileSize
7.69 KB

Tests 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. :)

moshe weitzman’s picture

I'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.

pwolanin’s picture

@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?

Dries’s picture

@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.

moshe weitzman’s picture

Recently, 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.

sun’s picture

I 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...?

pwolanin’s picture

@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.

carlos8f’s picture

Status: Needs review » Needs work

Seems 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?

sun’s picture

Status: Needs work » Needs review

So 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.

carlos8f’s picture

Status: Needs review » Needs work

I 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 :)

+++ modules/blog/blog.module
@@ -113,7 +113,7 @@ function blog_menu() {
-  $items['blog/%user/feed'] = array(
+  $items['blog/%user_uid_optional/feed'] = array(
     'title' => 'Blogs',

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.

+++ modules/user/user.module
@@ -1609,25 +1613,6 @@ function user_init() {
-function user_uid_optional_load($uid = NULL) {
-  if (!isset($uid)) {
-    $uid = $GLOBALS['user']->uid;
-  }
-  return user_load($uid);
-}

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.

+++ modules/user/user.module
@@ -1678,7 +1663,14 @@ function user_uid_optional_to_arg($arg) {
-function user_page_title($account) {
+function user_page_title($uid) {
+  // Shortcut for the current user.
+  if ($GLOBALS['user']->uid == $uid) {
+    $account = $GLOBALS['user'];
+  }
+  else {
+    $account = user_load($uid);
+  }
   return format_username($account);
 }

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?

carlos8f’s picture

Cross post. I still think this needs work though.

carlos8f’s picture

Agreed with @sun on #28. This seems like a regression and is actually the domain of entity caching. (what's the status on that?)

catch’s picture

OK 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.

carlos8f’s picture

I 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.

moshe weitzman’s picture

@catch, @chx - can we get a patch here which implements the menu changes described in #32?

carlos8f’s picture

Status: Needs work » Needs review
FileSize
5.82 KB

For 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:

carlos$ ab -n 1000 -c 1 -H 'Cookie: SESS1265b4b277ba52739b6cb93d7a381bd3=fa28c4c34bdb8c375bc8efaa479909e7' http://d7.7/

Document Path:          /
Document Length:        5612 bytes

Time taken for tests:   330.261 seconds
Complete requests:      1000
Requests per second:    3.03 [#/sec] (mean)
Time per request:       330.261 [ms] (mean)

After patch:

carlos$ ab -n 1000 -c 1 -H 'Cookie: SESS1265b4b277ba52739b6cb93d7a381bd3=fa28c4c34bdb8c375bc8efaa479909e7' http://d7.7/

Document Path:          /
Document Length:        5616 bytes

Time taken for tests:   306.233 seconds
Complete requests:      1000
Requests per second:    3.27 [#/sec] (mean)
Time per request:       306.233 [ms] (mean)

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.

carlos8f’s picture

I'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.

pwolanin’s picture

#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?

sun’s picture

So 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.

catch’s picture

I 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.

function user_menu() {
  $items['user/%user_uid_optional'] = array(
    'title' => 'My account',
    'title callback' => 'user_page_title',
    'title arguments' => array(1),
    'page callback' => 'user_build',
    'page arguments' => array(1),
    'access callback' => array(
      'operator' => 'OR',
      'user_access_to_own_profile',
      'user_is_user_admin',
      LOAD_ARGUMENTS_PLZ => 'user_can_access_other_users_profile',
    ),
    'access arguments' => array(1),
    'weight' => -10,
    'menu_name' => 'user-menu',
    'file' => 'user.pages.inc',
  );
}

etc.

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.

carlos8f’s picture

@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.

Dries’s picture

Just 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".

Dries’s picture

To 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.

carlos8f’s picture

I 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 :)

chx’s picture

FileSize
5.76 KB

This is what catch is talkin' about.

chx’s picture

On 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.

sun’s picture

1) 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.

carlos8f’s picture

Status: Needs review » Needs work

I'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.

int’s picture

subscribe

carlos8f’s picture

FileSize
11.69 KB

Here'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.

carlos8f’s picture

FileSize
11.98 KB

I 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.

carlos8f’s picture

Right now I'm curious what approach will be more effective: (inside an access callback)

  if (is_numeric($comment)) {
    $comment = db_query("SELECT cid, uid FROM {comment} WHERE cid = :cid", array(':cid' => $comment))->fetch();
  }
  if (is_numeric($comment)) {
    $comment = comment_load($comment);
  }

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.

catch’s picture

Just a note that comment_load() and menu_load() aren't static cached.

carlos8f’s picture

catch: any reason for that?

carlos8f’s picture

I imagine in the case of comments static caching would cause memory bloat. And caching menu_load() was the original purpose of this issue. :)

catch’s picture

For 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.

pwolanin’s picture

@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.

carlos8f’s picture

pwolanin, 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.

catch’s picture

One 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.

carlos8f’s picture

FileSize
25.4 KB

catch, interesting suggestion. I will think on that.

This patch should take care of the access callback changes. Now for title callbacks and tests.

catch’s picture

One 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.

carlos8f’s picture

catch, 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.

chx’s picture

See http://drupal.org/node/310077 on getmetadata and friends.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
35.49 KB

I 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.

Status: Needs review » Needs work
Issue tags: -Performance

The last submitted patch, , failed testing.

Status: Needs work » Needs review

Re-test of from comment #2403454 was requested by @user.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

Status: Needs work » Needs review
Issue tags: +Performance

Re-test of from comment #2403454 was requested by @user.

carlos8f’s picture

FileSize
34.44 KB

Improved 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.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
38.86 KB

Fixed 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.

David_Rothstein’s picture

Hm, 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.

  1. If we're willing to make API changes here, maybe we should discuss #39 and #44 more?

    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).

  2. If we don't want to make major changes to existing APIs, which ideally is a goal at this point, couldn't we do something where the lazy loading is "opt-in"? Again borrowing from a bit #44 (though the actual syntax wouldn't work for this), it would be something like the following:
    $items['whatever/%node/whatever/%user'] = array(
      ...
      'access callback' => 'some_access_callback_function',
      'access arguments' => array(
        'just give me the ID, not the whole object thank you very much' => 1,
        3,
      ),
      ...
    );
    

    Which would lead to an access callback function with the following signature:

    function some_access_callback_function($nid, $account) {
    ...
    }
    

    (And this approach would ideally work for all menu system callbacks, not just access callbacks.)

carlos8f’s picture

David_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.

carlos8f’s picture

Status: Needs review » Needs work

I 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:


if (user_is_logged_in()) {
  $links[] = l(t('My account'), 'user');
  $links[] = l(t('Log out'), 'user/logout');
  // (render links)
}

These links never really change. It's more expandable as a menu, but slows down every page load.

catch’s picture

Yeah 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).

Status: Needs work » Needs review

Re-test of perf_store_1.patch from comment #5 was requested by pradeepsingh.

carlos8f’s picture

FileSize
2.64 KB

So 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.

catch’s picture

Just 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.

sun’s picture

+++ modules/menu/menu.module	9 Jan 2010 02:26:13 -0000
@@ -213,9 +213,32 @@
+function menu_custom_menus() {

Can we name this menu_load_all() or similar?

Otherwise, we'll have:

menu_custom_menus()
menu_get_menus()
menu_load()

...which is... quite confusing.

+++ modules/menu/menu.module	9 Jan 2010 02:26:13 -0000
@@ -213,9 +213,32 @@
+  $custom_menus = &drupal_static(__FUNCTION__, NULL);

As catch mentioned, we can remove the NULL here.

+++ modules/menu/menu.module	9 Jan 2010 02:26:13 -0000
@@ -213,9 +213,32 @@
+  if (is_null($custom_menus)) {

!isset()

+++ modules/menu/menu.module	9 Jan 2010 02:26:13 -0000
@@ -213,9 +213,32 @@
+    if ($cached = cache_get('menu_custom')) {

I think this should use {cache_menu}

+++ modules/menu/menu.module	9 Jan 2010 02:26:13 -0000
@@ -213,9 +213,32 @@
+      $custom_menus = db_query("SELECT * FROM {menu_custom}")->fetchAllAssoc('menu_name', PDO::FETCH_ASSOC);

Single quotes.

Appending a drupal_alter() here would make sense, I think.

+++ modules/menu/menu.module	9 Jan 2010 02:26:13 -0000
@@ -213,9 +213,32 @@
@@ -242,6 +265,8 @@

Please re-roll with diff -up, because we don't see the context of changes.

+++ modules/menu/menu.module	9 Jan 2010 02:26:13 -0000
@@ -242,6 +265,8 @@
+  cache_clear_all('menu_custom', 'cache');

@@ -290,6 +315,8 @@
+  cache_clear_all('menu_custom', 'cache');

When using {cache_menu}, those are probably no longer required.

Powered by Dreditor.

catch’s picture

The 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.

Dries’s picture

I'm willing to make API changes prior to alpha if they bring significant performance improvements, where significant is (probably) 5% or more.

catch’s picture

FileSize
6.49 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, menu_access.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
+++ includes/menu.inc	9 Jan 2010 16:29:42 -0000
@@ -3171,6 +3207,13 @@ function _menu_router_build($callbacks) 
+    if (is_array($item['access callback'])) {
+      $item['access_callback']['#op'] = isset($item['access callback']['#op']) ? strtolower($item['access callback']['#op']) : 'and';
+      $item['access callback'] = serialize($item['access callback']);
+      foreach ($item['access arguments'] as &$arguments) {
+        $arguments = serialize($arguments);
+      }
+    }

+++ modules/user/user.module	9 Jan 2010 16:29:46 -0000
@@ -1515,8 +1513,17 @@ function user_menu() {
-    'access callback' => 'user_view_access',
-    'access arguments' => array(1),
+    'access callback' => array(
+      '#op' => 'or',
+      'user_uid_is_current',
+      'user_access',
+      'user_view_access',
+    ),
+    'access arguments' => array(
+      array('please menu system do not fire user_load so manu times' => 1),
+      array('administer users'),
+      array(1),
+    ),

I 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:

$item['access/OR/simple'] = array(
  'access callback' => 'menu_access_multiple',
  'access arguments' => array(
    // All OR'ed:
    'user_uid_is_current' => array(),
    'user_access' => array('administer users'),
    'user_view_access' => array(1),
  ),
);
$item['access/AND'] = array(
  'access callback' => 'menu_access_multiple',
  'access arguments' => array(
    // OR'ed:
    'user_uid_is_current' => array(),
    // AND'ed:
    array(
      'user_access' => array('administer users'),
      'user_view_access' => array(1),
    ),
  ),
);

Or similar. Thoughts?

5 days to code freeze. Better review yourself.

Dries’s picture

Sorry, 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.

catch’s picture

FileSize
123.94 KB
148.26 KB

While 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.

David_Rothstein’s picture

As I said above in #71....

  1. If we're going to do the full API change and allow multiple access callbacks strung together, then instead of any kind of and/or syntax, the simplest way to do that would be via a three-valued return system (similar to NODE_ACCESS_ALLOW, NODE_ACCESS_DENY, NODE_ACCESS_IGNORE).
  2. However, I don't see why we need this in Drupal 7. As far as I can see, the only part of that patch that we really need is the part that allows you to do this:
    +      array('please menu system do not fire user_load so manu times' => 1),
    

    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?

carlos8f’s picture

Status: Needs review » Needs work

I 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.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
3.18 KB

This should address everything in #79 and #80.

catch’s picture

Status: Needs review » Needs work

I 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.

carlos8f’s picture

catch, 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.

carlos8f’s picture

catch, 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?

sun’s picture

Note 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?

catch’s picture

Spoke to chx in irc. After going round on a few different options, we think this is good:

$items['foo'] = array(
  'skip load for links' => TRUE,
);

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.

catch’s picture

Status: Needs work » Needs review
FileSize
5.91 KB

And a patch. User tests pass, didn't run any others though. Performance numbers should be more or less identical to #86.

carlos8f’s picture

Status: Reviewed & tested by the community » Needs review

catch, this looks eerily familiar. I implemented the idea in #94 30 days ago, in #35! :)

Edit: 20 days ago actually.

chx’s picture

Status: Needs review » Reviewed & tested by the community

LOL! The only problem there then was schema change. BAH! We can't come up with anything better, #35 contains benchmarks, FIRE.

carlos8f’s picture

#89 is probably RTBC as well, should we merge them?

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

@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'.

chx’s picture

pwolanin, no, they will get either 123 or node_load(123)

carlos8f’s picture

pwolanin, 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I 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.

carlos8f’s picture

Status: Needs work » Needs review
FileSize
7.23 KB

Added better docs.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Since 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).

carlos8f’s picture

Keep 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.

catch’s picture

IMO #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.

carlos8f’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.12 KB

Rolling the two together, RTBC per #104 and #106: menu link optimization flag + menu_load() cache.

carlos8f’s picture

Status: Needs review » Reviewed & tested by the community

So #107 is the culmination of this issue, reviewed and tested. Phew, let's move on :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Why 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.

chx’s picture

Status: Needs work » Reviewed & tested by the community

_to_arg

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

@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.

catch’s picture

Status: Needs work » Needs review

There'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.

Gábor Hojtsy’s picture

#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.

Gábor Hojtsy’s picture

The 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 :)

Gábor Hojtsy’s picture

BTW 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).

catch’s picture

Keeping _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

Status: Needs review » Needs work

The last submitted patch, user-uid-only-optional.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.02 KB

The 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.

sun’s picture

+++ modules/user/user.module	13 Jan 2010 16:04:43 -0000
@@ -1519,17 +1546,18 @@ function user_menu() {
+  $items['user/%user_uid_only_optional'] = array(

Could 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.

catch’s picture

I'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.

pwolanin’s picture

Indeed - lazy doesn't make sense to me. The 'optional' relates to the to_arg function.

Gábor Hojtsy’s picture

Yeah, 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" :).

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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.

moshe weitzman’s picture

+++ modules/user/user.module	13 Jan 2010 16:34:12 -0000
@@ -1346,16 +1346,31 @@ function user_register_access() {
+        $account = user_load($uid);

If this fails due to bad uid, we should return FALSE in order to avoid a NOTICE on next line.

+++ modules/user/user.module	13 Jan 2010 16:34:12 -0000
@@ -1672,9 +1688,22 @@ function user_uid_optional_to_arg($arg) 
+  else {
+    $account = user_load($uid);
+  }

should protect against user_load failure to avoid notice

+++ modules/user/user.module	13 Jan 2010 16:34:12 -0000
@@ -2107,6 +2136,13 @@ function _user_cancel($edit, $account, $
+function user_view_page($uid) {
+  return user_view(user_load($uid));
+}

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?

sun’s picture

@moshe: Fixed a long time ago. Click the install/update link on http://drupal.org/project/dreditor

webchick’s picture

Status: Reviewed & tested by the community » Needs work
pwolanin’s picture

follow-up performance issue for RDF module: #683590: user_load still being called for every node view

carlos8f’s picture

I 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.42 KB

@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()

catch’s picture

The 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.

chx’s picture

I like this better to a generic change this late the cycle.

Dries’s picture

Would be great to get some performance numbers for the patch in #129 (if we all believe that is the way to go).

carlos8f’s picture

Status: Needs review » Needs work
+++ modules/user/user.module
@@ -1662,7 +1679,7 @@ function user_category_load($uid, &$map, $index) {
 /**
- * Returns the user id of the currently logged in user.
+ * Returns the user id of the currently logged in user unless $arg is not %.
  */

In English, please? :)

+++ modules/user/user.module
@@ -1672,10 +1689,23 @@ function user_uid_optional_to_arg($arg) {
 /**
+ * Returns the user id of the currently logged in user unless $arg is not %.
+ */
+function user_uid_only_optional_to_arg($arg) {

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

+++ modules/user/user.module
@@ -2107,6 +2137,16 @@ function _user_cancel($edit, $account, $method) {
+  return is_object($account) ? user_view($account) : drupal_not_found();

MENU_NOT_FOUND should be returned to trigger a 404.

1 days to code freeze. Better review yourself.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.41 KB

fixed 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()

carlos8f’s picture

One 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.

Dries’s picture

It might be good to add little @todo's in the code with reminders to clean things up in D8.

pwolanin’s picture

@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.

carlos8f’s picture

@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 :)

pwolanin’s picture

Well - 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 :-)

carlos8f’s picture

Just 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.

Gábor Hojtsy’s picture

Added @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.

carlos8f’s picture

Status: Needs review » Reviewed & tested by the community

Looks OK, let's do it.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alrighty! Committed to CVS HEAD. Thanks for all the hard work, folks. This should provide a nice win.

Status: Fixed » Closed (fixed)
Issue tags: -Performance

Automatically closed -- issue fixed for 2 weeks with no activity.