Implement PureRevertCache

Computing if a change is a pure revert of another change or commit
is an expensive operation as it requires a Git merge. This is especially
true for large Git repositories.

However, this information is easy to cache in a persistent cache as it
can be keyed by the SHA1s of the commits that we want to diff. Given
that this computation runs many times if the Prolog fact is used because
we freshly compute the submit rules often, this commit builds a
persisted cache for it.

There are many existing tests that cover the behavior in ChangeIT.

This commit adds a serializer for Protobuf to ensure we are not using
the default Java serialization as well as a test.

Change-Id: Id79e2fb2f6646d8e48fdfc3a3b0bcf03f51b1400
This commit is contained in:
Patrick Hiesel 2019-02-19 09:09:22 +01:00
parent 67f0c672f2
commit a57c0d6e69
15 changed files with 414 additions and 83 deletions

View File

@ -979,6 +979,11 @@ cache `"prolog_rules"`::
Caches parsed `rules.pl` contents for each project. This cache uses the same
size as the `projects` cache, and cannot be configured independently.
cache `"pure_revert"`::
+
Result of checking if one change or commit is a pure/clean revert of
another.
cache `"sshkeys"`::
+
Caches unpacked versions of user SSH keys, so the internal SSH daemon

View File

@ -58,6 +58,7 @@ import com.google.gerrit.server.extensions.events.EventUtil;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.PureRevertCache;
import com.google.gerrit.server.git.SearchingChangeCacheImpl;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
@ -160,6 +161,7 @@ public class BatchProgramModule extends FactoryModule {
install(ChangeKindCacheImpl.module());
install(MergeabilityCacheImpl.module());
install(TagCache.module());
install(PureRevertCache.module());
factory(CapabilityCollection.Factory.class);
factory(ChangeData.AssistedFactory.class);
factory(ProjectState.Factory.class);

View File

@ -0,0 +1,38 @@
// Copyright (C) 2019 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.cache.serialize;
import com.google.gerrit.proto.Protos;
import com.google.protobuf.MessageLite;
import com.google.protobuf.Parser;
/** A CacheSerializer for Protobuf messages. */
public class ProtobufSerializer<T extends MessageLite> implements CacheSerializer<T> {
private final Parser<T> parser;
public ProtobufSerializer(Parser<T> parser) {
this.parser = parser;
}
@Override
public byte[] serialize(T object) {
return Protos.toByteArray(object);
}
@Override
public T deserialize(byte[] in) {
return Protos.parseUnchecked(parser, in);
}
}

View File

@ -14,109 +14,49 @@
package com.google.gerrit.server.change;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.common.PureRevertInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.PureRevertCache;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.List;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.DiffFormatter;
import java.util.Optional;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.merge.ThreeWayMerger;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
/** Can check if a change is a pure revert (= a revert with no further modifications). */
@Singleton
public class PureRevert {
private final MergeUtil.Factory mergeUtilFactory;
private final GitRepositoryManager repoManager;
private final ProjectCache projectCache;
private final ChangeNotes.Factory notesFactory;
private final PatchSetUtil psUtil;
private final PureRevertCache pureRevertCache;
@Inject
PureRevert(
MergeUtil.Factory mergeUtilFactory,
GitRepositoryManager repoManager,
ProjectCache projectCache,
ChangeNotes.Factory notesFactory,
PatchSetUtil psUtil) {
this.mergeUtilFactory = mergeUtilFactory;
this.repoManager = repoManager;
this.projectCache = projectCache;
this.notesFactory = notesFactory;
this.psUtil = psUtil;
PureRevert(PureRevertCache pureRevertCache) {
this.pureRevertCache = pureRevertCache;
}
public PureRevertInfo get(ChangeNotes notes, @Nullable String claimedOriginal)
public boolean get(ChangeNotes notes, Optional<String> claimedOriginal)
throws OrmException, IOException, BadRequestException, ResourceConflictException {
PatchSet currentPatchSet = psUtil.current(notes);
PatchSet currentPatchSet = notes.getCurrentPatchSet();
if (currentPatchSet == null) {
throw new ResourceConflictException("current revision is missing");
}
if (claimedOriginal == null) {
if (notes.getChange().getRevertOf() == null) {
throw new BadRequestException("no ID was provided and change isn't a revert");
}
PatchSet ps =
psUtil.current(
notesFactory.createChecked(notes.getProjectName(), notes.getChange().getRevertOf()));
claimedOriginal = ps.getRevision().get();
if (!claimedOriginal.isPresent()) {
return pureRevertCache.isPureRevert(notes);
}
try (Repository repo = repoManager.openRepository(notes.getProjectName());
ObjectInserter oi = repo.newObjectInserter();
RevWalk rw = new RevWalk(repo)) {
RevCommit claimedOriginalCommit;
try {
claimedOriginalCommit = rw.parseCommit(ObjectId.fromString(claimedOriginal));
} catch (InvalidObjectIdException | MissingObjectException e) {
throw new BadRequestException("invalid object ID");
}
if (claimedOriginalCommit.getParentCount() == 0) {
throw new BadRequestException("can't check against initial commit");
}
RevCommit claimedRevertCommit =
rw.parseCommit(ObjectId.fromString(currentPatchSet.getRevision().get()));
if (claimedRevertCommit.getParentCount() == 0) {
throw new BadRequestException("claimed revert has no parents");
}
// Rebase claimed revert onto claimed original
ThreeWayMerger merger =
mergeUtilFactory
.create(projectCache.checkedGet(notes.getProjectName()))
.newThreeWayMerger(oi, repo.getConfig());
merger.setBase(claimedRevertCommit.getParent(0));
boolean success = merger.merge(claimedRevertCommit, claimedOriginalCommit);
if (!success || merger.getResultTreeId() == null) {
// Merge conflict during rebase
return new PureRevertInfo(false);
}
// Any differences between claimed original's parent and the rebase result indicate that the
// claimedRevert is not a pure revert but made content changes
try (DiffFormatter df = new DiffFormatter(new ByteArrayOutputStream())) {
df.setReader(oi.newReader(), repo.getConfig());
List<DiffEntry> entries =
df.scan(claimedOriginalCommit.getParent(0), merger.getResultTreeId());
return new PureRevertInfo(entries.isEmpty());
}
ObjectId claimedOriginalObjectId;
try {
claimedOriginalObjectId = ObjectId.fromString(claimedOriginal.get());
} catch (InvalidObjectIdException e) {
throw new BadRequestException("invalid object ID");
}
return pureRevertCache.isPureRevert(
notes.getProjectName(),
ObjectId.fromString(notes.getCurrentPatchSet().getRevision().get()),
claimedOriginalObjectId);
}
}

View File

@ -117,6 +117,7 @@ import com.google.gerrit.server.git.GitModule;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.MergedByPushOp;
import com.google.gerrit.server.git.NotesBranchUtil;
import com.google.gerrit.server.git.PureRevertCache;
import com.google.gerrit.server.git.ReceivePackInitializer;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.TransferConfig;
@ -237,6 +238,7 @@ public class GerritGlobalModule extends FactoryModule {
install(SubmitStrategy.module());
install(TagCache.module());
install(OAuthTokenCache.module());
install(PureRevertCache.module());
install(new AccessControlModule());
install(new CmdLineParserModule());

View File

@ -0,0 +1,203 @@
// Copyright (C) 2019 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.cache.proto.Cache;
import com.google.gerrit.server.cache.proto.Cache.PureRevertKeyProto;
import com.google.gerrit.server.cache.serialize.BooleanCacheSerializer;
import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
import com.google.gerrit.server.cache.serialize.ProtobufSerializer;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ProjectCache;
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 com.google.protobuf.ByteString;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.List;
import java.util.concurrent.ExecutionException;
import org.eclipse.jgit.diff.DiffEntry;
import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.merge.ThreeWayMerger;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
/** Computes and caches if a change is a pure revert of another change. */
@Singleton
public class PureRevertCache {
private static final String ID_CACHE = "pure_revert";
public static Module module() {
return new CacheModule() {
@Override
protected void configure() {
persist(ID_CACHE, Cache.PureRevertKeyProto.class, Boolean.class)
.maximumWeight(100)
.loader(Loader.class)
.version(1)
.keySerializer(new ProtobufSerializer<>(Cache.PureRevertKeyProto.parser()))
.valueSerializer(BooleanCacheSerializer.INSTANCE);
}
};
}
private final LoadingCache<PureRevertKeyProto, Boolean> cache;
private final ChangeNotes.Factory notesFactory;
@Inject
PureRevertCache(
@Named(ID_CACHE) LoadingCache<PureRevertKeyProto, Boolean> cache,
ChangeNotes.Factory notesFactory) {
this.cache = cache;
this.notesFactory = notesFactory;
}
/**
* Returns {@code true} if {@code claimedRevert} is a pure (clean) revert of the change that is
* referenced in {@link Change#getRevertOf()}.
*
* @return {@code true} if {@code claimedRevert} is a pure (clean) revert.
* @throws IOException if there was a problem with the storage layer
* @throws OrmException if there was a problem with the storage layer
* @throws BadRequestException if there is a problem with the provided {@link ChangeNotes}
*/
public boolean isPureRevert(ChangeNotes claimedRevert)
throws OrmException, IOException, BadRequestException {
if (claimedRevert.getChange().getRevertOf() == null) {
throw new BadRequestException("revertOf not set");
}
ChangeNotes claimedOriginal =
notesFactory.createChecked(
claimedRevert.getProjectName(), claimedRevert.getChange().getRevertOf());
return isPureRevert(
claimedRevert.getProjectName(),
ObjectId.fromString(claimedRevert.getCurrentPatchSet().getRevision().get()),
ObjectId.fromString(claimedOriginal.getCurrentPatchSet().getRevision().get()));
}
/**
* Returns {@code true} if {@code claimedRevert} is a pure (clean) revert of {@code
* claimedOriginal}.
*
* @return {@code true} if {@code claimedRevert} is a pure (clean) revert of {@code
* claimedOriginal}.
* @throws IOException if there was a problem with the storage layer
* @throws BadRequestException if there is a problem with the provided {@link ObjectId}s
*/
public boolean isPureRevert(
Project.NameKey project, ObjectId claimedRevert, ObjectId claimedOriginal)
throws IOException, BadRequestException {
try {
return cache.get(key(project, claimedRevert, claimedOriginal));
} catch (ExecutionException e) {
Throwables.throwIfInstanceOf(e.getCause(), BadRequestException.class);
throw new IOException(e);
}
}
@VisibleForTesting
static PureRevertKeyProto key(
Project.NameKey project, ObjectId claimedRevert, ObjectId claimedOriginal) {
ByteString original = ObjectIdConverter.create().toByteString(claimedOriginal);
ByteString revert = ObjectIdConverter.create().toByteString(claimedRevert);
return PureRevertKeyProto.newBuilder()
.setProject(project.get())
.setClaimedOriginal(original)
.setClaimedRevert(revert)
.build();
}
static class Loader extends CacheLoader<PureRevertKeyProto, Boolean> {
private final GitRepositoryManager repoManager;
private final MergeUtil.Factory mergeUtilFactory;
private final ProjectCache projectCache;
@Inject
Loader(
GitRepositoryManager repoManager,
MergeUtil.Factory mergeUtilFactory,
ProjectCache projectCache) {
this.repoManager = repoManager;
this.mergeUtilFactory = mergeUtilFactory;
this.projectCache = projectCache;
}
@Override
public Boolean load(PureRevertKeyProto key) throws BadRequestException, IOException {
try (TraceContext.TraceTimer ignored =
TraceContext.newTimer("Loading pure revert for %s", key)) {
ObjectId original = ObjectIdConverter.create().fromByteString(key.getClaimedOriginal());
ObjectId revert = ObjectIdConverter.create().fromByteString(key.getClaimedRevert());
Project.NameKey project = new Project.NameKey(key.getProject());
try (Repository repo = repoManager.openRepository(project);
ObjectInserter oi = repo.newObjectInserter();
RevWalk rw = new RevWalk(repo)) {
RevCommit claimedOriginalCommit;
try {
claimedOriginalCommit = rw.parseCommit(original);
} catch (InvalidObjectIdException | MissingObjectException e) {
throw new BadRequestException("invalid object ID");
}
if (claimedOriginalCommit.getParentCount() == 0) {
throw new BadRequestException("can't check against initial commit");
}
RevCommit claimedRevertCommit = rw.parseCommit(revert);
if (claimedRevertCommit.getParentCount() == 0) {
return false;
}
// Rebase claimed revert onto claimed original
ThreeWayMerger merger =
mergeUtilFactory
.create(projectCache.checkedGet(project))
.newThreeWayMerger(oi, repo.getConfig());
merger.setBase(claimedRevertCommit.getParent(0));
boolean success = merger.merge(claimedRevertCommit, claimedOriginalCommit);
if (!success || merger.getResultTreeId() == null) {
// Merge conflict during rebase
return false;
}
// Any differences between claimed original's parent and the rebase result indicate that
// the
// claimedRevert is not a pure revert but made content changes
try (DiffFormatter df = new DiffFormatter(new ByteArrayOutputStream())) {
df.setReader(oi.newReader(), repo.getConfig());
List<DiffEntry> entries =
df.scan(claimedOriginalCommit.getParent(0), merger.getResultTreeId());
return entries.isEmpty();
}
}
}
}
}
}

View File

@ -1111,7 +1111,7 @@ public class ChangeData {
return null;
}
try {
return pureRevert.get(notes(), null).isPureRevert;
return pureRevert.get(notes(), Optional.empty());
} catch (IOException | BadRequestException | ResourceConflictException e) {
throw new OrmException("could not compute pure revert", e);
}

View File

@ -25,6 +25,7 @@ import com.google.gerrit.server.change.PureRevert;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.Optional;
import org.kohsuke.args4j.Option;
public class GetPureRevert implements RestReadView<ChangeResource> {
@ -49,6 +50,7 @@ public class GetPureRevert implements RestReadView<ChangeResource> {
public PureRevertInfo apply(ChangeResource rsrc)
throws ResourceConflictException, IOException, BadRequestException, OrmException,
AuthException {
return pureRevert.get(rsrc.getNotes(), claimedOriginal);
boolean isPureRevert = pureRevert.get(rsrc.getNotes(), Optional.ofNullable(claimedOriginal));
return new PureRevertInfo(isPureRevert);
}
}

View File

@ -3695,7 +3695,7 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void pureRevertThrowsExceptionWhenChangeIsNotARevertAndNoIdProvided() throws Exception {
exception.expect(BadRequestException.class);
exception.expectMessage("no ID was provided and change isn't a revert");
exception.expectMessage("revertOf not set");
gApi.changes().id(createChange().getChangeId()).pureRevert();
}

View File

@ -17,5 +17,6 @@ junit_tests(
"//lib/truth",
"//lib/truth:truth-proto-extension",
"//proto:cache_java_proto",
"//proto/testing:test_java_proto",
],
)

View File

@ -0,0 +1,43 @@
// Copyright (C) 2019 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.cache.serialize;
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.proto.testing.Test.SerializableProto;
import com.google.gerrit.testing.GerritBaseTests;
import org.junit.Test;
public class ProtobufSerializerTest extends GerritBaseTests {
@Test
public void requiredAndOptionalTypes() {
assertRoundTrip(SerializableProto.newBuilder().setId(123));
assertRoundTrip(SerializableProto.newBuilder().setId(123).setText("foo bar"));
}
@Test
public void exactByteSequence() {
ProtobufSerializer<SerializableProto> s = new ProtobufSerializer<>(SerializableProto.parser());
SerializableProto proto = SerializableProto.newBuilder().setId(123).setText("foo bar").build();
byte[] serialized = s.serialize(proto);
// Hard-code byte sequence to detect library changes
assertThat(serialized).isEqualTo(new byte[] {8, 123, 18, 7, 102, 111, 111, 32, 98, 97, 114});
}
private static void assertRoundTrip(SerializableProto.Builder input) {
ProtobufSerializer<SerializableProto> s = new ProtobufSerializer<>(SerializableProto.parser());
assertThat(s.deserialize(s.serialize(input.build()))).isEqualTo(input.build());
}
}

View File

@ -0,0 +1,49 @@
// Copyright (C) 2019 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.server.cache.testing.CacheSerializerTestUtil.byteArray;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.cache.proto.Cache;
import com.google.protobuf.ByteString;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
public class PureRevertCacheKeyTest {
@Test
public void serialization() {
ObjectId revert = ObjectId.zeroId();
ObjectId original = ObjectId.fromString("aabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
byte[] serializedRevert =
new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
byte[] serializedOriginal =
byteArray(
0xaa, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb,
0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb);
Cache.PureRevertKeyProto key =
PureRevertCache.key(new Project.NameKey("test"), revert, original);
assertThat(key)
.isEqualTo(
Cache.PureRevertKeyProto.newBuilder()
.setProject("test")
.setClaimedRevert(ByteString.copyFrom(serializedRevert))
.setClaimedOriginal(ByteString.copyFrom(serializedOriginal))
.build());
}
}

View File

@ -234,3 +234,11 @@ message AllExternalIdsProto {
}
repeated ExternalIdProto external_id = 1;
}
// Key for com.google.gerrit.server.git.PureRevertCache.
// Next ID: 4
message PureRevertKeyProto {
string project = 1;
bytes claimed_original = 2;
bytes claimed_revert = 3;
}

12
proto/testing/BUILD Normal file
View File

@ -0,0 +1,12 @@
proto_library(
name = "test_proto",
testonly = 1,
srcs = ["test.proto"],
)
java_proto_library(
name = "test_java_proto",
testonly = 1,
visibility = ["//visibility:public"],
deps = [":test_proto"],
)

26
proto/testing/test.proto Normal file
View File

@ -0,0 +1,26 @@
// Copyright (C) 2019 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
syntax = "proto2";
package devtools.gerritcodereview.testing;
option java_package = "com.google.gerrit.proto.testing";
// Test type for ProtobufSerializerTest
// Next ID: 3
message SerializableProto {
required int32 id = 1;
optional string text = 2;
}