Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve menu render performance #130

Open
wants to merge 24 commits into
base: 4.0-dev
Choose a base branch
from

Conversation

andrepereiradasilva
Copy link
Owner

@andrepereiradasilva andrepereiradasilva commented Jun 29, 2017

Pull Request for Issue # .

Summary of Changes

Improves performance and memory usage of the menu render.

Testing Instructions

TODO

Performance improvements

Test scenario:

  • Joomla 4.0 latest (no caches)
  • PHP 7.0.19
  • Clean joomla sample test pata - password forget page
  • Best results in 25 tests

Before

image

After

image

Documentation Changes Required

None.

}
elseif ($values[$key] === null)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a typo:
"// If access filter is not set, use "

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done thanks

@frankmayer
Copy link

Nice optimizations. Looking good on code review. I hope I didn't miss anything. Another pair of eyes would be good.

@@ -16,36 +16,38 @@
$id = ' id="' . $tagId . '"';
}

$pathSize = count($path);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$pathFlip = array_flip($path); ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 20, sorry did not mark correctly

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done thanks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also calling $pathReserve (note 'reserve' instead of 'reverse') $pathFlipped or $pathFlip?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right

@andrepereiradasilva
Copy link
Owner Author

andrepereiradasilva commented Jun 29, 2017

@frankmayer this is only in my repo i did not submited a PR to joomla yet 😄

@frankmayer
Copy link

:D ah yes, didn't notice that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants