From 4b344f5db5fbc71b77cea3862bc8184ff23f962a Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Mon, 21 Jan 2019 16:52:14 +0000 Subject: [PATCH] ListProjects: Refactor to avoid excessive heap usage The JVM heap explosion during project list was introduced during the migration to the permission backend in change I0ba5491fc. Avoid pre-computing all the projects permissions and visibility checks when rendering the list, keeping the navigation through an iterator (fixed memory occupation) instead of pre-loading everything into a collection (variable memory occupation). Bug: Issue 10326 Change-Id: I7f0b7efcd57f096d2643723130bd543718b5dcc5 --- .../rest/project/ListProjectsIT.java | 5 ++ .../gerrit/server/project/ListProjects.java | 48 +++++++++---------- .../server/project/ProjectCacheImpl.java | 11 +++++ 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ListProjectsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ListProjectsIT.java index 3f07d54dd3..a8547649bb 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ListProjectsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ListProjectsIT.java @@ -35,6 +35,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.project.ProjectCacheImpl; import com.google.gerrit.server.project.Util; import com.google.inject.Inject; import java.util.List; @@ -92,6 +93,7 @@ public class ListProjectsIT extends AbstractDaemonTest { @Test public void listProjectsWithLimit() throws Exception { + ProjectCacheImpl projectCacheImpl = (ProjectCacheImpl) projectCache; for (int i = 0; i < 5; i++) { createProject("someProject" + i); } @@ -99,9 +101,12 @@ public class ListProjectsIT extends AbstractDaemonTest { String p = name(""); // 5, plus p which was automatically created. int n = 6; + projectCacheImpl.evictAllByName(); for (int i = 1; i <= n + 2; i++) { assertThatNameList(gApi.projects().list().withPrefix(p).withLimit(i).get()) .hasSize(Math.min(i, n)); + assertThat(projectCacheImpl.sizeAllByName()) + .isAtMost((long) (i + 2)); // 2 = AllProjects + AllUsers } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java index 887dfe3360..9ed04b0c43 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java @@ -16,12 +16,10 @@ package com.google.gerrit.server.project; import static com.google.gerrit.extensions.client.ProjectState.HIDDEN; import static java.nio.charset.StandardCharsets.UTF_8; -import static java.util.stream.Collectors.toList; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.errors.NoSuchGroupException; @@ -59,19 +57,19 @@ import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Set; +import java.util.Objects; import java.util.SortedMap; import java.util.SortedSet; import java.util.TreeMap; import java.util.TreeSet; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Ref; @@ -339,7 +337,8 @@ public class ListProjects implements RestReadView { PermissionBackend.WithUser perm = permissionBackend.user(currentUser); final TreeMap treeMap = new TreeMap<>(); try { - for (Project.NameKey projectName : filter(perm)) { + Iterable projectNames = filter(perm)::iterator; + for (Project.NameKey projectName : projectNames) { final ProjectState e = projectCache.get(projectName); if (e == null || (!all && e.getProject().getState() == HIDDEN)) { // If we can't get it from the cache, pretend its not present. @@ -478,31 +477,28 @@ public class ListProjects implements RestReadView { } } - private Collection filter(PermissionBackend.WithUser perm) - throws BadRequestException, PermissionBackendException { - Collection matches = Lists.newArrayList(scan()); + private Stream filter(PermissionBackend.WithUser perm) + throws BadRequestException { + Stream matches = StreamSupport.stream(scan().spliterator(), false); if (type == FilterType.PARENT_CANDIDATES) { - matches = parentsOf(matches); + matches = + matches.map(projectCache::get).map(this::parentOf).filter(Objects::nonNull).sorted(); } - return perm.filter(ProjectPermission.ACCESS, matches).stream().sorted().collect(toList()); + return matches.filter(p -> perm.project(p).testOrFalse(ProjectPermission.ACCESS)); } - private Collection parentsOf(Collection matches) { - Set parents = new HashSet<>(); - for (Project.NameKey p : matches) { - ProjectState ps = projectCache.get(p); - if (ps != null) { - Project.NameKey parent = ps.getProject().getParent(); - if (parent != null) { - if (projectCache.get(parent) != null) { - parents.add(parent); - } else { - log.warn("parent project {} of project {} not found", parent.get(), ps.getName()); - } - } - } + private Project.NameKey parentOf(ProjectState ps) { + if (ps == null) { + return null; } - return parents; + Project.NameKey parent = ps.getProject().getParent(); + if (parent != null) { + if (projectCache.get(parent) != null) { + return parent; + } + log.warn("parent project {} of project {} not found", parent.get(), ps.getName()); + } + return null; } private boolean isParentAccessible( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectCacheImpl.java index 3daf9c83e5..fff19ff5c8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectCacheImpl.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.project; import static java.util.stream.Collectors.toSet; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; @@ -337,4 +338,14 @@ public class ProjectCacheImpl implements ProjectCache { return mgr.list(); } } + + @VisibleForTesting + public void evictAllByName() { + byName.invalidateAll(); + } + + @VisibleForTesting + public long sizeAllByName() { + return byName.size(); + } }