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

[JENKINS-18377] Cache Role#hashCode to speed up RoleMap#getRolesHavingPermission #28

Conversation

@olivergondza
Copy link
Member

commented May 30, 2017

I have experienced severe slowdown caused by following stacktrace:

at java.util.AbstractSet.hashCode(AbstractSet.java:126)
at com.michelin.cio.hudson.plugins.rolestrategy.Role.hashCode(Role.java:149)
at java.util.HashMap.hash(HashMap.java:338)
at java.util.HashMap.put(HashMap.java:611)
at java.util.HashSet.add(HashSet.java:219)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$1.perform(RoleMap.java:310)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$RoleWalker.walk(RoleMap.java:387)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$RoleWalker.<init>(RoleMap.java:376)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$1.<init>(RoleMap.java:307)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap.getRolesHavingPermission(RoleMap.java:307)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap.hasPermission(RoleMap.java:107)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap.access$000(RoleMap.java:75)
at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$AclImpl.hasPermission(RoleMap.java:362)

It sounds like https://issues.jenkins-ci.org/browse/JENKINS-18377 but it is hard to be sure.

…gPermission

I have experienced severe slowdown caused by following stacktrace:

        at java.util.AbstractSet.hashCode(AbstractSet.java:126)
	at com.michelin.cio.hudson.plugins.rolestrategy.Role.hashCode(Role.java:149)
	at java.util.HashMap.hash(HashMap.java:338)
	at java.util.HashMap.put(HashMap.java:611)
	at java.util.HashSet.add(HashSet.java:219)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$1.perform(RoleMap.java:310)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$RoleWalker.walk(RoleMap.java:387)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$RoleWalker.<init>(RoleMap.java:376)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$1.<init>(RoleMap.java:307)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap.getRolesHavingPermission(RoleMap.java:307)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap.hasPermission(RoleMap.java:107)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap.access$000(RoleMap.java:75)
	at com.michelin.cio.hudson.plugins.rolestrategy.RoleMap$AclImpl.hasPermission(RoleMap.java:362)
Copy link
Member

left a comment

👍

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 30, 2017

Actually I should rework the class to avoid construction of HashMap for getRolesHavingPermission() at all. Just a role walker with Matcher should be enough

@olivergondza

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

IIUC the set is there to make the roles unique - not sure there is an easy way to avoid the hashCode calculations (equals would be about as slow). I was thinking more about creating cache for permission-to-roles lookup getRolesHavingPermission is calculating per each permission but this was so much easier and safer to implement as there is no need to invalidate it ever.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

Anyway, the change makes the things better. Merging

@oleg-nenashev oleg-nenashev merged commit 165538f into jenkinsci:master Jun 2, 2017
1 check passed
1 check passed
Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.