Performance issue caused by rendering in access check

I see there was a feature added with some config for display_empty which makes this harder to revert.

We can at least change the order here so build is only called if needed - but that will still leave the issue there for anybody with display_empty FALSE.

Honestly I'm confused to how the block is visible when empty - I need some steps to reproduce because when there is no active child and the block returns an empty array nothing is rendered for me. Either way, if this logic is needed it should be moved out of an access check and into the build method.

Setting MR to ready.

I've captured where I believe the display empty bug is - in that if the tree is empty a render array is being returned. Now if display_empty is false it just returns empty here.

There are a couple of existing empty array return points in build that I believe are not compatible with the display empty option - this likely needs further testing but that is not the point of this issue, this is to remove the rendering in the access check.

The test passes on the MR and the test only changes pass - so I believe I have not reverted the empty behaviour intent here - but it probably needs more testing from people who previously needed such behaviour.

I'll note that returning empty did not work previously for some people - https://www.drupal.org/project/menu_block/issues/3271218#comment-14499696 🐛 Menu block renders when tree is empty as of 8.x-1.8 [regression] RTBC

But I don't know the steps to reproduce that since it worked for others, if we can get conclusive steps to reproduce I have time to write a test for it.

Thanks for taking this on, I know for performance it’s nice to back the claim up with hard data, have you ran xhprof or did something point to this as a problem?

I’ve wrote my fair share of micro-optimization patches in my day;)

Yes - I noticed this while investigating the general performance of search pages on a site with a very large menu.

A few factors on the site lead it to be quite noticeable - which other sites may be susceptible to. Firstly since menu block hides the system block when adding blocks, we've ended up using menu block for all menus - which includes the very large main menu block. In addition to this, there were additional menu blocks for the footer menu, header menu and sidebar navigation block (which is also large as the search page in question is within a sub section of the site). All up that means a lot access checks for the menu. (It turns out, we aren't event using any of the features of menu block for the main menu - so editing the config to us the system block is also a solution for us). I guess because menu block hides the system block, many other sites may be using menu block for their primary navigation block.

Using php-spx it showed that it was using around 200 -> 250ms in the menu block block access method - so for us not a micro optimisation, this is a big chunk of the request. Again that is in the context of 5 menu blocks and a large menu - not all sites will have this issue.

Both the patch and the approach of changing to config to use system menu blocks improved page speeds by around .3s where the page is unached but the block cache is used (and no noticeable change in response time for when the block is uncached). There is a benefit of lower memory use as well given the in memory build cache is removed.

I don't think that time will be applicable for all sites, and I haven't profiled on a clean install - but for me it shows enough that its important to fix - and given the performance issue was already previously raised it will be beneficial for all.

My concern really is only that the people reported the previous patch in the related issue didn't work for them.

If we absolutely need to keep the option of using block access for this, I think the 'display_empty' behaviour should be changed so we have to explicitly opt in to the access check.

I’ll also be working on a large menu for an intranet soon, so I’ll definitely test this out. Thanks for the detailed response!

Ideally, we can find a solution that makes everyone happy. If there’s a relatively small change (like the one you made by switching conditions), those are easy wins, though they might not fully align with what you’re aiming for.

Adding latest MR diff as static patch file for composer.