DiffOperations: Move the timeout logic inside the diff cache
In this change we do the following:
1) We move the timeout logic from DiffOperations to the
GitFileDiffCache. The timeout is enforced while computing the file
header (similar to the old diff cache, see [2]). If the computation
times out, we cache negative results. We enforce the timeout with a
boolean variable (see (2)). The FileDiffCache (which wraps the
GitFileDiffCache and performs extra logic, e.g. computing rebase edits)
will also cache negative results.
2) Add an extra boolean field to the GitFileDiffCacheKey and the
FileDiffCacheKey called "useTimeout". We only enforce timeouts if the
diff algorithm uses the Myers diff fallback.
The new logic now goes as follows: DiffOperations requests diffs using
the HISTOGRAM_WITH_MYERS algorithm and with useTimeout=true. If we get
back negative results (meaning that the computation times out), we
request the diffs again using the HISTOGRAM_NO_FALLBACK and with
useTimeouts=false (we don't want to enforce timeouts in this case). This
logic is matching what was done in old diff cache.
The reasonning behind this change is the following:
* The timeout logic in the old diff cache was added in the past (many
years ago) because of a bug in the Myers diff algorithm in JGit that
causes an infinite loop. See Ib00de070 and [1]. It looks like the bug
isn't fixed yet. The timeout was only employed while computing the file
header (see [2]) so was not enforced on the whole diff computation. [1]
suggets that if the JGit bug in Myers diff is fixed then we can remove
the timeouts in Gerrit, i.e. we did introduce the timeout logic in
Gerrit as a workaround for this bug.
* It wasn't a good idea to keep the timeout logic in DiffOperations:
This caused the @DiffExecutor to create new threads with every request
to get the diff, whether it's cached or not, and caused an overload to
the diff executor. With the logic of this change, once the result is
cached (whether positive or negative), we will not have to go through
the cache loader (or the timeout) logic again.
[1] bugs.chromium.org/p/gerrit/issues/detail?id=487
[2] https://gerrit.googlesource.com/gerrit/+/0dc27d2a5b512fdcb7ce5f95b412e174fe33c7d2/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java#530
Change-Id: I680454cf93f78b90025bab95ed3717c74dbf32bf
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index 5183f8c..dccb7e7 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -19,6 +19,7 @@
import static org.eclipse.jgit.lib.Constants.EMPTY_TREE_ID;
import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.flogger.FluentLogger;
@@ -29,8 +30,6 @@
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.config.ConfigUtil;
-import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.patch.diff.ModifiedFilesCache;
import com.google.gerrit.server.patch.diff.ModifiedFilesCacheImpl;
import com.google.gerrit.server.patch.diff.ModifiedFilesCacheKey;
@@ -49,12 +48,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Future;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
/**
@@ -73,8 +66,6 @@
private final ModifiedFilesCache modifiedFilesCache;
private final FileDiffCache fileDiffCache;
private final BaseCommitUtil baseCommitUtil;
- private final long timeoutMillis;
- private final ExecutorService diffExecutor;
public static Module module() {
return new CacheModule() {
@@ -93,21 +84,10 @@
public DiffOperationsImpl(
ModifiedFilesCache modifiedFilesCache,
FileDiffCache fileDiffCache,
- BaseCommitUtil baseCommit,
- @DiffExecutor ExecutorService executor,
- @GerritServerConfig Config cfg) {
+ BaseCommitUtil baseCommit) {
this.modifiedFilesCache = modifiedFilesCache;
this.fileDiffCache = fileDiffCache;
this.baseCommitUtil = baseCommit;
- this.diffExecutor = executor;
- this.timeoutMillis =
- ConfigUtil.getTimeUnit(
- cfg,
- "cache",
- "diff",
- "timeout",
- TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
- TimeUnit.MILLISECONDS);
}
@Override
@@ -116,7 +96,7 @@
throws DiffNotAvailableException {
try {
DiffParameters diffParams = computeDiffParameters(project, newCommit, parent);
- return listModifiedFilesWithTimeout(diffParams);
+ return getModifiedFiles(diffParams);
} catch (IOException e) {
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
@@ -133,9 +113,8 @@
.newCommit(newCommit)
.baseCommit(oldCommit)
.comparisonType(ComparisonType.againstOtherPatchSet())
- .diffAlgorithm(DEFAULT_DIFF_ALGORITHM)
.build();
- return listModifiedFilesWithTimeout(params);
+ return getModifiedFiles(params);
}
@Override
@@ -154,9 +133,10 @@
diffParams.baseCommit(),
newCommit,
fileName,
- diffParams.diffAlgorithm(),
+ DEFAULT_DIFF_ALGORITHM,
+ /* useTimeout= */ true,
whitespace);
- return getModifiedFileWithTimeout(key, diffParams);
+ return getModifiedFileForKey(key);
} catch (IOException e) {
throw new DiffNotAvailableException(
"Failed to evaluate the parent/base commit for commit " + newCommit, e);
@@ -171,85 +151,16 @@
String fileName,
@Nullable DiffPreferencesInfo.Whitespace whitespace)
throws DiffNotAvailableException {
- DiffParameters params = // used for logging only
- DiffParameters.builder()
- .project(project)
- .baseCommit(oldCommit)
- .newCommit(newCommit)
- .comparisonType(ComparisonType.againstOtherPatchSet())
- .diffAlgorithm(DEFAULT_DIFF_ALGORITHM)
- .build();
FileDiffCacheKey key =
createFileDiffCacheKey(
- project, oldCommit, newCommit, fileName, params.diffAlgorithm(), whitespace);
- return getModifiedFileWithTimeout(key, params);
- }
-
- /**
- * Computes the list of modified files between two commits using the {@link
- * DiffAlgorithm#HISTOGRAM_WITH_FALLBACK_MYERS} diff algorithm. The computation runs with the
- * timeout specified by {@link #timeoutMillis} milliseconds. If the timeout is reached, this
- * method falls back to the {@link DiffAlgorithm#HISTOGRAM_NO_FALLBACK}.
- */
- private Map<String, FileDiffOutput> listModifiedFilesWithTimeout(DiffParameters params)
- throws DiffNotAvailableException {
- Future<DiffResult> task =
- diffExecutor.submit(
- () -> {
- ImmutableMap<String, FileDiffOutput> modifiedFiles = getModifiedFiles(params);
- return DiffResult.create(null, modifiedFiles);
- });
- try {
- return task.get(timeoutMillis, TimeUnit.MILLISECONDS).modifiedFiles();
- } catch (InterruptedException | TimeoutException e) {
- logger.atWarning().withCause(e).log(
- String.format(
- "Timeout reached while computing diff for project %s, old commit %s, new commit %s."
- + " Retrying using the fallback algorithm.",
- params.project(), params.baseCommit().name(), params.newCommit().name()));
- task.cancel(/* mayInterruptIfRunning= */ true);
- DiffParameters newParams =
- params.toBuilder().diffAlgorithm(DiffAlgorithm.HISTOGRAM_NO_FALLBACK).build();
- ImmutableMap<String, FileDiffOutput> modifiedFiles = getModifiedFiles(newParams);
- return DiffResult.create(null, modifiedFiles).modifiedFiles();
- } catch (ExecutionException e) {
- throw new DiffNotAvailableException(e);
- }
- }
-
- /**
- * Computes the file diff for a single file using the {@link
- * DiffAlgorithm#HISTOGRAM_WITH_FALLBACK_MYERS} diff algorithm. The computation runs with the
- * timeout specified by {@link #timeoutMillis} milliseconds. If the timeout is reached, this
- * method falls back to the {@link DiffAlgorithm#HISTOGRAM_NO_FALLBACK}.
- */
- private FileDiffOutput getModifiedFileWithTimeout(FileDiffCacheKey key, DiffParameters params)
- throws DiffNotAvailableException {
- Future<DiffResult> task = diffExecutor.submit(() -> getModifiedFileForKey(key));
- try {
- return task.get(timeoutMillis, TimeUnit.MILLISECONDS).fileDiff();
- } catch (InterruptedException | TimeoutException e) {
- logger.atWarning().withCause(e).log(
- String.format(
- "Timeout reached while computing diff for project %s, old commit %s, new commit %s."
- + " Recomputing using the fallback algorithm.",
- params.project(), params.baseCommit().name(), params.newCommit().name()));
- task.cancel(/* mayInterruptIfRunning= */ true);
- FileDiffCacheKey newKey =
- key.toBuilder().diffAlgorithm(DiffAlgorithm.HISTOGRAM_NO_FALLBACK).build();
- return getModifiedFileForKey(newKey).fileDiff();
- } catch (ExecutionException e) {
- throw new DiffNotAvailableException(e);
- }
- }
-
- private DiffResult getModifiedFileForKey(FileDiffCacheKey key) throws DiffNotAvailableException {
- Map<String, FileDiffOutput> diffList = getModifiedFilesForKeys(ImmutableList.of(key));
- FileDiffOutput fileDiffOutput =
- diffList.containsKey(key.newFilePath())
- ? diffList.get(key.newFilePath())
- : FileDiffOutput.empty(key.newFilePath(), key.oldCommit(), key.newCommit());
- return DiffResult.create(fileDiffOutput, null);
+ project,
+ oldCommit,
+ newCommit,
+ fileName,
+ DEFAULT_DIFF_ALGORITHM,
+ /* useTimeout= */ true,
+ whitespace);
+ return getModifiedFileForKey(key);
}
private ImmutableMap<String, FileDiffOutput> getModifiedFiles(DiffParameters diffParams)
@@ -270,7 +181,8 @@
oldCommit,
newCommit,
COMMIT_MSG,
- diffParams.diffAlgorithm(),
+ DEFAULT_DIFF_ALGORITHM,
+ /* useTimeout= */ true,
/* whitespace= */ null));
if (cmp.isAgainstAutoMerge() || isMergeAgainstParent(cmp, project, newCommit)) {
@@ -280,7 +192,8 @@
oldCommit,
newCommit,
MERGE_LIST,
- diffParams.diffAlgorithm(),
+ DEFAULT_DIFF_ALGORITHM,
+ /* useTimeout= */ true,
/*whitespace = */ null));
}
@@ -295,7 +208,8 @@
entity.newPath().isPresent()
? entity.newPath().get()
: entity.oldPath().get(),
- diffParams.diffAlgorithm(),
+ DEFAULT_DIFF_ALGORITHM,
+ /* useTimeout= */ true,
/* whitespace= */ null))
.forEach(fileCacheKeys::add);
}
@@ -305,22 +219,71 @@
}
}
+ private FileDiffOutput getModifiedFileForKey(FileDiffCacheKey key)
+ throws DiffNotAvailableException {
+ Map<String, FileDiffOutput> diffList = getModifiedFilesForKeys(ImmutableList.of(key));
+ return diffList.containsKey(key.newFilePath())
+ ? diffList.get(key.newFilePath())
+ : FileDiffOutput.empty(key.newFilePath(), key.oldCommit(), key.newCommit());
+ }
+
+ /**
+ * Lookup the file diffs for the input {@code keys}. For results where the cache reports negative
+ * results, e.g. due to timeouts in the cache loader, this method requests the diff again using
+ * the fallback algorithm {@link DiffAlgorithm#HISTOGRAM_NO_FALLBACK}.
+ */
private ImmutableMap<String, FileDiffOutput> getModifiedFilesForKeys(List<FileDiffCacheKey> keys)
throws DiffNotAvailableException {
- ImmutableMap.Builder<String, FileDiffOutput> files = ImmutableMap.builder();
ImmutableMap<FileDiffCacheKey, FileDiffOutput> fileDiffs = fileDiffCache.getAll(keys);
+ List<FileDiffCacheKey> fallbackKeys = new ArrayList<>();
- for (FileDiffOutput fileDiffOutput : fileDiffs.values()) {
+ ImmutableList.Builder<FileDiffOutput> result = ImmutableList.builder();
+
+ // Use the fallback diff algorithm for negative results
+ for (FileDiffCacheKey key : fileDiffs.keySet()) {
+ FileDiffOutput diff = fileDiffs.get(key);
+ if (diff.isNegative()) {
+ FileDiffCacheKey fallbackKey =
+ createFileDiffCacheKey(
+ key.project(),
+ key.oldCommit(),
+ key.newCommit(),
+ key.newFilePath(),
+ // Use the fallback diff algorithm
+ DiffAlgorithm.HISTOGRAM_NO_FALLBACK,
+ // We don't enforce timeouts with the fallback algorithm. Timeouts were introduced
+ // because of a bug in JGit that happens only when the histogram algorithm uses
+ // Myers as fallback. See https://bugs.chromium.org/p/gerrit/issues/detail?id=487
+ /* useTimeout= */ false,
+ key.whitespace());
+ fallbackKeys.add(fallbackKey);
+ } else {
+ result.add(diff);
+ }
+ }
+ result.addAll(fileDiffCache.getAll(fallbackKeys).values());
+ return mapByFilePath(result.build());
+ }
+
+ /**
+ * Map a collection of {@link FileDiffOutput} based on their file paths. The result map keys
+ * represent the old file path for deleted files, or the new path otherwise.
+ */
+ private ImmutableMap<String, FileDiffOutput> mapByFilePath(
+ ImmutableCollection<FileDiffOutput> fileDiffOutputs) {
+ ImmutableMap.Builder<String, FileDiffOutput> diffs = ImmutableMap.builder();
+
+ for (FileDiffOutput fileDiffOutput : fileDiffOutputs) {
if (fileDiffOutput.isEmpty() || allDueToRebase(fileDiffOutput)) {
continue;
}
if (fileDiffOutput.changeType() == ChangeType.DELETED) {
- files.put(fileDiffOutput.oldPath().get(), fileDiffOutput);
+ diffs.put(fileDiffOutput.oldPath().get(), fileDiffOutput);
} else {
- files.put(fileDiffOutput.newPath().get(), fileDiffOutput);
+ diffs.put(fileDiffOutput.newPath().get(), fileDiffOutput);
}
}
- return files.build();
+ return diffs.build();
}
private static boolean allDueToRebase(FileDiffOutput fileDiffOutput) {
@@ -350,6 +313,7 @@
ObjectId bCommit,
String newPath,
DiffAlgorithm diffAlgorithm,
+ boolean useTimeout,
@Nullable Whitespace whitespace) {
whitespace = whitespace == null ? DEFAULT_WHITESPACE : whitespace;
return FileDiffCacheKey.builder()
@@ -360,25 +324,10 @@
.renameScore(RENAME_SCORE)
.diffAlgorithm(diffAlgorithm)
.whitespace(whitespace)
+ .useTimeout(useTimeout)
.build();
}
- /** All interface methods create their results using this class. */
- @AutoValue
- abstract static class DiffResult {
- static DiffResult create(
- @Nullable FileDiffOutput fileDiff,
- @Nullable ImmutableMap<String, FileDiffOutput> modifiedFiles) {
- return new AutoValue_DiffOperationsImpl_DiffResult(fileDiff, modifiedFiles);
- }
-
- @Nullable
- abstract FileDiffOutput fileDiff();
-
- @Nullable
- abstract ImmutableMap<String, FileDiffOutput> modifiedFiles();
- }
-
@AutoValue
abstract static class DiffParameters {
abstract Project.NameKey project();
@@ -400,14 +349,10 @@
@Nullable
abstract Boolean skipFiles();
- abstract DiffAlgorithm diffAlgorithm();
-
static Builder builder() {
return new AutoValue_DiffOperationsImpl_DiffParameters.Builder();
}
- abstract Builder toBuilder();
-
@AutoValue.Builder
abstract static class Builder {
@@ -421,8 +366,6 @@
abstract Builder skipFiles(@Nullable Boolean skipFiles);
- abstract Builder diffAlgorithm(DiffAlgorithm diffAlgorithm);
-
abstract Builder comparisonType(ComparisonType comparisonType);
public abstract DiffParameters build();
@@ -433,11 +376,7 @@
private DiffParameters computeDiffParameters(
Project.NameKey project, ObjectId newCommit, Integer parent) throws IOException {
DiffParameters.Builder result =
- DiffParameters.builder()
- .project(project)
- .newCommit(newCommit)
- .parent(parent)
- .diffAlgorithm(DEFAULT_DIFF_ALGORITHM);
+ DiffParameters.builder().project(project).newCommit(newCommit).parent(parent);
if (parent != null) {
result.baseCommit(baseCommitUtil.getBaseCommit(project, newCommit, parent));
result.comparisonType(ComparisonType.againstParent(parent));
diff --git a/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java b/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java
index 12decc3..e3db1ce 100644
--- a/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java
+++ b/java/com/google/gerrit/server/patch/filediff/AllDiffsEvaluator.java
@@ -213,6 +213,7 @@
.renameScore(key.renameScore())
.diffAlgorithm(key.diffAlgorithm())
.whitespace(key.whitespace())
+ .useTimeout(key.useTimeout())
.build();
}
}
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
index ee5e156..d5ef546 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
@@ -99,7 +99,7 @@
persist(DIFF, FileDiffCacheKey.class, FileDiffOutput.class)
.maximumWeight(10 << 20)
.weigher(FileDiffWeigher.class)
- .version(6)
+ .version(7)
.keySerializer(FileDiffCacheKey.Serializer.INSTANCE)
.valueSerializer(FileDiffOutput.Serializer.INSTANCE)
.loader(FileDiffLoader.class);
@@ -392,6 +392,19 @@
for (AugmentedFileDiffCacheKey augmentedKey : allFileDiffs.keySet()) {
AllFileGitDiffs allDiffs = allFileDiffs.get(augmentedKey);
+ GitFileDiff mainGitDiff = allDiffs.mainDiff().gitDiff();
+
+ if (mainGitDiff.isNegative()) {
+ // If the result of the git diff computation was negative, i.e. due to timeout, cache a
+ // negative result.
+ result.put(
+ augmentedKey.key(),
+ FileDiffOutput.createNegative(
+ mainGitDiff.newPath().orElse(""),
+ augmentedKey.key().oldCommit(),
+ augmentedKey.key().newCommit()));
+ continue;
+ }
FileEdits rebaseFileEdits = FileEdits.empty();
if (!augmentedKey.ignoreRebase()) {
@@ -401,7 +414,6 @@
RevTree aTree = rw.parseTree(allDiffs.mainDiff().gitKey().oldTree());
RevTree bTree = rw.parseTree(allDiffs.mainDiff().gitKey().newTree());
- GitFileDiff mainGitDiff = allDiffs.mainDiff().gitDiff();
Long oldSize =
mainGitDiff.oldMode().isPresent() && mainGitDiff.oldPath().isPresent()
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java
index 26aa2b0..07493a2 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheKey.java
@@ -59,6 +59,9 @@
public abstract DiffPreferencesInfo.Whitespace whitespace();
+ /** Employ a timeout on the git computation while formatting the file header. */
+ public abstract boolean useTimeout();
+
/** Number of bytes that this entity occupies. */
public int weight() {
return stringSize(project().get())
@@ -66,7 +69,8 @@
+ stringSize(newFilePath())
+ 4 // renameScore
+ 4 // diffAlgorithm
- + 4; // whitespace
+ + 4 // whitespace
+ + 1; // useTimeout
}
public static FileDiffCacheKey.Builder builder() {
@@ -97,6 +101,8 @@
public abstract FileDiffCacheKey.Builder whitespace(Whitespace value);
+ public abstract FileDiffCacheKey.Builder useTimeout(boolean value);
+
public abstract FileDiffCacheKey build();
}
@@ -115,6 +121,7 @@
.setRenameScore(key.renameScore())
.setDiffAlgorithm(key.diffAlgorithm().name())
.setWhitespace(key.whitespace().name())
+ .setUseTimeout(key.useTimeout())
.build());
}
@@ -130,6 +137,7 @@
.renameScore(proto.getRenameScore())
.diffAlgorithm(DiffAlgorithm.valueOf(proto.getDiffAlgorithm()))
.whitespace(Whitespace.valueOf(proto.getWhitespace()))
+ .useTimeout(proto.getUseTimeout())
.build();
}
}
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
index e7f47ef..9a6bbc3 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
@@ -79,6 +79,12 @@
/** Difference in file size between the old and new commits. */
public abstract long sizeDelta();
+ /**
+ * Returns {@code true} if the diff computation was not able to compute a diff, i.e. for diffs
+ * taking a very long time to compute. We cache negative result in this case.
+ */
+ public abstract Optional<Boolean> negative();
+
/** A boolean indicating if all underlying edits of the file diff are due to rebase. */
public boolean allEditsDueToRebase() {
return !edits().isEmpty() && edits().stream().allMatch(TaggedEdit::dueToRebase);
@@ -122,15 +128,32 @@
.build();
}
+ public static FileDiffOutput createNegative(
+ String filePath, ObjectId oldCommitId, ObjectId newCommitId) {
+ return empty(filePath, oldCommitId, newCommitId).toBuilder().build();
+ }
+
/** Returns true if this entity represents an unchanged file between two commits. */
public boolean isEmpty() {
return headerLines().isEmpty() && edits().isEmpty();
}
+ /**
+ * Returns {@code true} if the diff computation was not able to compute a diff. We cache negative
+ * result in this case.
+ */
+ public boolean isNegative() {
+ return negative().isPresent() && negative().get();
+ }
+
public static Builder builder() {
return new AutoValue_FileDiffOutput.Builder();
}
+ public static Builder toBuilder() {
+ return new AutoValue_FileDiffOutput.Builder();
+ }
+
public int weight() {
int result = 0;
if (oldPath().isPresent()) {
@@ -151,6 +174,9 @@
for (String s : headerLines()) {
s += stringSize(s);
}
+ if (negative().isPresent()) {
+ result += 1;
+ }
return result;
}
@@ -179,6 +205,8 @@
public abstract Builder sizeDelta(long value);
+ public abstract Builder negative(Optional<Boolean> value);
+
public abstract FileDiffOutput build();
}
@@ -194,6 +222,9 @@
private static final FieldDescriptor PATCH_TYPE_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(4);
+ private static final FieldDescriptor NEGATIVE_DESCRIPTOR =
+ FileDiffOutputProto.getDescriptor().findFieldByNumber(12);
+
@Override
public byte[] serialize(FileDiffOutput fileDiff) {
ObjectIdConverter idConverter = ObjectIdConverter.create();
@@ -234,6 +265,10 @@
builder.setPatchType(fileDiff.patchType().get().name());
}
+ if (fileDiff.negative().isPresent()) {
+ builder.setNegative(fileDiff.negative().get());
+ }
+
return Protos.toByteArray(builder.build());
}
@@ -272,6 +307,9 @@
if (proto.hasField(PATCH_TYPE_DESCRIPTOR)) {
builder.patchType(Optional.of(Patch.PatchType.valueOf(proto.getPatchType())));
}
+ if (proto.hasField(NEGATIVE_DESCRIPTOR)) {
+ builder.negative(Optional.of(proto.getNegative()));
+ }
return builder.build();
}
}
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
index e1af81d..2f2d29b 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
@@ -29,11 +29,9 @@
import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
import com.google.gerrit.server.patch.filediff.Edit;
import com.google.protobuf.Descriptors.FieldDescriptor;
-import java.io.IOException;
import java.util.Map;
import java.util.Optional;
import org.eclipse.jgit.diff.DiffEntry;
-import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.lib.AbbreviatedObjectId;
import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.patch.FileHeader;
@@ -65,8 +63,7 @@
* Creates a {@link GitFileDiff} using the {@code diffEntry} and the {@code diffFormatter}
* parameters.
*/
- static GitFileDiff create(DiffEntry diffEntry, DiffFormatter diffFormatter) throws IOException {
- FileHeader fileHeader = diffFormatter.toFileHeader(diffEntry);
+ static GitFileDiff create(DiffEntry diffEntry, FileHeader fileHeader) {
ImmutableList<Edit> edits =
fileHeader.toEditList().stream().map(Edit::fromJGitEdit).collect(toImmutableList());
@@ -102,6 +99,15 @@
.build();
}
+ /**
+ * Create a negative result to be cached, i.e. if the diff computation did not finish in a
+ * reasonable amount of time.
+ */
+ static GitFileDiff createNegative(
+ AbbreviatedObjectId oldId, AbbreviatedObjectId newId, String newFilePath) {
+ return empty(oldId, newId, newFilePath).toBuilder().negative(Optional.of(true)).build();
+ }
+
/** An {@link ImmutableList} of the modified regions in the file. */
public abstract ImmutableList<Edit> edits();
@@ -133,6 +139,12 @@
public abstract Optional<PatchType> patchType();
/**
+ * Returns {@code true} if the diff computation was not able to compute a diff. We cache negative
+ * result in this case.
+ */
+ public abstract Optional<Boolean> negative();
+
+ /**
* Returns true if the object was created using the {@link #empty(AbbreviatedObjectId,
* AbbreviatedObjectId, String)} method.
*/
@@ -140,6 +152,14 @@
return edits().isEmpty();
}
+ /**
+ * Returns {@code true} if the diff computation was not able to compute a diff. We cache negative
+ * result in this case.
+ */
+ public boolean isNegative() {
+ return negative().isPresent() && negative().get();
+ }
+
/** Returns the size of the object in bytes. */
public int weight() {
int result = 20 * 2; // oldId and newId
@@ -161,6 +181,9 @@
if (newMode().isPresent()) {
result += 4;
}
+ if (negative().isPresent()) {
+ result += 1;
+ }
return result;
}
@@ -168,6 +191,8 @@
return new AutoValue_GitFileDiff.Builder();
}
+ public abstract Builder toBuilder();
+
@AutoValue.Builder
public abstract static class Builder {
@@ -191,6 +216,8 @@
public abstract Builder patchType(Optional<PatchType> value);
+ public abstract Builder negative(Optional<Boolean> value);
+
public abstract GitFileDiff build();
}
@@ -212,6 +239,9 @@
private static final FieldDescriptor PATCH_TYPE_DESCRIPTOR =
GitFileDiffProto.getDescriptor().findFieldByNumber(10);
+ private static final FieldDescriptor NEGATIVE_DESCRIPTOR =
+ GitFileDiffProto.getDescriptor().findFieldByNumber(11);
+
@Override
public byte[] serialize(GitFileDiff gitFileDiff) {
ObjectIdConverter idConverter = ObjectIdConverter.create();
@@ -246,6 +276,9 @@
if (gitFileDiff.patchType().isPresent()) {
builder.setPatchType(gitFileDiff.patchType().get().name());
}
+ if (gitFileDiff.negative().isPresent()) {
+ builder.setNegative(gitFileDiff.negative().get());
+ }
return Protos.toByteArray(builder.build());
}
@@ -279,6 +312,9 @@
if (proto.hasField(PATCH_TYPE_DESCRIPTOR)) {
builder.patchType(Optional.of(Patch.PatchType.valueOf(proto.getPatchType())));
}
+ if (proto.hasField(NEGATIVE_DESCRIPTOR)) {
+ builder.negative(Optional.of(proto.getNegative()));
+ }
return builder.build();
}
}
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
index 33773f6..abfd417 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
@@ -17,6 +17,7 @@
import static java.util.function.Function.identity;
import com.google.auto.value.AutoValue;
+import com.google.common.base.Throwables;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableList;
@@ -27,10 +28,13 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.config.ConfigUtil;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
+import com.google.gerrit.server.patch.DiffExecutor;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -42,6 +46,10 @@
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.eclipse.jgit.diff.DiffEntry;
@@ -50,9 +58,11 @@
import org.eclipse.jgit.diff.HistogramDiff;
import org.eclipse.jgit.diff.RawTextComparator;
import org.eclipse.jgit.lib.AbbreviatedObjectId;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.patch.FileHeader;
import org.eclipse.jgit.util.io.DisabledOutputStream;
/** Implementation of the {@link GitFileDiffCache} */
@@ -70,7 +80,7 @@
.weigher(GitFileDiffWeigher.class)
.keySerializer(GitFileDiffCacheKey.Serializer.INSTANCE)
.valueSerializer(GitFileDiff.Serializer.INSTANCE)
- .version(2)
+ .version(3)
.loader(GitFileDiffCacheImpl.Loader.class);
}
};
@@ -132,10 +142,24 @@
: entry.getNewPath();
private final GitRepositoryManager repoManager;
+ private final ExecutorService diffExecutor;
+ private final long timeoutMillis;
@Inject
- public Loader(GitRepositoryManager repoManager) {
+ public Loader(
+ @GerritServerConfig Config cfg,
+ GitRepositoryManager repoManager,
+ @DiffExecutor ExecutorService de) {
this.repoManager = repoManager;
+ this.diffExecutor = de;
+ this.timeoutMillis =
+ ConfigUtil.getTimeUnit(
+ cfg,
+ "cache",
+ "diff",
+ "timeout",
+ TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
+ TimeUnit.MILLISECONDS);
}
@Override
@@ -200,16 +224,18 @@
Map<String, DiffEntry> diffEntries = loadDiffEntries(formatter, options, filePaths.values());
for (GitFileDiffCacheKey key : filePaths.keySet()) {
String newFilePath = filePaths.get(key);
- if (diffEntries.containsKey(newFilePath)) {
- result.put(key, GitFileDiff.create(diffEntries.get(newFilePath), formatter));
+ if (!diffEntries.containsKey(newFilePath)) {
+ result.put(
+ key,
+ GitFileDiff.empty(
+ AbbreviatedObjectId.fromObjectId(key.oldTree()),
+ AbbreviatedObjectId.fromObjectId(key.newTree()),
+ newFilePath));
continue;
}
- result.put(
- key,
- GitFileDiff.empty(
- AbbreviatedObjectId.fromObjectId(key.oldTree()),
- AbbreviatedObjectId.fromObjectId(key.newTree()),
- newFilePath));
+ DiffEntry diffEntry = diffEntries.get(newFilePath);
+ GitFileDiff gitFileDiff = createGitFileDiff(diffEntry, formatter, key);
+ result.put(key, gitFileDiff);
}
return result.build();
}
@@ -258,6 +284,52 @@
return RawTextComparator.DEFAULT;
}
}
+
+ /**
+ * Create a {@link GitFileDiff}. The result depends on the value of the {@code useTimeout} field
+ * of the {@code key} parameter.
+ *
+ * <ul>
+ * <li>If {@code useTimeout} is true, the computation is performed with timeout enforcement
+ * (identified by {@link #timeoutMillis}). If the timeout is exceeded, this method returns
+ * a negative result using {@link GitFileDiff#createNegative(AbbreviatedObjectId,
+ * AbbreviatedObjectId, String)}.
+ * <li>If {@code useTimeouts} is false, the computation is performed synchronously without
+ * timeout enforcement.
+ */
+ private GitFileDiff createGitFileDiff(
+ DiffEntry diffEntry, DiffFormatter formatter, GitFileDiffCacheKey key) throws IOException {
+ if (!key.useTimeout()) {
+ FileHeader fileHeader = formatter.toFileHeader(diffEntry);
+ return GitFileDiff.create(diffEntry, fileHeader);
+ }
+ Future<FileHeader> fileHeaderFuture =
+ diffExecutor.submit(
+ () -> {
+ synchronized (diffEntry) {
+ return formatter.toFileHeader(diffEntry);
+ }
+ });
+ try {
+ // We employ the timeout because of a bug in Myers diff in JGit. See
+ // bugs.chromium.org/p/gerrit/issues/detail?id=487 for more details. The bug may happen
+ // if the algorithm used in diffs is HISTOGRAM_WITH_FALLBACK_MYERS.
+ fileHeaderFuture.get(timeoutMillis, TimeUnit.MILLISECONDS);
+ FileHeader fileHeader = formatter.toFileHeader(diffEntry);
+ return GitFileDiff.create(diffEntry, fileHeader);
+ } catch (InterruptedException | TimeoutException e) {
+ // If timeout happens, create a negative result
+ return GitFileDiff.createNegative(
+ AbbreviatedObjectId.fromObjectId(key.oldTree()),
+ AbbreviatedObjectId.fromObjectId(key.newTree()),
+ key.newFilePath());
+ } catch (ExecutionException e) {
+ // If there was an error computing the result, carry it
+ // up to the caller so the cache knows this key is invalid.
+ Throwables.throwIfInstanceOf(e.getCause(), IOException.class);
+ throw new IOException(e.getMessage(), e.getCause());
+ }
+ }
}
/** An entity representing the options affecting the diff computation. */
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java
index f104388..bb3dd41 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheKey.java
@@ -53,13 +53,17 @@
public abstract DiffPreferencesInfo.Whitespace whitespace();
+ /** Employ a timeout on the git computation while formatting the file header. */
+ public abstract boolean useTimeout();
+
public int weight() {
return stringSize(project().get())
+ 20 * 2 // oldTree and newTree
+ stringSize(newFilePath())
+ 4 // renameScore
+ 4 // diffAlgorithm
- + 4; // whitespace
+ + 4 // whitespace
+ + 1; // useTimeout
}
public static Builder builder() {
@@ -88,6 +92,8 @@
public abstract Builder whitespace(Whitespace value);
+ public abstract Builder useTimeout(boolean value);
+
public abstract GitFileDiffCacheKey build();
}
@@ -106,6 +112,7 @@
.setRenameScore(key.renameScore())
.setDiffAlgorithm(key.diffAlgorithm().name())
.setWhitepsace(key.whitespace().name())
+ .setUseTimeout(key.useTimeout())
.build());
}
@@ -121,6 +128,7 @@
.renameScore(proto.getRenameScore())
.diffAlgorithm(DiffAlgorithm.valueOf(proto.getDiffAlgorithm()))
.whitespace(Whitespace.valueOf(proto.getWhitepsace()))
+ .useTimeout(proto.getUseTimeout())
.build();
}
}
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffCacheKeySerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffCacheKeySerializerTest.java
index d738bf6..bb924ca 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffCacheKeySerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffCacheKeySerializerTest.java
@@ -41,6 +41,7 @@
.renameScore(65)
.diffAlgorithm(DiffAlgorithm.HISTOGRAM_WITH_FALLBACK_MYERS)
.whitespace(Whitespace.IGNORE_ALL)
+ .useTimeout(true)
.build();
byte[] serialized = FileDiffCacheKey.Serializer.INSTANCE.serialize(key);
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
index 17fd959..c5e8574 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
@@ -48,6 +48,7 @@
.sizeDelta(10)
.headerLines(ImmutableList.of("header line 1", "header line 2"))
.edits(edits)
+ .negative(Optional.of(true))
.build();
byte[] serialized = FileDiffOutput.Serializer.INSTANCE.serialize(fileDiff);
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffKeySerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffKeySerializerTest.java
index d17bf8e..d7fdfe6 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffKeySerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffKeySerializerTest.java
@@ -40,6 +40,7 @@
.renameScore(65)
.diffAlgorithm(DiffAlgorithm.HISTOGRAM_WITH_FALLBACK_MYERS)
.whitespace(Whitespace.IGNORE_ALL)
+ .useTimeout(true)
.build();
byte[] serialized = GitFileDiffCacheKey.Serializer.INSTANCE.serialize(key);
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffSerializerTest.java
index 93441a4..e73c774 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/GitFileDiffSerializerTest.java
@@ -51,6 +51,7 @@
.patchType(Optional.of(PatchType.UNIFIED))
.oldMode(Optional.of(FileMode.REGULAR_FILE))
.newMode(Optional.of(FileMode.REGULAR_FILE))
+ .negative(Optional.of(true))
.build();
byte[] serialized = Serializer.INSTANCE.serialize(gitFileDiff);
diff --git a/proto/cache.proto b/proto/cache.proto
index 9d2ba06..781538a 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -607,7 +607,7 @@
// Serialized form of a collection of
// com.google.gerrit.server.patch.gitfilediff.GitFileDiffCacheImpl.Key
-// Next ID: 8
+// Next ID: 9
message GitFileDiffKeyProto {
string project = 1;
bytes a_tree = 2;
@@ -616,10 +616,11 @@
int32 rename_score = 5;
string diff_algorithm = 6; // ENUM as string
string whitepsace = 7; // ENUM as string
+ bool useTimeout = 8;
}
// Serialized form of com.google.gerrit.server.patch.gitfilediff.GitFileDiff
-// Next ID: 11
+// Next ID: 12
message GitFileDiffProto {
message Edit {
int32 begin_a = 1;
@@ -637,11 +638,12 @@
string new_mode = 8; // ENUM as string
string change_type = 9; // ENUM as string
string patch_type = 10; // ENUM as string
+ bool negative = 11;
}
// Serialized form of
// com.google.gerrit.server.patch.fileDiff.FileDiffCacheKey
-// Next ID: 8
+// Next ID: 9
message FileDiffKeyProto {
string project = 1;
bytes old_commit = 2;
@@ -650,11 +652,12 @@
int32 rename_score = 5;
string diff_algorithm = 6;
string whitespace = 7;
+ bool useTimeout = 8;
}
// Serialized form of
// com.google.gerrit.server.patch.filediff.FileDiffOutput
-// Next ID: 12
+// Next ID: 13
message FileDiffOutputProto {
// Next ID: 5
message Edit {
@@ -685,6 +688,7 @@
bytes old_commit = 9;
bytes new_commit = 10;
ComparisonType comparison_type = 11;
+ bool negative = 12;
}
// Serialized form of com.google.gerrit.server.approval.ApprovalCacheImpl.Key.
