From f71bafe885001f40e0b2946ec4eaebb0b34ba861 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Mon, 24 Sep 2018 16:42:54 +0200 Subject: [PATCH] Instrument two possibly long-running access computations We are investigating what it would take to remove PerThreadCache and refactor the rest of the default permission backend. One potential cause of slowness and the success of PerThreadCache are two access computations, one in ProjectCache, one on PermissionCollection. This commit instruments these to validate our assumption. The metrics will be removed once we have data. Change-Id: I5f39ae7113d657bbb69b693367aaa0930cf21900 --- Documentation/metrics.txt | 7 ++ .../permissions/PermissionCollection.java | 84 +++++++++++-------- .../gerrit/server/project/ProjectState.java | 36 ++++++-- .../server/permissions/RefControlTest.java | 5 +- 4 files changed, 89 insertions(+), 43 deletions(-) diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index 19d3b41fc4..2647b359cf 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -138,6 +138,13 @@ failed by table. * `notedb/read_all_external_ids_latency`: Latency for reading all external ID's from NoteDb. +=== Permissions + +* `permissions/project_state/computation_latency`: Latency to compute current access +sections on a project by traversing it's parents. +* `permissions/permission_collection/filter_latency`: Latency to filter access sections +by user and ref. + === Reviewer Suggestion * `reviewer_suggestion/query_accounts`: Latency for querying accounts for diff --git a/java/com/google/gerrit/server/permissions/PermissionCollection.java b/java/com/google/gerrit/server/permissions/PermissionCollection.java index 81e8d24250..b419698415 100644 --- a/java/com/google/gerrit/server/permissions/PermissionCollection.java +++ b/java/com/google/gerrit/server/permissions/PermissionCollection.java @@ -26,6 +26,10 @@ import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule.Action; +import com.google.gerrit.metrics.Description; +import com.google.gerrit.metrics.Description.Units; +import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.metrics.Timer0; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; @@ -54,10 +58,18 @@ public class PermissionCollection { @Singleton public static class Factory { private final SectionSortCache sorter; + // TODO(hiesel): Remove this once we got production data + private final Timer0 filterLatency; @Inject - Factory(SectionSortCache sorter) { + Factory(SectionSortCache sorter, MetricMaker metricMaker) { this.sorter = sorter; + this.filterLatency = + metricMaker.newTimer( + "permissions/permission_collection/filter_latency", + new Description("Latency for access filter computations in PermissionCollection") + .setCumulative() + .setUnit(Units.NANOSECONDS)); } /** @@ -117,41 +129,43 @@ public class PermissionCollection { */ PermissionCollection filter( Iterable matcherList, String ref, CurrentUser user) { - if (isRE(ref)) { - ref = RefPattern.shortestExample(ref); - } else if (ref.endsWith("/*")) { - ref = ref.substring(0, ref.length() - 1); + try (Timer0.Context ignored = filterLatency.start()) { + if (isRE(ref)) { + ref = RefPattern.shortestExample(ref); + } else if (ref.endsWith("/*")) { + ref = ref.substring(0, ref.length() - 1); + } + + // LinkedHashMap to maintain input ordering. + Map sectionToProject = new LinkedHashMap<>(); + boolean perUser = filterRefMatchingSections(matcherList, ref, user, sectionToProject); + List sections = Lists.newArrayList(sectionToProject.keySet()); + + // Sort by ref pattern specificity. For equally specific patterns, the sections from the + // project closer to the current one come first. + sorter.sort(ref, sections); + + // For block permissions, we want a different order: first, we want to go from parent to + // child. + List> accessDescending = + Lists.reverse(Lists.newArrayList(sectionToProject.entrySet())); + + Map> accessByProject = + accessDescending + .stream() + .collect( + Collectors.groupingBy( + Map.Entry::getValue, + LinkedHashMap::new, + mapping(Map.Entry::getKey, toList()))); + // Within each project, sort by ref specificity. + for (List secs : accessByProject.values()) { + sorter.sort(ref, secs); + } + + return new PermissionCollection( + Lists.newArrayList(accessByProject.values()), sections, perUser); } - - // LinkedHashMap to maintain input ordering. - Map sectionToProject = new LinkedHashMap<>(); - boolean perUser = filterRefMatchingSections(matcherList, ref, user, sectionToProject); - List sections = Lists.newArrayList(sectionToProject.keySet()); - - // Sort by ref pattern specificity. For equally specific patterns, the sections from the - // project closer to the current one come first. - sorter.sort(ref, sections); - - // For block permissions, we want a different order: first, we want to go from parent to - // child. - List> accessDescending = - Lists.reverse(Lists.newArrayList(sectionToProject.entrySet())); - - Map> accessByProject = - accessDescending - .stream() - .collect( - Collectors.groupingBy( - Map.Entry::getValue, - LinkedHashMap::new, - mapping(Map.Entry::getKey, toList()))); - // Within each project, sort by ref specificity. - for (List secs : accessByProject.values()) { - sorter.sort(ref, secs); - } - - return new PermissionCollection( - Lists.newArrayList(accessByProject.values()), sections, perUser); } } diff --git a/java/com/google/gerrit/server/project/ProjectState.java b/java/com/google/gerrit/server/project/ProjectState.java index a9b19d9f19..8355625550 100644 --- a/java/com/google/gerrit/server/project/ProjectState.java +++ b/java/com/google/gerrit/server/project/ProjectState.java @@ -36,6 +36,11 @@ import com.google.gerrit.extensions.api.projects.ThemeInfo; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.index.project.ProjectData; +import com.google.gerrit.metrics.Description; +import com.google.gerrit.metrics.Description.Units; +import com.google.gerrit.metrics.Field; +import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.metrics.Timer1; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.BooleanProjectConfig; import com.google.gerrit.reviewdb.client.Branch; @@ -92,6 +97,9 @@ public class ProjectState { private final long globalMaxObjectSizeLimit; private final boolean inheritProjectMaxObjectSizeLimit; + // TODO(hiesel): Remove this once we got production data + private final Timer1 computationLatency; + /** Last system time the configuration's revision was examined. */ private volatile long lastCheckGeneration; @@ -119,6 +127,7 @@ public class ProjectState { List commentLinks, CapabilityCollection.Factory limitsFactory, TransferConfig transferConfig, + MetricMaker metricMaker, @Assisted ProjectConfig config) { this.sitePaths = sitePaths; this.projectCache = projectCache; @@ -136,6 +145,14 @@ public class ProjectState { this.globalMaxObjectSizeLimit = transferConfig.getMaxObjectSizeLimit(); this.inheritProjectMaxObjectSizeLimit = transferConfig.getInheritProjectMaxObjectSizeLimit(); + this.computationLatency = + metricMaker.newTimer( + "permissions/project_state/computation_latency", + new Description("Latency for access computations in ProjectState") + .setCumulative() + .setUnit(Units.NANOSECONDS), + Field.ofString("method")); + if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) { localOwners = Collections.emptySet(); } else { @@ -354,15 +371,20 @@ public class ProjectState { * cached. Callers should try to cache this result per-request as much as possible. */ public List getAllSections() { - if (isAllProjects) { - return getLocalAccessSections(); - } + try (Timer1.Context ignored = computationLatency.start("getAllSections")) { + if (isAllProjects) { + return getLocalAccessSections(); + } - List all = new ArrayList<>(); - for (ProjectState s : tree()) { - all.addAll(s.getLocalAccessSections()); + List all = new ArrayList<>(); + Iterable tree = tree(); + try (Timer1.Context ignored2 = computationLatency.start("getAllSections-parsing-only")) { + for (ProjectState s : tree) { + all.addAll(s.getLocalAccessSections()); + } + } + return all; } - return all; } /** diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java index 31eee9f74c..a3f9f93923 100644 --- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java +++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java @@ -43,6 +43,7 @@ import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.errors.InvalidNameException; import com.google.gerrit.extensions.api.projects.CommentLinkInfo; +import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -204,6 +205,7 @@ public class RefControlTest { @Inject private ThreadLocalRequestContext requestContext; @Inject private DefaultRefFilter.Factory refFilterFactory; @Inject private TransferConfig transferConfig; + @Inject private MetricMaker metricMaker; @Before public void setUp() throws Exception { @@ -291,7 +293,7 @@ public class RefControlTest { Cache c = CacheBuilder.newBuilder().build(); - sectionSorter = new PermissionCollection.Factory(new SectionSortCache(c)); + sectionSorter = new PermissionCollection.Factory(new SectionSortCache(c), metricMaker); parent = new ProjectConfig(parentKey); parent.load(newRepository(parentKey)); @@ -972,6 +974,7 @@ public class RefControlTest { commentLinks, capabilityCollectionFactory, transferConfig, + metricMaker, pc)); return repo; }