_menu_link_translate() might avoid calling _menu_load_objects()

Correct status

Please see #638070: Router loaders causing a lot of database hits for access checks, for a lengthy discussion on the same subject.

The test results speak by themselves. At the very minimum:

• your condition is wrong. I guess you meant is_numeric($item['access_callback']) instead of !isset($item['access_callback'])... which doesn't really make any sense to me
• if you remove the loading of objects from here, you have to put it below in _menu_item_localize() because the objects can be used when there is a title callback

Retitling. We can avoid calling _menu_load_objects() at all when translating a menu link in one specific case:

• The access callback is a boolean,
• The title_callback is t() and no title arguments are used

This is not much, but might be significant in some use cases (ie. Views) when costly objects are loaded. This specific change could be backported to D6.

Alternatively, we could refine _menu_load_objects() to load only the objects we care about for a particular callback. This might get us a significant improvement in performance because for links we only care about the access and title callbacks, that might in a lot of cases not need to load the objects (or not all of them). I believe that this is a more significant change, that can only targets D7 (or even only D8 at this stage of the game).

@Damien: I haven't verified it, but I think Mixologic's original report on Views mentioned _menu_load_objects() getting called twice here when viewing the actual views page callback. Views doesn't static cache it's loader function so that results in it building everything twice.

Is this a worry if _menu_load_objects are cached? #238695: Document static cache requirements for menu loaders

Here is a patch with @Damien's suggested changes from comment #6. I don't expect this to be at the end state, just a next step.

I would still rather see the objects cached in _menu_load_objects but I'll code whatever you agree upon.

Downgrading this from critical. Would be good to fix but it doesn't block a release, we need to document somewhere that modules should either do static caching in their router loaders or use plain wildcards.

@catch see #238695: Document static cache requirements for menu loaders comment number 23 with a documentation patch.

subscribe from #402944: Persistent caching for unpack_options() calls from building displays
6.x patch attached

@mikeytown2: please read #6: there is only one case when we can safely skip loading the menu objects. Your patch will break stuff down the road if (for example) the title callback wants to use one of the loaded objects.

I re-rolled the patch basing on what I understood from comment #6 from Damien.

Here is a reroll from #16 for D6.

In regards to #6, #13, and #14 for D6 this is the logic I've come up with

    // menu_tree_check_access() may set this ahead of time for links to nodes.
    if (!isset($item['access'])) {
      if (isset($item['access_callback']) && $item['access_callback'] == 'views_access' && $item['access_arguments'] == 'a:1:{i:0;b:1;}' && $item['title_callback'] == 't' && empty($item['title_arguments'])) {
        $item['access'] = TRUE;
      }
      elseif (!_menu_load_objects($item, $map)) {
        // An error occurred loading an object.
        $item['access'] = FALSE;
        return FALSE;
      }
      _menu_check_access($item, $map);
    }

It's highly specialized as the access_callback is set to views_access for views. Gives over a 250ms improvement on our test server for a front page load.

Stock:
Total Self: 76ms
Total Cumulative: 675ms
_menu_link_translate() calls: 684

D6 Patch:
Total Self: 81ms
Total Cumulative: 404ms
_menu_link_translate() calls: 684

#16: 753064_avoid_calling_menu_load_objects.patch queued for re-testing.

#9: 753064.menu_inc-8.patch queued for re-testing.

The proposed patches are copy-ing existing code.
1. (!is_numeric($item['access_callback']) is already contained in _menu_check_access().
1. ($item['title_callback'] != 't' && !empty($item['title_arguments'])) is already contained in _menu_item_localize();

#1978176: Build menu_tree without loading so many objects contains another approach, by trying to load the object as late as possible, and to execute _menu_load_objects() only in menu_unserialize(). It's not perfect, yet, and needs help.
#1978176 avoids some other loading problems, too.

Testing D6 patch from #20 and will move issue back to D7 once this passes.

@johnv
The D6 patch I've been using will skip loading the view if there are no access checks for it; access argument = array(0 => TRUE)

#25 passes, moving this back to 7.x