New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimizing menu item parent calculation #8863

Merged
merged 10 commits into from Oct 18, 2016

Conversation

Projects
None yet
10 participants
@Hackwar
Member

Hackwar commented Jan 8, 2016

mod_menu checks for every menu item if the item is a parent of another menu item. The method to do that via $menu->getItems() is pretty expensive and can create great delays on bigger sites. The proposed code will reduce this immensely by only checking those menu items that can have a child due to lft and rgt differing by more than one. Unfortunately that will not automatically mean that a menu item actually has children, since children could be present, but unpublished or in a different language. In those cases lft and rgt differ, but there is no child item anyway.

This will not change existing sites, but will mean a performance improvement. So the best way to test this would be to compare execution times on sites with a lot of menu items (>500). Another way would be a code review by a maintainer. 馃槃

This came up because of the insistence of @volandku in the Joomla IRC channel on Freenode who made me look into this. Thank you for being persistent. 馃槃

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 9, 2016

I have tested this item 馃敶 unsuccessfully on 51de1eb

I get this php notices:

Notice: Undefined property: stdClass::$rgt in /path/to/joomla-cms/modules/mod_menu/helper.php on line 77
Notice: Undefined property: stdClass::$lft in /path/to/joomla-cms/modules/mod_menu/helper.php on line 77

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.
@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 9, 2016

@Hackwar, instead of doing this:

foreach ($items as $i => $item)
{
    /* Code removed for simplifying */
    $item->deeper     = false;
    $item->shallower  = false;
    $item->level_diff = 0;

    if (isset($items[$lastitem]))
    {
        $items[$lastitem]->deeper     = ($item->level > $items[$lastitem]->level);
        $items[$lastitem]->shallower  = ($item->level < $items[$lastitem]->level);
        $items[$lastitem]->level_diff = ($items[$lastitem]->level - $item->level);
    }

    $item->parent = (boolean) ($item->rgt > $item->lft + 1) && $menu->getItems('parent_id', (int) $item->id, true);
    /* Code removed for simplifying */
}

if (isset($items[$lastitem]))
{
    $items[$lastitem]->deeper     = (($start?$start:1) > $items[$lastitem]->level);
    $items[$lastitem]->shallower  = (($start?$start:1) < $items[$lastitem]->level);
    $items[$lastitem]->level_diff = ($items[$lastitem]->level - ($start?$start:1));
}

Can't we do something like this?:

foreach ($items as $i => $item)
{
    /* Code removed for simplifying */
    $item->deeper     = false;
    $item->shallower  = false;
    $item->level_diff = 0;
    $item->parent     = 0;

    if (isset($items[$lastitem]))
    {
        $items[$lastitem]->deeper     = ($item->level > $items[$lastitem]->level);
        $items[$lastitem]->shallower  = ($item->level < $items[$lastitem]->level);
        $items[$lastitem]->level_diff = ($items[$lastitem]->level - $item->level);
        $items[$lastitem]->parent     = ($items[$lastitem]->level_diff == -1);
    }
    /* Code removed for simplifying */
}

if (isset($items[$lastitem]))
{
    $items[$lastitem]->deeper     = (($start?$start:1) > $items[$lastitem]->level);
    $items[$lastitem]->shallower  = (($start?$start:1) < $items[$lastitem]->level);
    $items[$lastitem]->level_diff = ($items[$lastitem]->level - ($start?$start:1));
    $items[$lastitem]->parent     = ($items[$lastitem]->level_diff == -1);
}

In other words, can't we just don't use the getItems to calculate if current menu item is a parent or not and use the already calculated level_diff to do that.

If we can do that we get even further performance improvements.

I run a test with a menu with several levels (thank for batch processing :)) and the for cycle got from:

  • 25 published items: ~6.5 ms to ~4.7 ms.
  • 50 published items: ~15 ms to ~9 ms.
  • 100 published items: ~30 ms to ~19 ms.
  • 200 published items: ~78 ms to ~38 ms.
  • 400 published items: ~232 ms to ~76 ms.
  • 800 published items: ~730 ms to ~155 ms.
  • and so on ...
@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 9, 2016

This PR has received new commits.

CC: @andrepereiradasilva


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jan 9, 2016

First of all, sorry for the broken code. Yes, you are right, your proposal is even better. I added your code. I'm setting the parent to false by default to have it set at all times.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 9, 2016

I have tested this item successfully on df6acd2

Thank you for find that needed performance optimization.
This can have some performance impact on sites with a lot of menu items.

Correct me if i'm wrong but the last element can't be the parent of anything, since it's the last.
That is why you didn't put the $items[$lastitem]->parent = ($items[$lastitem]->level_diff == -1); in the setting of last item after the for cycle as i suggested.
Makes sense.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@HLeithner

This comment has been minimized.

Contributor

HLeithner commented Jan 21, 2016

I have tested this PR 馃敶 unsuccessfully on df6acd2.

Simple test, I take default installation added 5 Menu Items:

  • Item 1
    • Item 1a
  • Item 2
    • Item 2a
      • Item 2b

Module with "Show Sub-menu Items" off shows on active "Item 1" a difference at the last item, in this case "Item 2".

Without patch: parent = true
With patch: parent = false

If "Show Sub-menu Items" is on the "Item 2" parent value is correct (true).

Also if "Show Sub-menu Items" is on the the complete testcase is correct.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 26, 2016

@EFERIS we know the last item parent value can be not ok. See my comment when i tested.

Correct me if i'm wrong but the last element can't be the parent of anything, since it's the last.
That is why you didn't put the $items[$lastitem]->parent = ($items[$lastitem]->level_diff == -1); in the setting of last item after the for cycle as i suggested.
Makes sense.

But in what that affected your test? Did the menu not appeared correctly?

@HLeithner

This comment has been minimized.

Contributor

HLeithner commented Jan 26, 2016

@andrepereiradasilva sure because the li-tag for the last element doesn't get the "parent" class, that could lead to a visual problem if you use this class (the only way) to mark the link as parent item.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 26, 2016

Ok, let's wait for @Hackwar to solve that.

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 27, 2016

This PR has received new commits.

CC: @andrepereiradasilva


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jan 27, 2016

This PR has received new commits.

CC: @andrepereiradasilva


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jan 27, 2016

Please test this again. This should solve the previous issues.

@HLeithner

This comment has been minimized.

Contributor

HLeithner commented Jan 28, 2016

Hi,

while testing this I think I came to a better solution.

Add this to the beginning of the foreach loop before filtering the unseen items.

$item->parent = false;
if (isset($items[$lastitem])
  && $items[$lastitem]->id == $item->parent_id)
{
  $items[$lastitem]->parent = true;
}

btw. is it normal that the joomla patch tester overrides the complete file with a incompatible version?

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 28, 2016

I think the prior solution (before revert) works, if you add this line inside the if after the for cycle (https://github.com/Hackwar/joomla-cms/blob/df6acd259cb3062ad0ec1b1499f72aa6cea3e10e/modules/mod_menu/helper.php#-L126-L131) to set the last parent:

$items[$lastitem]->parent     = ($items[$lastitem]->level_diff == -1);
@HLeithner

This comment has been minimized.

Contributor

HLeithner commented Jan 28, 2016

@andrepereiradasilva I tested this first, but no that will not work because we don't have the child at this point. if you can please try my solution.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 28, 2016

ok i will check. thanks

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 28, 2016

@EFERIS i tried you solution and it worked. And without the getItems. Good work.

How did i test:

foreach ($items as $i => $item)
{
    $item->parent = false;
    if (isset($items[$lastitem]) && $items[$lastitem]->id == $item->parent_id)
    {
        $items[$lastitem]->parent = true;
    }
    // code removed for brevity
}

@Hackwar can you check? With @EFERIS solution i think we can get rid of getItems and with that improving performance even further.

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Apr 12, 2016

@Hackwar any comment on the proposed improvement from @EFERIS would be good to move this PR forward if we can


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@roland-d

This comment has been minimized.

Contributor

roland-d commented Jun 24, 2016

@Hackwar Another nudge so we can move this PR forward.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jun 25, 2016

I don't know which one is @EFERIS. So far this PR is complete the way it is.

@HLeithner

This comment has been minimized.

Contributor

HLeithner commented Jun 27, 2016

@Hackwar sorry I renamed the github Username

Instead of trying to get the ->parent value based on the level_diff its better to set it at the beginning before we delete the information.

Add the following code to the beginning of the foreach loop at Line 57, see comment from @andrepereiradasilva

    $item->parent = false;
    if (isset($items[$lastitem]) && $items[$lastitem]->id == $item->parent_id)
    {
        $items[$lastitem]->parent = true;
    }`
@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jun 28, 2016

This PR has received new commits.

CC: @andrepereiradasilva


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jun 28, 2016

I've added the change like you proposed @HLeithner.

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jun 28, 2016

This PR has received new commits.

CC: @andrepereiradasilva


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@HLeithner

This comment has been minimized.

Contributor

HLeithner commented Jun 28, 2016

One thing, adding ", m.lft, m.rgt" to the query is no longer necessary.

@@ -54,6 +54,12 @@ public static function getList(&$params)
{
foreach ($items as $i => $item)
{
$item->parent = false;
if (isset($items[$lastitem]) && $items[$lastitem]->id == $item->parent_id)

This comment has been minimized.

@roland-d

roland-d Jun 29, 2016

Contributor

CS: We need an empty line before the if statement.

@joomla-cms-bot

This comment has been minimized.

joomla-cms-bot commented Jun 29, 2016

This PR has received new commits.

CC: @andrepereiradasilva


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jun 29, 2016

One thing, adding ", m.lft, m.rgt" to the query is no longer necessary.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Aug 23, 2016

Menu with 500+ items

Before patch
image

After patch
image

So aprox. half the time

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Aug 23, 2016

I have tested this item successfully on f1aa5a6

as comment above


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@gwsdesk

This comment has been minimized.

gwsdesk commented Sep 6, 2016

I have tested this item successfully on c4d5c8c

I have tested this on a stock Joomla distro (3.6.3) will "Learning Sample data" in 4 languages and it has a visible faster loading time compared to pre-patch


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

@laoneo

This comment has been minimized.

Member

laoneo commented Sep 7, 2016

This issue has two successful tests.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8863.

wilsonge and others added some commits Oct 5, 2016

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 5, 2016

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/8863.

@joomla-cms-bot joomla-cms-bot added the RTC label Oct 5, 2016

@wilsonge wilsonge added this to the Joomla! 3.6.4 milestone Oct 5, 2016

@wilsonge wilsonge added RTC and removed RTC labels Oct 5, 2016

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Oct 17, 2016

Milestone changed to 3.7 as it is now not planned to have a 3.6.4 release

@rdeutz rdeutz merged commit 5b0d2cf into joomla:staging Oct 18, 2016

2 checks passed

continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@brianteeman brianteeman removed the RTC label Oct 18, 2016

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