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

LPS-33468 - Icon taglib JavaScript performance improvements #1066

Closed
wants to merge 33 commits into from

Conversation

Projects
None yet
@jonmak08
Copy link

commented Mar 13, 2013

Hey Nate,

Attached are the new performance improvements to the icon taglib. These updates can be found at http://issues.liferay.com/browse/LPS-33468.

NOTE: DynaTrace performance profiling data can be found at https://docs.google.com/spreadsheet/ccc?key=0AsMk027ozTuBdHVHS3hVdjNrQXNKT2g4OHlmVWRwTVE#gid=0.

Please let me know if you have any questions.

Thanks!

  • Jon Mak

ealonso and others added some commits Mar 12, 2013

LPS-33622 Permission over model resources at portal level are include…
…d at site level in the roles and permissions management
LPS-32742 Include the implementation of the registerResources method …
…defined in the original HttpService specs. Included a couple of useful methods which will be used in the future
LPS-32742 See Portal#PATH_*, none of them end with a /. So we will be…
… consistent in the var value so that it also returns just /o. That means we'll have to append a trailing / more often, but it's better because it's more consistent.
LPS-32400 Since the method now is hard coded to "function", the metho…
…d name is really not appropriate. Make it so that we can reuse this method later and rename it appropriately.
@natecavanaugh

This comment has been minimized.

Copy link
Owner

commented Mar 15, 2013

Hey guys,
I'm going to err on the side of conservative here and not pull in these changes. Mainly, the reason is because the overall change isn't drastic enough, IMO, to warrant making changes. When I look at the times, the overall distribution seems like it executes more often at a longer execution time after the changes (for instance, look at the Javascript column:
Before:
2 2000
1 1800
2 1700
3 1600

After:
2 2000
5 1700
1 1600

So on the whole, it more often executes at a slightly higher range. Not drastic, but it just doesn't seem worth the risk of adding a bug.

If you guys want, we can discuss more next week when I'm back in town.

Thanks guys,

@blzaugg

This comment has been minimized.

Copy link

commented Mar 19, 2013

@natecavanaugh, I'm fine if the performance gain isn't enough to justify the change, but I believe these numbers are just for Marc's changes.

@marclundgren, could you confirm this?

@blzaugg

This comment has been minimized.

Copy link

commented Mar 19, 2013

But looking over my own data, my changes only save 40-80 ms. So it's definitely not a noticeable change.
https://docs.google.com/a/liferay.com/spreadsheet/ccc?key=0An23mPyxEOvOdHB4d3BmRFctZWwtZUhDT2Q1SlVhWkE#gid=12

@marclundgren

This comment has been minimized.

Copy link

commented Mar 19, 2013

Ahh yes, @blzaugg is correct.

I created this profile to compare the effect of Byran's changes (1244ee7 + 2df4c71) vs my changes on top of his (a5a0b77). The "no changes" column were profiles containing Byran's changes only. In hindsight, I could have been much more clear. The "changes" column has my commit.

@natecavanaugh @blzaugg I feel that we should create a profile report for just Byran's changes (if we haven't already) before we decide on the fate of this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.