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

Conversation

@AbhyudayaSharma
Copy link
Member

commented Jun 20, 2019

From our discussion yesterday on the Gitter chat and since the newMatchingRoleMap is called for every ACL request and a lot of permissions are checked for a project when the home page loads, having a cache avoids checking regular expressions again for every time permission checks are called for a given Item.

Add a cache to newMatchingRoleMap
Since the newMatchingRoleMap is called for every ACL request and a lot
of permissions are checked for a project when the home page loads, having
a cache avoids checking regular expressions again for every time permission
checks are called for a given Item.

@AbhyudayaSharma AbhyudayaSharma requested a review from jenkinsci/gsoc2019-role-strategy Jun 20, 2019

@AbhyudayaSharma AbhyudayaSharma self-assigned this Jun 20, 2019

@AbhyudayaSharma AbhyudayaSharma changed the title Add a cache for `RoleMap#newMatchingRoleMap()` Cache RoleMaps produced by `RoleMap#newMatchingRoleMap()` Jun 20, 2019

@oleg-nenashev
Copy link
Member

left a comment

Would be great to think about cache invalidation.
I would guess that ItemListener would be enough for it (so that we invalidate the cache on item creation or deletion). WDYT?

Also, would be great to have some performance metrics before and after the change from our benchmarks

* for different permissions for the same {@code itemName}, so cache them and avoid wasting time
* matching regular expressions.
*/
private final Cache<String, RoleMap> matchingRoleMapCache = CacheBuilder.newBuilder()

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 20, 2019

Author Member

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 21, 2019

Member

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

@AbhyudayaSharma

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Results compared to the benchmarks carried out in #80 :

image

Invalidate cache on adding and removing role
* cleanup JavaDoc for newMatchingRoleMap
* remove unused private method
* cleanup unAssignRole
*/
public RoleMap newMatchingRoleMap(String namePattern) {
RoleMap cachedRoleMap = matchingRoleMapCache.getIfPresent(namePattern);
public RoleMap newMatchingRoleMap(String itemName) {

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 20, 2019

Author Member

I've changed the JavaDoc because I don't think it was correctly telling what was going on.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 20, 2019

Member

I agree that the original Javadoc was wrong,
Maybe we could rename it to itemNamePrefix or so, because actually it does some kind of "pattern matching", just not the regexp/wildcard/whatever. It just takes a string and verifies it against regexp, but it does not really have to be a full item name. For better or worse...

* @param permission The permission you want to check
* @return A Set of Roles holding the given permission
*/
private Set<Role> getRolesHavingPermission(final Permission permission) {

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 20, 2019

Author Member

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

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 20, 2019

Author Member

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 21, 2019

Member

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

@@ -203,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();

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 20, 2019

Author Member

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.

This comment has been minimized.

Copy link
@oleg-nenashev
@oleg-nenashev
Copy link
Member

left a comment

Fine with me, just minor comments

* for different permissions for the same {@code itemName}, so cache them and avoid wasting time
* matching regular expressions.
*/
private final Cache<String, RoleMap> matchingRoleMapCache = CacheBuilder.newBuilder()

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 21, 2019

Member

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

@@ -203,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();

This comment has been minimized.

Copy link
@oleg-nenashev
* @param permission The permission you want to check
* @return A Set of Roles holding the given permission
*/
private Set<Role> getRolesHavingPermission(final Permission permission) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 21, 2019

Member

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

@AbhyudayaSharma AbhyudayaSharma force-pushed the AbhyudayaSharma:new-matching-rolemap-cache branch from 67fb6a0 to b267c97 Jun 21, 2019

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Test failure is really odd here, and last reopen did not trigger the build. Let me try

*/
private final Cache<String, RoleMap> matchingRoleMapCache = CacheBuilder.newBuilder()
.softValues()
.maximumSize(Settings.USER_DETAILS_CACHE_MAX_SIZE)

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 24, 2019

Author Member

The default value for the max size of the cache is 100. If there are more than 100 objects managed by Role Strategy, the cache would start evicting entries. Also, the max size of the cache cannot be increased after its creation. Should we care about this situation and create a larger cache using an ItemListener and a ComputerListener at the cost of higher memory usage?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 24, 2019

Member

+1. The objects are not big, so bumping to 2048 or so would make sense

* 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()

This comment has been minimized.

Copy link
@AbhyudayaSharma

AbhyudayaSharma Jun 24, 2019

Author Member

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?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 24, 2019

Member

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

@oleg-nenashev oleg-nenashev changed the title Cache RoleMaps produced by `RoleMap#newMatchingRoleMap()` Performance: Cache RoleMaps produced by `RoleMap#newMatchingRoleMap()` Jun 24, 2019

@oleg-nenashev
Copy link
Member

left a comment

re-👍

@runzexia runzexia added on-hold and removed on-hold labels Jun 27, 2019

@runzexia runzexia merged commit 474e172 into jenkinsci:master Jun 27, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@AbhyudayaSharma AbhyudayaSharma deleted the AbhyudayaSharma:new-matching-rolemap-cache branch Jun 27, 2019

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