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
This commit is contained in:
parent
099f7592de
commit
4b344f5db5
|
@ -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
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -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<TopLevelResource> {
|
|||
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 @@ public class ListProjects implements RestReadView<TopLevelResource> {
|
|||
}
|
||||
}
|
||||
|
||||
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) {
|
||||
private Project.NameKey parentOf(ProjectState ps) {
|
||||
if (ps == null) {
|
||||
return null;
|
||||
}
|
||||
Project.NameKey parent = ps.getProject().getParent();
|
||||
if (parent != null) {
|
||||
if (projectCache.get(parent) != null) {
|
||||
parents.add(parent);
|
||||
} else {
|
||||
return parent;
|
||||
}
|
||||
log.warn("parent project {} of project {} not found", parent.get(), ps.getName());
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return parents;
|
||||
return null;
|
||||
}
|
||||
|
||||
private boolean isParentAccessible(
|
||||
|
|
|
@ -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();
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue