Problem/Motivation

We are currently using menu_item_extras with layout_builder to build a mega menu.
After placing a block in layout builder, I noticed the build() function of that block was called 3 times per page load.

So it seems the entities are being built several times:
First build from \Drupal\menu_item_extras\Service\MenuLinkTreeHandler::getMenuLinkItemContent where it runs:

$render_output = array_merge(
  $view_builder->view($entity, $view_mode),
  EntityViewDisplay::collectRenderDisplay($entity, $view_mode)->build($entity)
);

It is calling EntityViewDisplay::collectRenderDisplay($entity, $view_mode)->build($entity) but the #pre_render callback - coming from the output of $view_builder->view($entity, $view_mode) - will build the entity anyway when rendering it in the twig template.

Second and 3rd build from templates/menu-levels.html.twig

{% set rendered_content = item.content|without('') %}
{% if rendered_content|render %}
  {{ rendered_content }}
{% endif %}

So it renders twice there just to check emptiness?
Can't we use something like:

{% set rendered_content = item.content|without('')|render %}
{% if rendered_content %}
  {{ rendered_content }}
{% endif %}
CommentFileSizeAuthor
#4 perf_hit_on_render-3227764-4.patch2.33 KBherved
#2 perf_hit_on_render-3227764-2.patch1.25 KBherved

Comments

herved created an issue. See original summary.

herved’s picture

herved commented
Status: Active » Needs review
StatusFileSize
new perf_hit_on_render-3227764-2.patch1.25 KB

Here's a first draft.

I'm not quite sure why EntityViewDisplay::collectRenderDisplay($entity, $view_mode)->build($entity) is there ATM in MenuLinkTreeHandler::getMenuLinkItemContent since it it called when the #pre_render runs (when rendering it in the twig).
So let me know if I'm missing anything.
Maybe the entity needs to be built to do some checks before it is sent to the twig?? If so then maybe we can call the #pre_render there if really needed then unset it.

I can't see any regression with this.
Could you test on your side if this works as before?

Thanks a lot

herved’s picture

herved commented
Status: Needs review » Needs work

From the git log I now see that EntityViewDisplay::collectRenderDisplay($entity, $view_mode) was added by #3034814: Output individual fields or children only to be able to access field values from menu-levels.html.twig.
I'm not sure this is the best idea as there may be alternatives. If not, I think it's preferable to call the #pre_render then unset it.

There is also someting intriguing me, why is the cache being unset there? Seems like #3106743: New second level items only show after flushing cache has something to do with it but why wouldn't we rely on the cache metadata returned from $view_builder->view() in the first place?

I'll investigate further when I have time but any info from a maintainer is welcome.

herved’s picture

herved commented
StatusFileSize
new perf_hit_on_render-3227764-4.patch2.33 KB

New version with
- a call to EntityViewBuilder::build() and the #pre_render removed so #3034814: Output individual fields or children only will still work
- I restored the unset($render_output['#cache']); from #3106743: New second level items only show after flushing cache as cache keys would cause issues and if I understand correctly. The cache is handled by menu_item_extras_link_item_content_active_trails
- added some comments for documentation

BTW is there a valid reason why menu-levels.html.twig does a without('')?

herved’s picture

herved commented
Status: Needs work » Needs review
ozin’s picture

ozin
Primary language Ukrainian
commented
Assigned: Unassigned » ozin

HI @herved!
Thanks for the patch, I will check it as soon as possible.
Regarding the without('') I belive it filter out empty render, but I have to double-check. Test example
item.content = ['#markup' => '']

  • ozin committed e617bab on 8.x-2.x authored by herved 
    Issue #3227764 by herved: Performance hit when rendering menu items (...
ozin’s picture

ozin
Primary language Ukrainian
commented
Status: Needs review » Fixed
herved’s picture

herved commented

@ozin, thanks for the update.

I'm still curious about the cache being unset there.

+++ b/src/Service/MenuLinkTreeHandler.php
@@ -81,21 +81,22 @@ class MenuLinkTreeHandler implements MenuLinkTreeHandlerInterface {
     unset($render_output['#cache']);

This originates from #3106743: New second level items only show after flushing cache.
Unsetting the cache keys and bin as follows seems to make more sense and solve that issue as well but I'm not sure of the implications and whether there would be any performance gain:
unset($render_output['#cache']['keys'], $render_output['#cache']['bin']);

I didn't have time to dig into it yet.
Any thoughts on that?
Should we create a new issue to investigate this?

Thanks

Status: Fixed » Closed (fixed)

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