prevent KnpMenu from loading the entire menu/route/content tree

[lsmith77]

see https://groups.google.com/forum/?fromgroups=#!topic/symfony-cmf-devs/ikJ9jpomzaQ and https://groups.google.com/forum/?fromgroups=#!topic/symfony-cmf-devs/otAht7VS04Y

i have classified this as a bug as it makes it impossible to use our menu bundle in larger setups.

i was able to reproduce this issue. it doesnt really cause a lot of problems with a few nodes, but obviously if you start having a large tree, then its a big issue.

we need to investigate this .. i am not yet sure if it just loads content/routes that are referenced in the menu or if it just flat out loads everything. i would doubt that.

potentially its impossible to prevent loading all the menu items with KnpMenu 1.x, but can hopefully ensure that at least route/content nodes are lazy loaded. i dont know the menu system that well yet.

@uwej711 you havent run into this issue yet?

@petesiss's app is suffering severely because of this, even with node caching enabled

@dbu i could come by fribourg the second half of next week if necessary.

[uwej711]

Basically the menu item needs the content item and the route item to generate the proper link for the menu. The content actually ties together the menu and the routes, so as long as the whole menu tree is created as KnpMenu does currently everything is pulled from the repository.

In my small application I have only little content (actually only two menus with 5 and 4 nodes) and I use ESI caching.

While at first I thought having hundreds of menu items is nothing that makes sense I remembered an application we did (not with the cmf) which had about 200 to 300 items ...

But with that many items I guess your only chance is really caching the complete menu - if you use some sort of highlighting the current item you end up with 200 - 300 cache entries but this is still much better than handling so many objects on each request and generating the routes for the object - not tested but I think even with a menu in code you burn some cycles just generating the urls ...

KnpMenu first builds the menu and then renders the menu. Building the menu involves generating the url and therefore loading content objects and route objects (look at Knp\Menu\MenuFactory::createFromNode and Symfony\Cmf\MenuBundle\ContentAwareFactory::createItem).

So even with optimizations in KnpMenu you can easily have use cases with lots of menu items and you need to cache the menu.

[lsmith77]

Ah ok I get it now. So as part of building the menu graph, KnpMenu will need to generate the uri etc of all nodes that could possibly appear in the tree. So it seems the only solution would be a totally rewrite of KnpMenu, where it explicitly would only build those parts of the menu graph that will actually be displayed. Obviously when building a sitemap it would then still touch everything, but that is ok in that case since everything is going to be displayed.

@stof: so the gist of this is I guess is that if you have a large menu graph (especially one that isnt hardcoded), its imperative to also support a mode that only loads the parts of the tree that will be displayed. hopefully its not too late to handle this approach in 2.0.

@petesiss: so i fear for now the only solution is ESI caching.

@uwej711: for ESI caching I guess it would also be helpful if one could generate a version of the menu for each depth level without any active tab and then simple make the given tab active from outside of the ESI block.

[stof]

@lsmith77 I'm want to work on it, indeed. I will look at it tomorrow (I spent today on symfony itself as you noticed)

[uwej711]

@petesiss: couldn't you create another menu with less entries for the regular pages and one with all for the site map?

[petesiss]

@uwej711 I can see that would help, but not sure how practical it is given it would need to be managed seamlessly still by clients with the CMS. Will have a think...

In general, does it make sense to continue with a data structure where the content nodes are required to build the menu? Surely it should be possible to have all required properties in the menu and route nodes? That alone would potentially reduce lookups by 30%.

[uwej711]

The idea in the first place is to have menu, routes and content separated to make things flexible. I think it would be possible to have the menu items reference the routes directly but not sure what change that means for the routing stuff (currently the router can create an url for content, if we change this structure, i.e. have menu reference routes, the router will also have to be able to generate routes from route items, which should be possible). But caching would be your best bet anyway I guess ...

[petesiss]

@uwej711 how do you deal with setting current in your cached menu fragments?

[uwej711]

I created a controller with a parameter currentUri, Now in the template I generate the currentUri with the twig path function and pass it to the render call:

{% render 'portal.cmf_controller:navigation' with { 'currentUri': path('some_route', { 'param': current_thing.someProperty }) }, { 'standalone': true } %}

[dbu]

yep, the idea of generating the route from content was to avoid redundancy. we have 3 things that are together, and if we had them all cross-reference storing updates would become very tricky. simplecms bundle stuffs everything together, for single lang pages with simple menu structures, its probably the best (aka most efficient) solution.

that said, menu items can also store the route but i think they currently prefer the content if they have it.

[dantleech]

I have previously solved this type of problem by caching the dynamic application routes in a flat array, just like symfony does (or has done). So that when the menu is rendered it doesn't need to do any DB lookups for generating the menu item routes. Would something like that fix this problem?

[stof]

I have 2 questions related to this issue:

• does the CMF need to render the current_ancestor class (or a different class name of course as it can be changed) on nodes which are an ancestor of a current node ? Or is it not necessary ? If it is needed, I fear there will be no way to optimize things as applying this class requires looking at the subtree to check whether it contains an item matched as current.
• does the CMF need to build a breadcrumb based on its menu ? If if yes, can it know the path in the menu tree to reach the node for which the breadcrumb should be rendered or does it need to rely on the matcher to find the current node and render the breadcrumb for it ?

[lsmith77]

well i think there will be cases where its needed. however 1) it should be an option 2) it should stop reading once it has found something .. right now it just reads the entire tree .. always.

[stof]

@lsmith77 Yeah. I have an idea to avoid loading all the tree (unless you actually need to render it of course). but these are the 2 cases I see where the optimization would break.
The first case is easy (it simply means allowing to pass null as class name and disable the check in this case)

For the second case, even if it stops reading the tree when it founds something, it could still need to load everything in case the curent item is the last one found when traversing the tree.
But in the case of the CMF, the retrieval of the breadcrumb target could potentially be optimized: you will probably be able to find easily the current item in the DB (it is the one related to the content object you are rendering, no need to run all the matchers to find it in this case), and then it should be possible to write a query giving you the path to this node in the tree (i.e. an array like ['top-level child name', 'second-level child', 'current item name'] in case the current node is at a depth of 3 in the tree). MySQL would have some pain getting it (it sucks are recursive relations) but PHPCR probably can get it easily as it deals with hierarchical structures.
In this case, the traversal of the menu to build the breadcrumb would be far more efficient, as it could go directly to the right item instead of having to traverse the whole tree to find it. Of course, in case the current node cannot be found based on the content node, the logic could fallback to the full menu traversal, to allow finding an item marked as current based on some other voter.

Of course, this second case is not relevant in case you don't display a breadcrumb, but I think it is a common need for the CMF.

[lsmith77]

were the underlying issues for this addressed in KnpMenu 2.0.0? @Nek- @stof ?

[Nek-]

@lsmith77 I imagine you're thinking about the matchingDepth option, you can set it to 0. You will not have the current_ancestor class anymore but it's a great optimization.

https://github.com/KnpLabs/KnpMenu/blob/master/doc/01-Basic-Menus.markdown#other-rendering-options

[lsmith77]

@Nek- no, though this might help. with KnpMenu 1.x even if you specified a depth, it still read the entire tree to render.

ie.

knp_menu_render(' AcmeDemoBundle:Builder:mainMenu', {'depth': 2, 'currentAsLink': false})

this would still end up reading the entire menu tree into memory, which is fine if the tree is defined in a yaml file maybe but not if it comes from the database.

[Nek-]

The builder is defined by you (the user of KnpMenuBundle) and you can pass some options as third parameter by using the knp_menu_get twig function before the knp_menu_render.

Why not using an option of depth ?

[Nek-]

Another solution could be to create a MenuProxyItem that will be activated only when the item need theses children.

But IMO the best solution is to create a big query to retrieve all the menu.

In facts all this solutions are not KnpMenu related. But choose one and I will try to help or make a PR.