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

Performance: Cache RoleMaps produced by RoleMap#newMatchingRoleMap() #81

Merged
merged 6 commits into from
Jun 27, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,16 @@ public class RoleMap {
.expireAfterWrite(Settings.USER_DETAILS_CACHE_EXPIRATION_TIME_SEC, TimeUnit.SECONDS)
.build();

/**
* {@link RoleMap}s are created again and again using {@link RoleMap#newMatchingRoleMap(String)}
* for different permissions for the same {@code itemNamePrefix}, so cache them and avoid wasting time
* matching regular expressions.
*/
private final Cache<String, RoleMap> matchingRoleMapCache = CacheBuilder.newBuilder()
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we continue with the Cache from Guava (avoid adding any dependencies) or use something else like JCS, Ehcache or something else?

Copy link
Member

Choose a reason for hiding this comment

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

It is fine to use Guava as long as we do not needed newer versions.
Maybe we could have a research task for comparing cache implementations with benchmarks in later phases

Copy link
Member Author

Choose a reason for hiding this comment

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

The matching RoleMaps that are created also have a cache in them which is not needed. Do we reorganize the class structure to avoid it or don't care about a little more memory use?

Copy link
Member

Choose a reason for hiding this comment

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

I would submit a follow-up ticket for it. Memory usage is not a problem for the plugin ATM

.softValues()
.maximumSize(2048)
.expireAfterWrite(1, TimeUnit.HOURS)
.build();

RoleMap() {
this.grantedRoles = new ConcurrentSkipListMap<Role, Set<String>>();
Expand Down Expand Up @@ -193,8 +203,8 @@ public SidACL getACL(RoleType roleType, AccessControlled controlledItem) {
public void addRole(Role role) {
if (this.getRole(role.getName()) == null) {
this.grantedRoles.put(role, new CopyOnWriteArraySet<>());
matchingRoleMapCache.invalidateAll();
Copy link
Member Author

Choose a reason for hiding this comment

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

This cache would only be affected when a change to grantedRoles is made because the RoleMap returned contains only those roles applicable for a given name. So it won't be necessary to use ItemListener in this case.

Copy link
Member

Choose a reason for hiding this comment

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

agreed

}

}

/**
Expand All @@ -215,11 +225,10 @@ public void assignRole(Role role, String sid) {
* @since 2.6.0
*/
public void unAssignRole(Role role, String sid) {
if (this.hasRole(role)) {
if (this.grantedRoles.get(role).contains(sid)) {
this.grantedRoles.get(role).remove(sid);
Set<String> sids = grantedRoles.get(role);
if (sids != null) {
sids.remove(sid);
}
}
}

/**
Expand Down Expand Up @@ -294,6 +303,7 @@ public Role getRole(String name) {
*/
public void removeRole(Role role){
this.grantedRoles.remove(role);
matchingRoleMapCache.invalidateAll();
}


Expand Down Expand Up @@ -354,50 +364,31 @@ public Set<String> getSidsForRole(String roleName) {
}

/**
* Create a sub-map of the current {@link RoleMap} containing only the
* {@link Role}s matching the given pattern.
* @param namePattern The pattern to match
* @return A {@link RoleMap} containing only {@link Role}s matching the given name
* Create a sub-map of this {@link RoleMap} containing {@link Role}s that are applicable
* on the given {@code itemNamePrefix}.
*
* @param itemNamePrefix the name of the {@link hudson.model.AbstractItem} or {@link hudson.model.Computer}
* @return A {@link RoleMap} containing roles that are applicable on the itemNamePrefix
*/
public RoleMap newMatchingRoleMap(String itemNamePrefix) {
RoleMap cachedRoleMap = matchingRoleMapCache.getIfPresent(itemNamePrefix);
if (cachedRoleMap != null) {
return cachedRoleMap;
}

public RoleMap newMatchingRoleMap(String namePattern) {
SortedMap<Role, Set<String>> roleMap = new TreeMap<>();
new RoleWalker() {
public void perform(Role current) {
Matcher m = current.getPattern().matcher(namePattern);
Matcher m = current.getPattern().matcher(itemNamePrefix);
if (m.matches()) {
roleMap.put(current, grantedRoles.get(current));
}
}
};
return new RoleMap(roleMap);
}

/**
* Get all the roles holding the given permission.
* @param permission The permission you want to check
* @return A Set of Roles holding the given permission
*/
private Set<Role> getRolesHavingPermission(final Permission permission) {
Copy link
Member Author

@AbhyudayaSharma AbhyudayaSharma Jun 20, 2019

Choose a reason for hiding this comment

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

I've removed it because it was not being used anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was refactored out in #54 and has no references to it now.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like a good API method, but we can always recover it later if somebody needs it

final Set<Role> roles = new HashSet<>();
final Set<Permission> permissions = new HashSet<>();
Permission p = permission;

// Get the implying permissions
for (; p!=null; p=p.impliedBy) {
permissions.add(p);
}
// Walk through the roles, and only add the roles having the given permission,
// or a permission implying the given permission
new RoleWalker() {
public void perform(Role current) {
if (current.hasAnyPermission(permissions)) {
roles.add(current);
}
}
};

return roles;
RoleMap newMatchingRoleMap = new RoleMap(roleMap);
matchingRoleMapCache.put(itemNamePrefix, newMatchingRoleMap);
return newMatchingRoleMap;
}

/**
Expand Down