[JENKINS-55803] - Attempt to solve issue:removing the intermediate collections created each time getRolesHavingPermission() is invoked

[deepanshnagaria]

my attempt to fix the issue(55803,Performance: Replace RoleMap#getRolesHavingPermission() usages by iterators) by removing the intermediate collections each time the method is invoked. i did so by implementing the logic of the for loop in the roleWalker() itself. I removed the now not used method getRolesHavingPermission().

[oleg-nenashev]

@deepanshnagaria could you please the formatting?

In Java the common formatting is

if (something) {
   // ...
} else if (somethingElse) {
   // ...
} else {
   // ..
}

The logic itself looks Ok

[oleg-nenashev]

Optional would be recommended, but ok

[deepanshnagaria]

Sir i didn't understand.... Can you clarify a bit....?

[oleg-nenashev]

https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html

[deepanshnagaria]

Sorry for the issue in formatting i took reference from if else instead of this: (. I shall do that. Sorry again for repeated issues in formatting, i shall try to ignore further such issues.

[runzexia]

The logic looks ok,only some formatting problems.
And I was wondering if I could break the walker when the perform was true, just a possible improve.

[runzexia]

It would be better to add an overide annotation here.

[deepanshnagaria]

sure

[deepanshnagaria]

@runzexia I myself thought of breaking the RoleWalker but with current implementation it doesn't seem to be possible. But we can do the same by not calling the RoleWalker and implementing the logic of walk function, that could be done. Should I do that?

[runzexia]

I think you can try to do that, I don't know how oleg thinks, this PR already looks good.
@deepanshnagaria

[deepanshnagaria]

I shall do that in the next commit, it won't take much i think. Thanks for the review.

[deepanshnagaria]

@runzexia this commit contains changes for breaking the loop. cc: @oleg-nenashev

[oleg-nenashev]

@deepanshnagaria @runzexia

I myself thought of breaking the RoleWalker but with current implementation it doesn't seem to be possible. But we can do the same by not calling the RoleWalker and implementing the logic of walk function, that could be done. Should I do that?

Yes, breaking the cycle will be an interesting change

RoleWalker is a part of the plugin codebase:

role-strategy-plugin/src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleMap.java

Lines 415 to 436 in 529512e

	private abstract class RoleWalker {
	RoleWalker() {
	walk();
	}
	/**
	* Walk through the roles.
	*/
	public void walk() {
	Set<Role> roles = RoleMap.this.getRoles();
	Iterator<Role> iter = roles.iterator();
	while (iter.hasNext()) {
	Role current = iter.next();
	perform(current);
	}
	}
	/**
	* The method to implement which will be called on each {@link Role}.
	*/
	abstract public void perform(Role current);

My suggestion would be to create a follow-up ticket to support breaking the cycle from RoleWalker API (e.g. a new abort() method), and then this change can be landed in a follow-up PR.

[deepanshnagaria]

@oleg-nenashev I have created an issue https://issues.jenkins-ci.org/browse/JENKINS-55916 adding a point for memory, do tell if changes are required.
cc: @runzexia

[deepanshnagaria]

I have updated this commit in sink with changes in #58 please suggest changes if required. @runzexia @oleg-nenashev

[oleg-nenashev]

@deepanshnagaria there is a minor merge conflict after integration of other PRs. Are you able to resolve it? Or do you need some assistance?

[deepanshnagaria]

@deepanshnagaria there is a minor merge conflict after integration of other PRs. Are you able to resolve it? Or do you need some assistance?

yeah i shall do resolve that.

[deepanshnagaria]

ignore last commit pls

[oleg-nenashev]

@deepanshnagaria could you please configure your IDE to disable automatic code reformatting? Even if the code formatting is not good, massive formatting changes usually leads to merge conflicts. It is better to handle it in a separate PR next time

[oleg-nenashev]

LGTM apart from the code formatting

[deepanshnagaria]

LGTM apart from the code formatting

I have solved most of the formatting issues. Thanks for the review. Think, that it is ready for merge. Suggest changes if needed. @oleg-nenashev @runzexia