opendev/gerrit
Code Issues Proposed changes

Don't use LoadingCache for change kind and mergeability

Browse Source 
As we've seen from recent changes, storing a reference to an open
resource in a LoadingCache key is a memory-leak-causing antipattern.
Get rid of this in the two caches where we were storing a Repository
reference, by using a standard Cache where the loader is passed in as
an argument to the get method. References can't leak into the stored
key if they never existed in the first place.

PatchListKey and IntralineDiffKey also contain transient data passed
through to the loader, but those will be a bit more code to untangle.
In any case they only leak immutable data, not open resources.

Change-Id: If505d71f04e21de273c7a803f295e1e3f7fd7306
This commit is contained in:
Dave Borowitz
2015-08-06 14:26:59 -07:00
committed by Shawn Pearce
parent e47caabfa8
commit 9efcff3124
2 changed files with 48 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
View File

@@ -19,8 +19,7 @@ import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.cache.Cache;
import com.google.common.cache.Weigher;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -35,7 +34,6 @@ import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Singleton;
import com.google.inject.name.Named;
import org.eclipse.jgit.errors.LargeObjectException;
@@ -54,6 +52,7 @@ import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.util.Collection;
import java.util.Objects;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
public class ChangeKindCacheImpl implements ChangeKindCache {
@@ -69,8 +68,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
bind(ChangeKindCache.class).to(ChangeKindCacheImpl.class);
persist(ID_CACHE, Key.class, ChangeKind.class)
.maximumWeight(2 << 20)
.weigher(ChangeKindWeigher.class)
.loader(Loader.class);
.weigher(ChangeKindWeigher.class);
}
};
}
@@ -99,8 +97,8 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
public ChangeKind getChangeKind(ProjectState project, Repository repo,
ObjectId prior, ObjectId next) {
try {
return new Loader().load(
new Key(repo, prior, next, useRecursiveMerge));
Key key = new Key(prior, next, useRecursiveMerge);
return new Loader(key, repo).call();
} catch (IOException e) {
log.warn("Cannot check trivial rebase of new patch set " + next.name()
+ " in " + project.getProject().getName(), e);
@@ -122,17 +120,13 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
private transient ObjectId next;
private transient String strategyName;
private transient Repository repo; // Passed through to loader on miss.
private Key(Repository repo, ObjectId prior,
ObjectId next, boolean useRecursiveMerge) {
private Key(ObjectId prior, ObjectId next, boolean useRecursiveMerge) {
checkNotNull(next, "next");
String strategyName = MergeUtil.mergeStrategyName(
true, useRecursiveMerge);
this.prior = prior.copy();
this.next = next.copy();
this.strategyName = strategyName;
this.repo = repo;
}
public Key(ObjectId prior, ObjectId next, String strategyName) {
@@ -182,15 +176,22 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
}
}
@Singleton
private static class Loader extends CacheLoader<Key, ChangeKind> {
private static class Loader implements Callable<ChangeKind> {
private final Key key;
private final Repository repo;
private Loader(Key key, Repository repo) {
this.key = key;
this.repo = repo;
}
@Override
public ChangeKind load(Key key) throws IOException {
public ChangeKind call() throws IOException {
if (Objects.equals(key.prior, key.next)) {
return ChangeKind.NO_CODE_CHANGE;
}
try (RevWalk walk = new RevWalk(key.repo)) {
try (RevWalk walk = new RevWalk(repo)) {
RevCommit prior = walk.parseCommit(key.prior);
walk.parseBody(prior);
RevCommit next = walk.parseCommit(key.next);
@@ -217,7 +218,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
// having the same tree as would exist when the prior commit is
// cherry-picked onto the next commit's new first parent.
ThreeWayMerger merger = MergeUtil.newThreeWayMerger(
key.repo, MergeUtil.createDryRunInserter(key.repo), key.strategyName);
repo, MergeUtil.createDryRunInserter(repo), key.strategyName);
merger.setBase(prior.getParent(0));
try {
if (merger.merge(next.getParent(0), prior)
@@ -229,8 +230,6 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
// it was a rework.
}
return ChangeKind.REWORK;
} finally {
key.repo = null;
}
}
@@ -264,7 +263,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
}
}
private final LoadingCache<Key, ChangeKind> cache;
private final Cache<Key, ChangeKind> cache;
private final boolean useRecursiveMerge;
private final ChangeData.Factory changeDataFactory;
private final ProjectCache projectCache;
@@ -273,7 +272,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
@Inject
ChangeKindCacheImpl(
@GerritServerConfig Config serverConfig,
@Named(ID_CACHE) LoadingCache<Key, ChangeKind> cache,
@Named(ID_CACHE) Cache<Key, ChangeKind> cache,
ChangeData.Factory changeDataFactory,
ProjectCache projectCache,
GitRepositoryManager repoManager) {
@@ -288,7 +287,8 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
public ChangeKind getChangeKind(ProjectState project, Repository repo,
ObjectId prior, ObjectId next) {
try {
return cache.get(new Key(repo, prior, next, useRecursiveMerge));
Key key = new Key(prior, next, useRecursiveMerge);
return cache.get(key, new Loader(key, repo));
} catch (ExecutionException e) {
log.warn("Cannot check trivial rebase of new patch set " + next.name()
+ " in " + project.getProject().getName(), e);

75
gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java
View File

@@ -14,7 +14,6 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.ioutil.BasicSerialization.readString;
@@ -23,8 +22,7 @@ import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
import com.google.common.base.MoreObjects;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.cache.Cache;
import com.google.common.cache.Weigher;
import com.google.common.collect.BiMap;
import com.google.common.collect.ImmutableBiMap;
@@ -63,6 +61,7 @@ import java.io.Serializable;
import java.util.Arrays;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
@Singleton
@@ -90,8 +89,7 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
protected void configure() {
persist(CACHE_NAME, EntryKey.class, Boolean.class)
.maximumWeight(1 << 20)
.weigher(MergeabilityWeigher.class)
.loader(Loader.class);
.weigher(MergeabilityWeigher.class);
bind(MergeabilityCache.class).to(MergeabilityCacheImpl.class);
}
};
@@ -111,10 +109,6 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
private SubmitType submitType;
private String mergeStrategy;
// Only used for loading, not stored. Callers MUST clear this field after
// loading to avoid leaking resources.
private transient LoadHelper load;
public EntryKey(ObjectId commit, ObjectId into, SubmitType submitType,
String mergeStrategy) {
this.commit = checkNotNull(commit, "commit");
@@ -123,13 +117,6 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
this.mergeStrategy = checkNotNull(mergeStrategy, "mergeStrategy");
}
private EntryKey(ObjectId commit, ObjectId into, SubmitType submitType,
String mergeStrategy, Branch.NameKey dest, Repository repo,
ReviewDb db) {
this(commit, into, submitType, mergeStrategy);
load = new LoadHelper(dest, repo, db);
}
public ObjectId getCommit() {
return commit;
}
@@ -196,41 +183,30 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
}
}
private static class LoadHelper {
private class Loader implements Callable<Boolean> {
private final EntryKey key;
private final Branch.NameKey dest;
private final Repository repo;
private final ReviewDb db;
private LoadHelper(Branch.NameKey dest, Repository repo, ReviewDb db) {
this.dest = checkNotNull(dest, "dest");
this.repo = checkNotNull(repo, "repo");
this.db = checkNotNull(db, "db");
}
}
@Singleton
public static class Loader extends CacheLoader<EntryKey, Boolean> {
private final SubmitStrategyFactory submitStrategyFactory;
@Inject
Loader(SubmitStrategyFactory submitStrategyFactory) {
this.submitStrategyFactory = submitStrategyFactory;
Loader(EntryKey key, Branch.NameKey dest, Repository repo, ReviewDb db) {
this.key = key;
this.dest = dest;
this.repo = repo;
this.db = db;
}
@Override
public Boolean load(EntryKey key)
public Boolean call()
throws NoSuchProjectException, MergeException, IOException {
LoadHelper load = key.load;
key.load = null;
checkArgument(load != null, "Key cannot be loaded: %s", key);
if (key.into.equals(ObjectId.zeroId())) {
return true; // Assume yes on new branch.
}
RefDatabase refDatabase = load.repo.getRefDatabase();
RefDatabase refDatabase = repo.getRefDatabase();
Iterable<Ref> refs = Iterables.concat(
refDatabase.getRefs(Constants.R_HEADS).values(),
refDatabase.getRefs(Constants.R_TAGS).values());
try (RevWalk rw = CodeReviewCommit.newRevWalk(load.repo)) {
try (RevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
RevFlag canMerge = rw.newFlag("CAN_MERGE");
CodeReviewCommit rev = parse(rw, key.commit);
rev.add(canMerge);
@@ -240,18 +216,18 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
accepted.addAll(Arrays.asList(rev.getParents()));
return submitStrategyFactory.create(
key.submitType,
load.db,
load.repo,
db,
repo,
rw,
null /*inserter*/,
canMerge,
accepted,
load.dest,
dest,
null).dryRun(tip, rev);
}
}
private static Set<RevCommit> alreadyAccepted(RevWalk rw, Iterable<Ref> refs)
private Set<RevCommit> alreadyAccepted(RevWalk rw, Iterable<Ref> refs)
throws MissingObjectException, IOException {
Set<RevCommit> accepted = Sets.newHashSet();
for (Ref r : refs) {
@@ -264,7 +240,7 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
return accepted;
}
private static CodeReviewCommit parse(RevWalk rw, ObjectId id)
private CodeReviewCommit parse(RevWalk rw, ObjectId id)
throws MissingObjectException, IncorrectObjectTypeException,
IOException {
return (CodeReviewCommit) rw.parseCommit(id);
@@ -280,10 +256,14 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
}
}
private final LoadingCache<EntryKey, Boolean> cache;
private final SubmitStrategyFactory submitStrategyFactory;
private final Cache<EntryKey, Boolean> cache;
@Inject
MergeabilityCacheImpl(@Named(CACHE_NAME) LoadingCache<EntryKey, Boolean> cache) {
MergeabilityCacheImpl(
SubmitStrategyFactory submitStrategyFactory,
@Named(CACHE_NAME) Cache<EntryKey, Boolean> cache) {
this.submitStrategyFactory = submitStrategyFactory;
this.cache = cache;
}
@@ -291,17 +271,14 @@ public class MergeabilityCacheImpl implements MergeabilityCache {
public boolean get(ObjectId commit, Ref intoRef, SubmitType submitType,
String mergeStrategy, Branch.NameKey dest, Repository repo, ReviewDb db) {
ObjectId into = intoRef != null ? intoRef.getObjectId() : ObjectId.zeroId();
EntryKey key =
new EntryKey(commit, into, submitType, mergeStrategy, dest, repo, db);
EntryKey key = new EntryKey(commit, into, submitType, mergeStrategy);
try {
return cache.get(key);
return cache.get(key, new Loader(key, dest, repo, db));
} catch (ExecutionException e) {
log.error(String.format("Error checking mergeability of %s into %s (%s)",
key.commit.name(), key.into.name(), key.submitType.name()),
e.getCause());
return false;
} finally {
key.load = null;
}
}