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

Open
lsmith77 opened this Issue Oct 13, 2012 · 18 comments

Comments

Projects
None yet
9 participants
@lsmith77
Member

lsmith77 commented Oct 13, 2012

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

This comment has been minimized.

Show comment
Hide comment
@uwej711

uwej711 Oct 13, 2012

Member

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.

Member

uwej711 commented Oct 13, 2012

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

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Oct 13, 2012

Member

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.

Member

lsmith77 commented Oct 13, 2012

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 13, 2012

Contributor

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

Contributor

stof commented Oct 13, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@uwej711

uwej711 Oct 13, 2012

Member

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

Member

uwej711 commented Oct 13, 2012

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

@petesiss

This comment has been minimized.

Show comment
Hide comment
@petesiss

petesiss Oct 13, 2012

Contributor

@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%.

Contributor

petesiss commented Oct 13, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@uwej711

uwej711 Oct 13, 2012

Member

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 ...

Member

uwej711 commented Oct 13, 2012

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

This comment has been minimized.

Show comment
Hide comment
@petesiss

petesiss Oct 14, 2012

Contributor

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

Contributor

petesiss commented Oct 14, 2012

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

@uwej711

This comment has been minimized.

Show comment
Hide comment
@uwej711

uwej711 Oct 14, 2012

Member

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 } %}
Member

uwej711 commented Oct 14, 2012

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

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Oct 15, 2012

Member

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.

Member

dbu commented Oct 15, 2012

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

This comment has been minimized.

Show comment
Hide comment
@dantleech

dantleech May 3, 2013

Member

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?

Member

dantleech commented May 3, 2013

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?

@lsmith77 lsmith77 referenced this issue in symfony-cmf/symfony-cmf-docs Oct 29, 2013

Open

explain how to cache menu blocks with ESI #312

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Nov 22, 2013

Contributor

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 ?
Contributor

stof commented Nov 22, 2013

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

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Nov 22, 2013

Member

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.

Member

lsmith77 commented Nov 22, 2013

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Nov 22, 2013

Contributor

@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.

Contributor

stof commented Nov 22, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Aug 12, 2014

Member

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

Member

lsmith77 commented Aug 12, 2014

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

@Nek-

This comment has been minimized.

Show comment
Hide comment
@Nek-

Nek- Sep 4, 2014

@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

Nek- commented Sep 4, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Sep 4, 2014

Member

@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.

Member

lsmith77 commented Sep 4, 2014

@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-

This comment has been minimized.

Show comment
Hide comment
@Nek-

Nek- Sep 4, 2014

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- commented Sep 4, 2014

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-

This comment has been minimized.

Show comment
Hide comment
@Nek-

Nek- Sep 5, 2014

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.

Nek- commented Sep 5, 2014

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.

@lsmith77 lsmith77 added this to the 1.3 milestone Jun 17, 2015

@wouterj wouterj added the wip/poc label Jan 9, 2016

@lsmith77 lsmith77 added review and removed wip/poc labels Jan 9, 2016

@wouterj wouterj modified the milestones: 2.0, 2.1 Jun 18, 2016

@ElectricMaxxx ElectricMaxxx modified the milestones: 2.2, 2.1 Nov 28, 2016

@wouterj wouterj added wip/poc and removed review labels Jan 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment