opendev/gerrit
Code Issues Proposed changes

Merge changes Ia9009deb,I7ae66ac8

Browse Source 
* changes:
  Convert ChangeKindCacheImpl.Key to AutoValue
  ChangeKindCacheImpl: Don't use reference equality for trees
This commit is contained in:
Alice Kober-Sotzek 2018-05-24 12:35:58 +00:00 committed by Gerrit Code Review
commit 3602efc523
2 changed files with 27 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

78
java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
View File

@ -15,8 +15,8 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.Cache;
import com.google.common.cache.Weigher;
@ -50,6 +50,7 @@ import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import org.eclipse.jgit.errors.LargeObjectException;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
@ -104,7 +105,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
ObjectId prior,
ObjectId next) {
try {
Key key = new Key(prior, next, useRecursiveMerge);
Key key = Key.create(prior, next, useRecursiveMerge);
return new Loader(key, repoManager, project, rw, repoConfig).call();
} catch (IOException e) {
log.warn(
@ -125,52 +126,21 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
}
}
public static class Key {
private transient ObjectId prior;
private transient ObjectId next;
private transient String strategyName;
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;
@AutoValue
public abstract static class Key {
public static Key create(AnyObjectId prior, AnyObjectId next, String strategyName) {
return new AutoValue_ChangeKindCacheImpl_Key(prior.copy(), next.copy(), strategyName);
}
public Key(ObjectId prior, ObjectId next, String strategyName) {
this.prior = prior;
this.next = next;
this.strategyName = strategyName;
private static Key create(AnyObjectId prior, AnyObjectId next, boolean useRecursiveMerge) {
return create(prior, next, MergeUtil.mergeStrategyName(true, useRecursiveMerge));
}
public ObjectId getPrior() {
return prior;
}
public abstract ObjectId prior();
public ObjectId getNext() {
return next;
}
public abstract ObjectId next();
public String getStrategyName() {
return strategyName;
}
@Override
public boolean equals(Object o) {
if (o instanceof Key) {
Key k = (Key) o;
return Objects.equals(prior, k.prior)
&& Objects.equals(next, k.next)
&& Objects.equals(strategyName, k.strategyName);
}
return false;
}
@Override
public int hashCode() {
return Objects.hash(prior, next, strategyName);
}
public abstract String strategyName();
@VisibleForTesting
static class Serializer implements CacheSerializer<Key> {
@ -179,9 +149,9 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
ObjectIdConverter idConverter = ObjectIdConverter.create();
return ProtoCacheSerializers.toByteArray(
ChangeKindKeyProto.newBuilder()
.setPrior(idConverter.toByteString(object.getPrior()))
.setNext(idConverter.toByteString(object.getNext()))
.setStrategyName(object.getStrategyName())
.setPrior(idConverter.toByteString(object.prior()))
.setNext(idConverter.toByteString(object.next()))
.setStrategyName(object.strategyName())
.build());
}
@ -190,7 +160,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
ChangeKindKeyProto proto =
ProtoCacheSerializers.parseUnchecked(ChangeKindKeyProto.parser(), in);
ObjectIdConverter idConverter = ObjectIdConverter.create();
return new Key(
return create(
idConverter.fromByteString(proto.getPrior()),
idConverter.fromByteString(proto.getNext()),
proto.getStrategyName());
@ -226,7 +196,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
@SuppressWarnings("resource") // Resources are manually managed.
@Override
public ChangeKind call() throws IOException {
if (Objects.equals(key.prior, key.next)) {
if (Objects.equals(key.prior(), key.next())) {
return ChangeKind.NO_CODE_CHANGE;
}
@ -239,9 +209,9 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
config = repo.getConfig();
}
try {
RevCommit prior = rw.parseCommit(key.prior);
RevCommit prior = rw.parseCommit(key.prior());
rw.parseBody(prior);
RevCommit next = rw.parseCommit(key.next);
RevCommit next = rw.parseCommit(key.next());
rw.parseBody(next);
if (!next.getFullMessage().equals(prior.getFullMessage())) {
@ -272,7 +242,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.
try (ObjectInserter ins = new InMemoryInserter(rw.getObjectReader())) {
ThreeWayMerger merger = MergeUtil.newThreeWayMerger(ins, config, key.strategyName);
ThreeWayMerger merger = MergeUtil.newThreeWayMerger(ins, config, key.strategyName());
merger.setBase(prior.getParent(0));
if (merger.merge(next.getParent(0), prior)
&& merger.getResultTreeId().equals(next.getTree())) {
@ -316,7 +286,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
}
private static boolean isSameDeltaAndTree(RevCommit prior, RevCommit next) {
if (next.getTree() != prior.getTree()) {
if (!Objects.equals(next.getTree(), prior.getTree())) {
return false;
}
@ -329,7 +299,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
// Make sure that the prior/next delta is the same - not just the tree.
// This is done by making sure that the parent trees are equal.
for (int i = 0; i < prior.getParentCount(); i++) {
if (next.getParent(i).getTree() != prior.getParent(i).getTree()) {
if (!Objects.equals(next.getParent(i).getTree(), prior.getParent(i).getTree())) {
return false;
}
}
@ -342,7 +312,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
public int weigh(Key key, ChangeKind changeKind) {
return 16
+ 2 * 36
+ 2 * key.strategyName.length() // Size of Key, 64 bit JVM
+ 2 * key.strategyName().length() // Size of Key, 64 bit JVM
+ 2 * changeKind.name().length(); // Size of ChangeKind, 64 bit JVM
}
}
@ -372,7 +342,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
ObjectId prior,
ObjectId next) {
try {
Key key = new Key(prior, next, useRecursiveMerge);
Key key = Key.create(prior, next, useRecursiveMerge);
return cache.get(key, new Loader(key, repoManager, project, rw, repoConfig));
} catch (ExecutionException e) {
log.warn("Cannot check trivial rebase of new patch set " + next.name() + " in " + project, e);

5
javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java
View File

@ -22,6 +22,7 @@ import static com.google.gerrit.server.cache.testing.SerializedClassSubject.asse
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.server.cache.CacheSerializer;
import com.google.gerrit.server.cache.proto.Cache.ChangeKindKeyProto;
import com.google.gerrit.server.change.ChangeKindCacheImpl.Key;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
@ -29,7 +30,7 @@ public class ChangeKindCacheImplTest {
@Test
public void keySerializer() throws Exception {
ChangeKindCacheImpl.Key key =
new ChangeKindCacheImpl.Key(
Key.create(
ObjectId.zeroId(),
ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
"aStrategy");
@ -55,7 +56,7 @@ public class ChangeKindCacheImplTest {
@Test
public void keyFields() throws Exception {
assertThatSerializedClass(ChangeKindCacheImpl.Key.class)
.hasFields(
.hasAutoValueMethods(
ImmutableMap.of(
"prior", ObjectId.class, "next", ObjectId.class, "strategyName", String.class));
}