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

Optimizing JMenu::getItems() #8864

Merged
merged 1 commit into from Jan 10, 2016

Conversation

Projects
None yet
6 participants
@Hackwar
Member

Hackwar commented Jan 8, 2016

This is related to #8863. I looked into JMenu::getItems() to see if this could be optimized further. While this is a small optimization, it still means that the count operation only has to be run once instead of for each menu item. The number of attributes does not change during this method call, so we can move this to the beginning of the method instead of it being part of the for-loop.

Thanks @volandku for this one, too.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 9, 2016

I have tested this item successfully on 5f0b23d

Thanks.
IMHO, all performance optimizations (even small ones), and especially the ones in parts of Joomla that are used in every sites, are a +100.


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

@Fedik

This comment has been minimized.

Contributor

Fedik commented Jan 9, 2016

it would be performance loss in case of:

for ($i = 0; $i < count($attributes); $i++)

In existing code all fine, for me. As I remember the first part of the for() loop is not called each cycle.
so I do not see the profit in suggested changes, just moving the code around? 😉

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 9, 2016

Yeah, i checked the code diff and i think @Fedik is right.

@sovainfo

This comment has been minimized.

Contributor

sovainfo commented Jan 9, 2016

Suggest to look again: The for loop you are referring to is within the for loop of _items!

@Fedik

This comment has been minimized.

Contributor

Fedik commented Jan 9, 2016

@sovainfo okay okay, I take my words back 😄

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Jan 9, 2016

Ups, must be sleeping, yes, there is a for before.
thanks @sovainfo.
So the test remains ok. :)

@sovainfo

This comment has been minimized.

Contributor

sovainfo commented Jan 9, 2016

Not to worry, it was the first thing I checked. Viewing the complete source helps to put things in perspective. The reputation of Hannes (Hackwar) also helps, of course.

@Fedik

This comment has been minimized.

Contributor

Fedik commented Jan 9, 2016

I have tested this item successfully on 5f0b23d


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

wilsonge added a commit that referenced this pull request Jan 10, 2016

Merge pull request #8864 from Hackwar/patch-2
Optimizing JMenu::getItems()

@wilsonge wilsonge merged commit 3576e2f into joomla:staging Jan 10, 2016

2 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Jan 10, 2016

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