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
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 3f07d54..a854764 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.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 @@
@Test
public void listProjectsWithLimit() throws Exception {
+ ProjectCacheImpl projectCacheImpl = (ProjectCacheImpl) projectCache;
for (int i = 0; i < 5; i++) {
createProject("someProject" + i);
}
@@ -99,9 +101,12 @@
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 887dfe3..9ed04b0 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 @@
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.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 @@
PermissionBackend.WithUser perm = permissionBackend.user(currentUser);
final TreeMap<Project.NameKey, ProjectNode> treeMap = new TreeMap<>();
try {
- for (Project.NameKey projectName : filter(perm)) {
+ Iterable<Project.NameKey> 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 @@
}
}
- private Collection<Project.NameKey> filter(PermissionBackend.WithUser perm)
- throws BadRequestException, PermissionBackendException {
- Collection<Project.NameKey> matches = Lists.newArrayList(scan());
+ private Stream<Project.NameKey> filter(PermissionBackend.WithUser perm)
+ throws BadRequestException {
+ Stream<Project.NameKey> 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<Project.NameKey> parentsOf(Collection<Project.NameKey> matches) {
- Set<Project.NameKey> 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 3daf9c8..fff19ff 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 @@
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 @@
return mgr.list();
}
}
+
+ @VisibleForTesting
+ public void evictAllByName() {
+ byName.invalidateAll();
+ }
+
+ @VisibleForTesting
+ public long sizeAllByName() {
+ return byName.size();
+ }
}
