Closed (fixed)
Project:
Menu Breadcrumb
Version:
8.x-1.9
Component:
Documentation
Priority:
Minor
Category:
Task
Assigned:
Reporter:
Created:
23 Sep 2019 at 10:25 UTC
Updated:
26 Feb 2020 at 11:34 UTC
Jump to comment: Most recent
We found a regression on ram consuming for new node translations on sites with big menus.
When a editor adds a new translation on a site with content_moderation, several languages and a menu with around 800 menu links, there is a memory_limit error with 128M ram. The example could look like very edge case, but before the 1.9 release this memory limit is not reproduced, so clearly there is a regression and a optimization opportunity.
The responsible for that regression is the following commit:
https://git.drupalcode.org/project/menu_breadcrumb/commit/364e3f90c59122...
As quick fix, setting the "derived_active_trail" to "true" on the menu_breadcrumb.settings configuration entity brings the error away.
That means the following code:
$menuActiveTrail = new MenuActiveTrail($this->menuLinkManager, $route_match, $this->cacheMenu, $this->lock);
$trail_ids = $menuActiveTrail->getActiveTrailIds($menu_name);
is more perfomative compared with the following code
$trail_ids = $this->menuActiveTrail->getActiveTrailIds($menu_name);
Using dependency injection is best practice and should be used, so we need more investigation and bench-marking to understand why using the injected dependency consume more ram than instancing the object several times.
Comments
Comment #2
rphair commentedThanks @akalam - I agree it sounds like an optimisation opportunity, but probably not for this module. Let's leave this one open for a while to see what kind of attention it gets, though I am already planning on punting this upstream to the active_trail issue queue.
I think you would be the best choice to file that issue because of your use case & ready testing environment, but I'll submit the issue if not: one way or another I want to see this issue delivered into the correct forum.
I'm not optimistic about our ability to attract their attention. An earlier module issue (#2820206: No active trail for views with a NULL parameter) pointed out such a core problem with active_trail and we provided a lot of supporting data, currently without a response for nearly 3 years (#2820751: Menu Active Trail is empty for built-in view pages). I'm not linking these as related issues since they are only a case in point.
In the meantime I'm setting the priority to "minor" due to the relatively low number of sites that would be affected by this issue. Just because there is an incidental workaround based on a different derivation of active_trail (option setting introduced in #3036501: Service menu.active_trail dependency injection doesn't mean that the memory limit bug is caused by this module's code.
I'd like to hear other points of view but according to my understanding of the Drupal paradigm we must insist on a fix coming from core rather than reinventing active_trail module by module.
Still, depending on the discussion outcome we might patch the documentation to notify admins that RAM utilisation is more efficient (and less likely to blow up) with the "derived active trail" option selected.
Comment #4
rphair commentedI've decided the best way to deal with this issue is through documentation, so the sites potentially affected can anticipate this problem & report their findings. That means putting a note by the option setting with a reference to this issue. I realise that's a breach of protocol, but this provides the greatest opportunity to provide evidence to the active_trail team to fix whatever bug there may still be in core.
Anyone who wants to follow up that way should please reference this issue, which will stay highlighted in the online documentation itself until we get some consensus or official report that the injected active_trail doesn't blow up in menu_breadcrumb under the same conditions.
Comment #5
akalam commentedThanks for taking it in account!