Problem/Motivation

The menu tree must be loaded without loading the objects, menu items are linking to.
On every page request, the menu tree is rebuilt. For every Entity (node, term, product, ...) with a menu link, the Entity itself is loaded, too. This is done too often, giving performance problems (memory problems, page load time >> 1sec) or too few, giving rendering-problems (in case a menu is shown a secon time on a page).

We do need some properties for every Entity in the menu:
- its Access must be checked, to see if the Entity is to be included in the menu.
- its title is used for the menu link.
But we do NOT need to load the object (in most cases).

This issue proposes to move the loading of the object to the moment the entity is really needed.

Proposed resolution

When building the menu tree using _menu_link_translate(), function _menu_load_objects() is called too often or too few. Attached patch tries to resolve the problem, not only the symptom: _menu_load_objects() should be called just-in-time.

The attached patch gives these results on a page with admin_menu, a megamenu and a menu_block, and a menu structure that contains a taxonomy_menu of 2 vocabularies (of 63 and 13 tems):
1. original code of menu.inc:

Executed 373 queries in 258 ms. Page execution time was 1973 ms. Memory used at: devel_boot()=1.5 MB, devel_shutdown()=46.09 MB, PHP peak=47.75 MB.
Executed 374 queries in 253 ms. Page execution time was 1763 ms. Memory used at: devel_boot()=1.5 MB, devel_shutdown()=46.09 MB, PHP peak=47.75 MB
Executed 373 queries in 244 ms. Page execution time was 1752 ms. Memory used at: devel_boot()=1.5 MB, devel_shutdown()=46.09 MB, PHP peak=47.75 MB.

2. patched code of menu.inc:

Executed 248 queries in 229 ms. Page execution time was 1705 ms. Memory used at: devel_boot()=1.5 MB, devel_shutdown()=45.61 MB, PHP peak=47.5 MB.
Executed 248 queries in 219 ms. Page execution time was 1704 ms. Memory used at: devel_boot()=1.5 MB, devel_shutdown()=45.61 MB, PHP peak=47.5 MB.

This is how it works: The callstack in question is doing something like this:
menu_tree_all_data($menu_name)
->menu_tree_check_access($tree, $node_links)
-->_menu_tree_check_access(&$tree)
--->_menu_link_translate($item)
----> _menu_check_access($item, $map);
-----> menu_unserialize()
----> _menu_item_localize($item, $map, TRUE);
-----> menu_unserialize()
Proposed patch moves _menu_load_objects() from _menu_check_access($item, $map) to menu_unserialize().
This way, the object is loaded only when it is needed.

Remaining tasks

- In the core code, Nodes are loaded all-at-once in a special request, all other Entities 1-by-1. This seems out-dated, with Entities becoming ubiquitous.
- menu_unserialize() is really an internal function to menu.inc, so should be renamed to _menu_unserialize().

User interface changes

None.

API changes

Some internal functions have a changed interface. The public API is not changed.

The following issues all have different symptoms and solutions for the same problem.
#753064: _menu_link_translate() might avoid calling _menu_load_objects()

If you have menu items that refer to views (or any other expensive load_functions for that matter), this patch will dramatically increase your sites performance.

#1697570: _menu_load_objects() is not always called when building menu trees

...I wanted to display the main menu two times, one time as a dropdown menu like described above, and one time in the footer. Everything works fine with that, output is just like expected. But if there are two blocks, the following type of error occurs for every menu-item that links to a node: ....

#1973920: Memory/Performance problems, when building a menu with a large taxonomy_menu

I have taxonomy_menu enabled... The whole vocabulary is read term-by-term by function _menu_load_objects(). In most cases, the menu can be generated without loading every term.

#1905144: High load time for admin_menu upon user login/menu refresh

I noticed that admin_menu is rebuilding the menu every time user is logging in. Login page loads on average in 30 seconds, which is way too long.

CommentFileSizeAuthor
#127 interdiff.txt631 bytesizmeez
#127 drupal-menu-load-objects-1978176-127.patch9.8 KBizmeez
#75 drupal-1978176-menu_load_objects-75.patch9.76 KBjohnv
#72 drupal-1978176-menu_load_objects-72.patch9.78 KBjohnv
#65 interdiff.txt1 KBstefan.r
#64 drupal-1978176-menu_load_objects-64.patch9.54 KBstefan.r
#51 Screenshot 2014-12-07 12.29.02.png38.53 KBtorgosPizza
#51 Screenshot 2014-12-07 12.26.28.png29.3 KBtorgosPizza
#46 drupal-1978176-menu_load_objects.patch8.47 KBSteven Jones
#32 drupal7_1978176_32_menu_load_objects.patch9.55 KBjohnv
#30 drupal7_1978176_28_menu_load_objects.patch9.62 KBmikeytown2
#28 drupal7_1978176_28_menu_load_objects.patch.txt9.62 KBjohnv
#25 drupal7_1978176_25_menu_load_objects.patch11.23 KBjohnv
#18 drupal8_menu_1978176_17_build_menu_objects.patch11.53 KBjohnv
#16 drupal7_menu_1978176_15_menu_load_objects-do-not-test.patch14.44 KBjohnv
#14 drupal8_menu_1978176_14_build_menu withoutloadingobjects.patch615 bytesjohnv
#11 drupal7_1978176_11_menu_load_objects.patch9.88 KBjohnv
#9 drupal7_1978176_9_menu_load_objects.patch8.41 KBjohnv
#8 drupal7_1978176_8_menu_load_objects.patch8.44 KBjohnv
#4 drupal7_1978176_4_menu_load_objects.patch8.15 KBjohnv
#1 drupal7_1978176_1_menu_load_objects.patch4.57 KBjohnv
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

johnv’s picture

Status: Active » Needs review
FileSize
4.57 KB

Here is a D7 patch.

Status: Needs review » Needs work

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

johnv’s picture

So #1 only works for objects with numeric id's. Most tests fail because of the 'node type edit page', where the object_id is e.g., 'page' of 'article'.

It did make a huge difference in page load time. Below the results for a page with a menu structure, including a vocabulary-menu with 56 unnecessarily-loaded terms:
Pre: 353 queries in 247.42 ms. Page execution time was 1022.89 ms. Memory used at: devel_boot()=1.08 MB, devel_shutdown()=15.11 MB, PHP peak=16.25 MB
Post: 225 queries in 202.47 ms. Page execution time was 935.32 ms. Memory used at: devel_boot()=1.08 MB, devel_shutdown()=14.61 MB, PHP peak=15.5 MB.

johnv’s picture

Status: Needs work » Needs review
FileSize
8.15 KB

New try. resolved several cases.
function field_ui_menu_title() hase 3 cases. 1 still generates errors (after cache flush);

Status: Needs review » Needs work

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

johnv’s picture

Issue summary: View changes

Better decription.

johnv’s picture

Title: _menu_link_translate() should call _menu_load_objects() just-in-time » Building the menu_tree: when to load menu objects
Version: 7.x-dev » 8.x-dev
Status: Needs work » Active

It seems the patch #4 still needs some work. Before putting more effort in this, I'd like to see what the changes are too get this in Core, be it D8 of (only) D7.

The patch is developed in D7, but a quickscan on the D8-code, it seems the affected code hasn't changed that much.

johnv’s picture

Issue summary: View changes

Better info

johnv’s picture

Title: menu_tree building performance: loads menu objects too often & too few. » Building the menu_tree: when to load menu objects
Version: 7.x-dev » 8.x-dev
Status: Needs review » Active

Added the following issue to the summary, since also admin_menu suffers from the same problem:
#1905144: High load time for admin_menu upon user login/menu refresh

johnv’s picture

Title: Building the menu_tree: when to load menu objects » menu_tree building performance: loads menu objects too often & too few.
Version: 8.x-dev » 7.x-dev
Status: Active » Needs review
FileSize
8.44 KB

testbot.

johnv’s picture

removed dpm().

Title: Building the menu_tree: when to load menu objects » menu_tree building performance: loads menu objects too often & too few.
Version: 8.x-dev » 7.x-dev
Status: Active » Needs work

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

Anonymous’s picture

Issue summary: View changes

Added reference to admin_menu

johnv’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

Attached patch contains corrections for
- calling not-existing pages
- hide message for admin_menu showing all fields of user.

Status: Needs review » Needs work

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

johnv’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Active

Still having some problems on 'page not found' pages.

Back to D8 for feasibility.

johnv’s picture

Issue summary: View changes

Rewrite.

johnv’s picture

Title: menu_tree building performance: loads menu objects too often & too few. » Build menu_tree without loading so much objects.
FileSize
615 bytes

This patch should do the trick.

johnv’s picture

Status: Active » Needs review
johnv’s picture

This is the D7-version.

Status: Needs review » Needs work

The last submitted patch, drupal8_menu_1978176_14_build_menu withoutloadingobjects.patch, failed testing.

johnv’s picture

Status: Needs work » Needs review
FileSize
11.53 KB

#14 was in error. Attached is the correct D8 patch (hopefully).

Status: Needs review » Needs work

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

corneboele’s picture

The patch from #16 helped us bringing the loading times of a customer's site from 5-10 sec to a steady 1.5 sec.

After searching for hours what could cause the slowness of the site we finally came across an issue in the Superfish module (https://drupal.org/node/1926480), where a link to this thread was posted. After applying the patch the site works very responsive again.

We would be very happy if this patch made it into the core (7.23) of Drupal.

johnv’s picture

Made a crosspost on #1805054-83: Cache localized, access filtered, URL resolved, and rendered menu trees, to find out is this patch is still valid for D8.
Depending on this, I will / won't continue to solve all test issues.

johnv’s picture

Issue summary: View changes

Updated issue summary.

player259’s picture

After applying patch #16 (D7.24) on node and term pages appeared duplicate titles.
Problem is here

function menu_get_object($type = 'node', $position = 1, $path = NULL) {
  $router_item = menu_get_item($path);
  if (isset($router_item['load_functions'][$position]) && !empty($router_item['map'][$position]) && $router_item['load_functions'][$position] == $type . '_load') {
    return $router_item['map'][$position];
  }
}

load_functions is unserialized

Another issue caused Webform module
path "node/3/webform/components"
Errors:

Notice: Undefined offset: 4 in _menu_translate() (line 824 of /var/www/MYSITE/includes/menu.inc).
Notice: Undefined offset: 4 in _menu_translate() (line 830 of /var/www/MYSITE/includes/menu.inc).
Wim Leers’s picture

Great work on this, johnv! If you can move these forward again, I can promise you reviews to get this in :)

johnv’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes
Status: Needs work » Needs review
FileSize
11.23 KB

@Wim, thanks voor je aanmoediging.

Please find a new patch attached. It is against latest -dev and stable D7-release.
The problems reported in #22 are now resolved.

There is still a problem with the following use case:
- 2 node types/entity types (A and B).
- each node type has extra views, that are shown as menu tab. (So, alongside the horizontal tabs View, Edit, etc.)
- some extra views should only be shown on A, others on B. Up until now, they were hidden by the views' 'argument validation'. After this patch, they all show on every node type.

I could not find how to resolve that.

ITMT, the D8 menu.inc file is stripped from the 'old' menu system, so IMO this (solution to the) problem is now a strict D7 issue.

Status: Needs review » Needs work

The last submitted patch, 25: drupal7_1978176_25_menu_load_objects.patch, failed testing.

The last submitted patch, 25: drupal7_1978176_25_menu_load_objects.patch, failed testing.

johnv’s picture

FileSize
9.62 KB
johnv’s picture

Status: Needs work » Needs review
mikeytown2’s picture

#28 will not be tested as the extension is txt. I've uploaded the same patch with the correct extension :)

Status: Needs review » Needs work

The last submitted patch, 30: drupal7_1978176_28_menu_load_objects.patch, failed testing.

johnv’s picture

Status: Needs work » Needs review
FileSize
9.55 KB

Made a fresh one. Didn't use the latest dev version to compare with.

Status: Needs review » Needs work

The last submitted patch, 32: drupal7_1978176_32_menu_load_objects.patch, failed testing.

johnv’s picture

Wow, happy with those test results. Some of them appear to refer to another problem I forgot to mention: working with URLs that include a sub-object, like taxonomy/term/1.
Someone likes to tackle that?

[EDIT]
They generate the following error, which is reported and fixed in several contribs:
Notice: Undefined offset: 5 in _menu_translate() on line 777 of menu.inc

It is on pages with an optional argument as last parameter: the bad path: user/%/events/% (intended to pass uid and date in the url); the good path: user/%/events

See this comment and its links :
#2038933-11: Undefined offset: 4 in _menu_translate() (line 777 of includes/menu.inc) - Caused by Webform
#1999286-1: Notice: Undefined offset: 5 in _menu_translate() (line 777 of includes/menu.inc for menu items with a magic menu loader argument.

Wim Leers’s picture

Version: 7.x-dev » 8.x-dev

Why did you move this to Drupal 8? These things must *first* be fixed in Drupal 8, and only when it's fixed in Drupal 8, they may be backported to Drupal 7.

pounard’s picture

Version: 8.x-dev » 7.x-dev

Is that even still appliable to Drupal 8? It moved so fast with routing and stuff that I seriously doubt this actually applies to it. Don't mess up with Drupal 7 performance patches, they can greatly help already existing sites, and could help right now, don't hostage issues like this with the "Drupal 8 first" rule.

Wim Leers’s picture

Version: 7.x-dev » 8.x-dev

I didn't invent this rule. And it exists for good reasons. I've had to get a D7 patch in too, and I didn't like it either, but everybody has to. (Irony wants that I got the D8 patch committed, but the D7 patch still not yet: #1961340: CSS aggregation breaks file URL rewriting because it abuses it).

When I first looked at this issue, it was for D8 — johnv cross-posted to #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, which has >40 followers because it's so important — that's why I was interested. But it indeed looks like parts of this code no longer exist in D8. Even better then :) But many functions still exist and are very similar: _menu_item_localize(), _menu_link_translate() and menu_tree_check_access().

I'm not well-versed enough in the menu or routing system to determine whether this is still relevant, but until then, this is a 8.x issue. I hope johnv can take a look at it soon.

dawehner’s picture

If we care about that we should do it after #2207893: Convert menu tree building to a service. is in.

johnv’s picture

@Wim,
I changed to D7 for the following reasons:
non-valid reasons:
- D8 code is a moving target,
- I have a D7-site with big enough content to notice performance improvements and test pages. I'm working against that, porting occasionally to D8.
- The uploaded patch was against D7, so in order to be tested, the version had to be (temporarily) reset to D7.

Valid reasons:

ITMT, the D8 menu.inc file is stripped from the 'old' menu system, so IMO this (solution to the) problem is now a strict D7 issue.

The main instigator of the patch is the following removal of code:

-      if (!empty($item['load_functions']) && !_menu_load_objects($item, $map)) {
-        // An error occurred loading an object.
-        $item['access'] = FALSE;
-        return FALSE;
-      }

This happens in two locations:
- _menu_translate(), which builds the menu, and loads the objects. This is removed in D8 in favour of the new router system. That's why IMO this patch is not D8-relevant. The new router design might avoid this pitfall itself.
- _menu_link_translate(), which indeed still exists in D8, so I might be wrong.

P.S. I don't think I have time to work on the patch the next 4 weeks.

pounard’s picture

Version: 8.x-dev » 7.x-dev

#38 and #39 are two very valid reasons of switching this back to D7. D8 is not D7-like enough anymore regarding menus to keep this D8 first. There should be 2 different issues, one for D7 and one for D8.

I'm going to be a black sheep here:I cannot stand a D7 performance patch being held hostage because this specific part of D8 is being refactored first. We all now what will realistically happen there: D8 will diverge so much that this patch won't make any sense in it. Let's just do this for D7 and forget D8 here.

Wim Leers’s picture

#39 indeed indicates sufficiently that this no longer applies to D8.

pounard’s picture

Thank you very much, I hope this patch will move forward. I might give a try someday.

rooby’s picture

Title: Build menu_tree without loading so much objects. » Build menu_tree without loading so many objects
derMatze’s picture

After enabling #39 I get the following error on every node page:
Notice: Trying to get property of non-object in node_page_title() (Line 2211 /modules/node/node.module)

Anyone with the same issue?

//edit:
After re-reading the whole thread I'm not quite sure which patches/corehacks in D7 I need to do. Should #39 be enough or not?

MXT’s picture

@derMAtze not sure why you are referring to #39, latest patch for D7 is in #32.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones
Status: Needs work » Needs review
FileSize
8.47 KB

This may undo all the performance improvements, but I wonder if this will make all the tests pass.
I wonder if the premise of this issue is essentially: 'loading entities to check access is slow, lets skip checking access'.

Steven Jones’s picture

Assigned: Steven Jones » Unassigned
vacho’s picture

the patch at #46 works fine for me. I am working ove druapl "openpublic"

sammarks15’s picture

Status: Needs review » Needs work

The patch at #46 no longer applies successfully in Drupal 7.31. If I have time, I'll post a patch a little later.

Edit: Nevermind - I was looking at the wrong patch. This one works fine.

sammarks15’s picture

Status: Needs work » Needs review

Nevermind - I was looking at the wrong patch. This one works fine. Sorry for the confusion.

torgosPizza’s picture

I've tested the last two patches for D7, and I can confirm that @Steven Jones' quote in #46:

This may undo all the performance improvements...

With the patch from #32, cache_get_multiple() is not called for menu objects that the user does not have access to. For us this results in dramatically faster page load times.

However with patch #46 applied, menu_load_objects() ends up calling CTools' get_some_defaults() functions, which in turn loads all default Views that have a path, regardless of whether the user has access, and regardless of whether the view is disabled or enabled. Each of these calls to the cached Views object is anywhere from 10-30ms on average, and we have a couple dozen Views, resulting in additional time spent in PHP - anywhere from 900ms to 1600ms. (In general it stays around 1600ms.)

For this reason the patch in #32 is the one I prefer, because it actually does have a very large, very measurable impact on performance. I would love to see if that one can be reworked as the one in #46 undoes all of the benefits this Issue was created for.

Problem was unrelated.

The last submitted patch, 32: drupal7_1978176_32_menu_load_objects.patch, failed testing.

torgosPizza’s picture

It looks like I was wrong, apparently there was another module potentially interfering and causing this vast difference in page timing. It does look like the patch in 46 works as well as the last D7 patch. Sorry for the noise.

mikeytown2’s picture

@torgosPizza
"there was another module potentially interfering"
What was it? Or if you can't name it, what did it do to cause issues?

torgosPizza’s picture

EDIT: Turns out I had a corrupted cache item that was causing CTools to constantly reload every View from the cache. Mea culpa.

torgosPizza’s picture

There is a problem with Patch #32:

Logged in as user 1, I can see links (tabs) to menu items I previously could not see due to various access handlers. (This is a Commerce-provided View of User Orders.)

When clicking on the tab, I get a WSOD with these errors:
Warning: call_user_func_array() expects parameter 2 to be array, null given in menu_execute_active_handler() (line 525 of /docroot/includes/menu.inc).

Warning: array_merge(): Argument #1 is not an array in menu_get_item() (line 488 of /docroot/includes/menu.inc).

Reverting, and applying patch from #46, the error no longer occurs.

frost’s picture

Successfully applied #46 to 7.34 site using taxonomy_menu with very large vocabulary.
Page load times running locally before patching were 7-10 seconds. After patching, page loads are down to about 3 seconds. Thank you!

So far looks good but will report back if there are any issues.

kscheirer’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #46 still applies to Drupal 7.34 cleanly. Patch works well, no issues found in a production setting. Used drush cc all; drush cc menu before each execution.

Before: Page execution time was 11129.85 ms. Memory used at: devel_boot()=1.34 MB, devel_shutdown()=56.26 MB, PHP peak=72.5 MB.
After: Page execution time was 10871.38 ms. Memory used at: devel_boot()=1.34 MB, devel_shutdown()=50.1 MB, PHP peak=66.5 MB.

torgosPizza’s picture

I'd also like to see if #32 could be re-rolled and benchmarked again, because it seems from the comments that it had a much better increase in performance than the patch from #46.

kscheirer’s picture

Unpatched: 10873.5 / 10482.37 / 11012.57
Patch from 32: 1730.38 / 1755.37 / 10655.47
Patch from 46: 10629.97 / 1814.1 / 10799.1

Needs more investigation, I'm not sure why the big drop from over 10s to under 2s in some runs is not consistent.

stefan.r’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
9.54 KB
stefan.r’s picture

FileSize
1 KB
stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

Indeed it does solve that issue as patch is green! Reverting to old status.

joelpittet’s picture

@kscheirer What's your profiling scenario?

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

I really like the idea here, but this is a complicated patch touching some pretty fundamental parts of the codebase, so I think it's going to need some more review and testing. In particular this:

 function _menu_check_access(&$item, $map) {
+  // menu_tree_check_access() may set this ahead of time for links to nodes.
+  if (isset($item['access'])) {
+    if ($item['access'] === FALSE) {
+      return FALSE;
+    }
+    return TRUE;
+  }
   $item['access'] = FALSE;

Looks like it has the potential to introduce security vulnerabilities unless we're very confident $item['access'] will never be set to anything but FALSE by code that doesn't intend to.

A couple other things:

  1. This patch is making some big changes to menu_unserialize(), which is technically a public API function (although I doubt it's used often outside the menu system). It would be great if there's a way to avoid that.
  2. -        $router_item['page_arguments'] = array_merge(menu_unserialize($router_item['page_arguments'], $map), array_slice($map, $router_item['number_parts']));
    -        $router_item['theme_arguments'] = array_merge(menu_unserialize($router_item['theme_arguments'], $map), array_slice($map, $router_item['number_parts']));
    +        $router_item['page_arguments'] = array_merge(menu_unserialize($router_item['page_arguments'], $map, $router_item) ? menu_unserialize($router_item['page_arguments'], $map, $router_item) : array(), array_slice($map, $router_item['number_parts']));
    +        $router_item['theme_arguments'] = array_merge(menu_unserialize($router_item['theme_arguments'], $map, $router_item) ? menu_unserialize($router_item['theme_arguments'], $map, $router_item) : array(), array_slice($map, $router_item['number_parts']));
    

    The double call to menu_unserialize() in each of these seems a bit wasteful.

  3. +  if (!_menu_check_access($router_item, $map)) {
    +  //if (isset ($router_item['_cached_object_map']) && $router_item['_cached_object_map'] === FALSE) {
    

    Should remove commented-out code from the patch.

buddym’s picture

Applied patch #46 and everything looks good so far. Thank you very much! No more log notices makes me feel warm and fuzzy : ). Applied patch #46 to Drupal 7.36 with profile OpenPublic (openpublic-7.x-1.5).

Jordan Samouh’s picture

Hi,

This patch does not fix the problem for me...

I use taxonomy menu.

Each time you refresh a page with a main menu (front), the call menu_check_access is made and taxonomies of the menu is loaded.

Example:
- apply patch
- Loading homepage with a general taxonomy menu
- menu_check_access is called => menu_link_translate => menu_load_object for each taxonomy in the menu
- Loading homepage again
- same result.

The idea is for any page containing a general menu, call has to be made once.

johnv’s picture

@Steven Jones, #46: "I wonder if the premise of this issue is essentially: 'loading entities to check access is slow, lets skip checking access'."

I'd rephrase that: "loading entities to check access is slow, lets skip checking access until really needed."

johnv’s picture

Attached patch is taken from #46 #64 and addresses partly the comments from @David Rothstein, #68:

The part in function _menu_check_access(&$item, $map) is essentially moved from the following chunk.

@@ -917,14 +968,10 @@ function _menu_link_translate(&$item, $translate = FALSE) {
       $item['access'] = FALSE;
       return FALSE;
     }
-    // menu_tree_check_access() may set this ahead of time for links to nodes.
-    if (!isset($item['access'])) {
-      if (!empty($item['load_functions']) && !_menu_load_objects($item, $map)) {
-        // An error occurred loading an object.
-        $item['access'] = FALSE;
-        return FALSE;
-      }
-      _menu_check_access($item, $map);
+    if (!_menu_check_access($item, $map)) {
+      // An error occurred loading an object.
+      $item['access'] = FALSE;
+      return FALSE;
     }

However, #1277596: Menu system unexpectedly uses different access control for node links completely removes menu_tree_check_access() completely, which is a good idea IMO. Then this part may be removed from this patch.

ad 1: "This patch is making some big changes to menu_unserialize() [...]". It seems like the introduced 3rd parameter can be made optional, avoiding regression problems. This is addressed in attached patch.
ad 2: This is addressed in attached patch.
ad 3: This is addressed in attached patch.

Status: Needs review » Needs work

The last submitted patch, 72: drupal-1978176-menu_load_objects-72.patch, failed testing.

Anybody’s picture

Patch looks good and improves performance a lot for me. Can we have further feedback? I think this is really important but also a dangerous point.

johnv’s picture

We could raise the priority.

stefan.r’s picture

Priority: Normal » Major
Fabianx’s picture

Added Performance tag, so it can be properly found when searching for Performance.

Marking for review by me.

bjcooper’s picture

The patch in #75 seems to work well for me.

johnv’s picture

If someone else could do a thorough test with the latest patch to set this to RBTC, that would be awesome.

torgosPizza’s picture

The patch does improve performance for me but it seems to be incompatible with Drupal Commerce. After applying this patch, some hooks - mainly, hook_commerce_checkout_complete(), no longer are firing correctly. That hook drives lots of business logic and so if this does affect Commerce this way then I believe it's still Needs Work.

I will do some further testing and see if reverting the patch resolves the issue for me.

EDIT: Problem was unrelated. Sorry for the noise.

joseph.olstad’s picture

subscribing, sounds like a must have.

joseph.olstad’s picture

Patch #75 works well on the latest release 7.54.

kscheirer’s picture

Status: Needs review » Reviewed & tested by the community

Based on #80 and #84.

David_Rothstein’s picture

Issue tags: +Drupal 7.60 target
tabercreative’s picture

Patch #75 is not working on 7.55.

It worked fine in 7.54, but I'm getting Error 500s across the board after updating to 7.55.

joseph.olstad’s picture

We're using patch #75 on the latest release 7.55 , no issues, I do not get an Error 500 , @tabercreative , please review your core upgrade and patch procedure.

We wrote a custom script for our core upgrades, that way it's not prone to human error.

Alternatively, you can do drush up drupal and then manually re-apply your core patches.

Patch #75 is still good on the latest 7.55 it works as well as it did in 7.54

Anybody’s picture

Confirming #88

tabercreative’s picture

Apologies if I'm getting this wrong, but I don't see any of the updates to "includes/menu.inc" or "modules/menu/menu.test" in Patch #75 when I download 7.55 from here: https://www.drupal.org/project/drupal/releases/7.55

Is that not the correct location to download the core update?

joseph.olstad’s picture

@tabercreative
that is correct, this patch was not included in release 7.55
if you want to benefit from the code in the patch #75 you have to download it and apply it (patch it) against your own core.
Instructions:

cd drupal;
curl https://www.drupal.org/files/issues/drupal-1978176-menu_load_objects-75.patch > 75.patch;
patch -p1 --dry-run < 75.patch;#TEST THE PATCH, if there is no errors THEN:

patch -p1 < 75.patch;#apply the patch for real

dsutter’s picture

RTBC+ patch #75

gdaw’s picture

Patch #75 RTBC +1

hargobind’s picture

Patch #75 works like a charm on 7.56. RTBC++

Arrived here from #2071607: Saving a view causes the entire cache to be invalidated because saving a view on a large site that I manage tends to regularly take up to 30 seconds. After both patches, saving a view only takes a couple seconds.

DigitWings’s picture

I arrived at patch in #75 from #1697570: _menu_load_objects() is not always called when building menu trees. In the small site I have tested, I have not noticed any performance improvements. However it does fix a bug i was facing. Which is emptying of node titles in a book menu, when the node was edited. I have workbench moderation and transliteration modules enabled too.

le72’s picture

The patch #75 adds a bug to my build. I am using latest stable workbench moderation with Drupal 7.56. When I create a draft of a node and edit title on /node/[node_id]/draft page the code

$node = menu_get_object();
print $node->title;

prints original node title.
I did some review of code and patch. The issue comes from _menu_load_objects() function call in menu_get_object() if I return back original code

return $router_item['map'][$position];

title show correctly.

The patch does really big change of drupal core menu routing system and do not recommend to apply it to next realise at least without good testing.

joseph.olstad’s picture

Stable and workbench_moderation is a bit of an oxymoron, I've never ran workbench_moderation without several patches including core patches just to get it to work (our systems are using entity_translation , multilingual sites are a bit extra challenge.

What other core patches are you using to get workbench_moderation working ? and what patches to workbench_moderation? Assuming you're also using the 'drafty' module if it's the latest workbench_moderation?

Are you also using the 'title' module? entity_translation?

joseph.olstad’s picture

Issue tags: -Drupal 7.60 target +Drupal 7.61 target

Bumping to 7.61. This didn't make it into 7.60.

joseph.olstad’s picture

tejasvaidya01’s picture

On Drupal 7.61 #75 fixed my "Notice: Trying to get property 'title' of non-object in node_page_title()". Thanks.

tejasvaidya01’s picture

On Drupal 7.61 #75 fixed my "Notice: Trying to get property 'title' of non-object in node_page_title()". Thanks.

Anybody’s picture

Would be great to see this in the next release! Hope a core maintainer will care for that. :)

joseph.olstad’s picture

darrell_ulm’s picture

Tried the patch in #75 seems to give about a 9% speedup after running a new, not comprehensive, tests.

Fabianx’s picture

I like the patch, but I have trouble understanding what it does.

Can someone help?

joseph.olstad’s picture

The issue summary has a pretty good explanation of how it works. There's a test also included. The performance improvement is impressive! Especially for sites with a lot of menu links.

Proposed patch moves _menu_load_objects() from _menu_check_access($item, $map) to menu_unserialize().
This way, the object is loaded only when it is needed.

mr.york’s picture

Good patch. Works for me. Thank you.

darrell_ulm’s picture

Drupal 7.65 target ?!

kscheirer’s picture

Issue tags: -Drupal 7.64 target +Drupal 7.65 target

updated tag

MustangGB’s picture

Issue tags: -Drupal 7.65 target +Drupal 7.68 target
joseph.olstad’s picture

I would consider this change to be worthy of a release by and of it's own.
Why?) Because the performance improvement is huge! Especially for large complicated sites with a huge menu_tree.

Re-read this issue again, putting the core maintainer hat on.

I would like to ask Drumm from the drupal.org team to add this patch into his dev /qa and eventually prod environment and report his findings. While this patch might not ever make it into core, it would be nice if it made it into drupal.org.

Also, there was one report about a workbench moderation issue, those can be tricky.

The decider on this has to weigh a few things:
1) Does the benefit outweigh the risk?
2) If the answer is yes, the community will help resolve the issues.
3) if the answer is no, what tests might we need to add to improve our confidence?

Speaking of which: To increase our confidence in the test results for the above issue, these two should go in first so we can re-queue php 7.3 and php 5.3 tests.
#3047844: [Regression] Tests fail on PHP 5.3
#3025335: session_id() cannot be changed after session is started

MustangGB’s picture

Issue tags: -Drupal 7.68 target +Drupal 7.69 target
MustangGB’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
hargobind’s picture

@joseph.olstad based on your last comment in #111, I requeued the patch for testing across all environments.

hargobind’s picture

@joseph.olstad The two issues you mentioned as dependencies have now been committed and closed.

Additionally, the patch in #75 is passing all tests.

You suggested having Drumm take a look at this and test it out for D.O. I can't see the list of followers on this issue, but they haven't commented on this issue so far. Were you able to get in touch with them?

You mentioned workbench_moderation as a possible blocker/breaker. The module currently has ~20,000 7.x installs as of Feb 2020 (including about 5,000 installs in the unsupported 1.x release). Therefore the bug reported by @le72 in #96 would certainly affect lots of people. Unfortunately @le72 never responded back about their setup, so we don't know exactly when/how this bug might appear.

Since you mentioned have a fancy maintainer's hat 😉, what are the next steps you suggest?

Would you like to contact the maintainers of workbench_moderation to see if they have any insight on how to reproduce the bug and if they could offer a fix? I'm not suggesting that fixing a single contrib module would give us the green light to commit this patch; however, it may help us rewrite some portion(s) of this patch to make it more compatible with any contrib module that might be affected by this patch.

FWIW, I'm supporting more than 10 Drupal sites with over 275 contrib modules in total, and none of the sites seem to be affected with this patch. So I'm curious if workbench_moderation is doing something with core hooks/tables that is out of the ordinary.

joseph.olstad’s picture

@FabianX would have to review

for what it's worth, I've used this patch on many different sites with some having over 300 enabled modules including workbench_moderation and no issues.

After several years I see no credible reports of an issue with this patch and it does pass testing.

drupal.org would benefit greatly from this patch.

btw, I propose that D7 acquia support should be extended to 6 months after drupal.org is upgraded to Drupal 9. LTS support would begin at that time.

hargobind’s picture

Issue tags: -Drupal 7.70 target +Drupal 7.72 target

Moving this forward to be considered for the next release. Queued #75 for testing with 7.4.

hargobind’s picture

Issue tags: -Drupal 7.72 target +Drupal 7.74 target

Bumping

izmeez’s picture

Status: Reviewed & tested by the community » Needs work

Patch in comment #75 causes the following php notices with drupal 7.78 and php 7.4 on "page not found" whether logged in or not:

  • Notice: Trying to access array offset on value of type bool in menu_get_object() (line 1035 of includes/menu.inc).
  • Notice: Trying to access array offset on value of type bool in menu_get_object() (line 1036 of includes/menu.inc).

The lines this refers to are the ones added in the first part of this hunk:

@@ -980,8 +1030,15 @@ function _menu_link_translate(&$item, $translate = FALSE) {
  */
 function menu_get_object($type = 'node', $position = 1, $path = NULL) {
   $router_item = menu_get_item($path);
+
+  if (!is_array($router_item['load_functions'])) {
+    $router_item['load_functions'] = unserialize($router_item['load_functions']);
+  }
   if (isset($router_item['load_functions'][$position]) && !empty($router_item['map'][$position]) && $router_item['load_functions'][$position] == $type . '_load') {
-    return $router_item['map'][$position];
+    $map = $router_item['map'];
+    if (_menu_load_objects($router_item, $map)) {
+      return $map[$position];
+    }
   }
 }

This is easy to reproduce. Ensure caches are cleared then go at an url that does not exist.

The notices are the same as the failures in Drupal automatic tests.

[Edit] The following may not be related to this patch:
The patch also causes repeated php notices anytime a module is installed or uninstalled:

Notice: Undefined index: php in _system_modules_build_row() (line 1025 of modules/system/system.admin.inc).

Without the patch these repeated Notices do not appear. [/Edit]

Changing the status to needs work in light of these findings.

izmeez’s picture

Not sure how best to fix this patch to avoid php 7.4+ notices on "page not found".

joseph.olstad’s picture

@ismeez when you patch on your test environment, what line of code is 1035 ?

is it:

if (!is_array($router_item['load_functions'])) {

?

izmeez’s picture

@joseph.olstad Yes, that is line 1035 and the following line of the patch is 1036.

joseph.olstad’s picture

change that line of code to this:

if (!isset($router_item['load_functions']) || !is_array($router_item['load_functions'])) {

this will fix the glitch, suggest rerolling the patch with this

if that index is not set it's not going to be an array either so this is a safe logical change that will avoid the notice and respect the logic flow.

izmeez’s picture

@joseph.olstad Thanks. I tried the change suggested in #123 and it fixes the first notice but not the second notice when testing "page not found".

Notice: Trying to access array offset on value of type bool in menu_get_object() (line 1036 of includes/menu.inc).

This refers to the very next line in the function,

function menu_get_object($type = 'node', $position = 1, $path = NULL) {
  $router_item = menu_get_item($path);

  if (!isset($router_item['load_functions']) || !is_array($router_item['load_functions'])) {
    $router_item['load_functions'] = unserialize($router_item['load_functions']);
  }
  if (isset($router_item['load_functions'][$position]) && !empty($router_item['map'][$position]) && $router_item['load_functions'][$position] == $type . '_load') {
    $map = $router_item['map'];
    if (_menu_load_objects($router_item, $map)) {
      return $map[$position];
    }
  }
}
joseph.olstad’s picture

Oops take the ! off the isset and change || to &&

joseph.olstad’s picture

change that line of code to this:

if (isset($router_item['load_functions']) && !is_array($router_item['load_functions'])) {
izmeez’s picture

OK, thanks, that seems to do the job. Quick testing shows nothing else appears to be broken.

Here's the new patch and interdiff for review and testing.

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

looks good, back to RTBC, thanks @izmeez

izmeez’s picture

@joseph.olstad Thank you. With the patch in #127 passing all automated tests, the patch being another big performance win, support of @David_Rothstein (comment #86) and @Fabianx (comment #105 liking it and scratching his head) is it ready for a priority commit to 7.x core?

Also, is the patch in #997918-32: Menu gets rebuilt continually during install and update (and menu system stores entire, out-of-date theme objects) a companion to this patch, a duplicate, or will need a re-roll when this gets committed?

Similar questions for patch in #1277596-12: Menu system unexpectedly uses different access control for node links.

izmeez’s picture

Not sure if it's related but further to comment #119, with the new patch I am not seeing php notices on php 7.4 when a module is installed or uninstalled:

Notice: Undefined index: php in _system_modules_build_row() (line 1025 of modules/system/system.admin.inc).

The notices appear to be fixed.

joseph.olstad’s picture

This patch alone is worth a release. Performance enhancements to reduce greenhouse gases. Do it for the environment!

gdaw’s picture

RTBC +1 - With all the time and effort that has gone into this over the years it makes sense on every level to get this wrapped up.

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work

I can see the need for the performance improvement here, but the statement that "_menu_load_objects is moved from one part to another" is not true.

_menu_load_objects is NOT removed anywhere, so this might just be opting out of view access checking under certain circumstances completely.

If we want to improve performance by not loading the object, that is generally fine, BUT are we even able to check access WITHOUT loading the object - unless it is "user_permission, view content"?

And yeah I would probably change _menu_translate more directly to call something, which does not load the object, then check IF any objects need to be loaded (e.g. user_permission, 'view content' is obviously not needing the object) and then do the access check with lazy loading of the object.

This needs way less changes and hence has a higher chance of landing - unless this already happens. [needs to be benchmarked]

Benchmark idea:

Change menu router for e.g. taxonomy terms to generally allow view access instead of having "entity_access, view, %1" and see if that alone already improves the performance.

Whenever I wanted to optimize this part in the past, I usually ended up render caching the menu per role AS I knew I did not have any specific links with per user permissions.

Core cannot know that however.

Anybody’s picture

Issue tags: -Drupal 7.74 target

Removing old target, furthermore #127 needs a reroll (doesn't apply anymore) considering the points from #133!

Still an important issue.