opendev/gerrit
Code Issues Proposed changes

Cache negative results in PatchListCache

Browse Source 
Computing PatchLists can fail with LargeObjectException. In this case,
computing the PatchList was very expensive in the first place. This
commit adds logic to cache such negative results and don't attempt to
recompute the PatchList since it would yield the same result.

At Google, we see a lot of CPU time being spent on computing PatchLists
for large objects that eventually fail because of LargeObjectExceptions.
Caching negative results will at least prevent pointless retries.

If the LargeObject threshold is changed, host admins can flush the cache
or manually remove the tombstones.

Change-Id: Ia87cebede79fb1c933eb33072ec9a76768938afd
This commit is contained in:
Patrick Hiesel
2017-07-18 11:46:10 +02:00
committed by Edwin Kempin
parent 252e197e21
commit 8d26b686cf
2 changed files with 16 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchList.java
View File

@@ -105,6 +105,8 @@ public class PatchList implements Serializable {
this.patches = patches; this.patches = patches;
} }
protected PatchList() {}
/** Old side tree or commit; null only if this is a combined diff. */ /** Old side tree or commit; null only if this is a combined diff. */
@Nullable @Nullable
public ObjectId getOldId() { public ObjectId getOldId() {

14
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
View File

@@ -101,6 +101,10 @@ public class PatchListCacheImpl implements PatchListCache {
throws PatchListNotAvailableException { throws PatchListNotAvailableException {
try { try {
PatchList pl = fileCache.get(key, fileLoaderFactory.create(key, project)); PatchList pl = fileCache.get(key, fileLoaderFactory.create(key, project));
if (pl instanceof LargeObjectTombstone) {
throw new PatchListNotAvailableException(
"Error computing " + key + ". Previous attempt failed with LargeObjectException");
}
if (key.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF) { if (key.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF) {
diffSummaryCache.put(DiffSummaryKey.fromPatchListKey(key), toDiffSummary(pl)); diffSummaryCache.put(DiffSummaryKey.fromPatchListKey(key), toDiffSummary(pl));
} }
@@ -110,6 +114,9 @@ public class PatchListCacheImpl implements PatchListCache {
throw new PatchListNotAvailableException(e); throw new PatchListNotAvailableException(e);
} catch (UncheckedExecutionException e) { } catch (UncheckedExecutionException e) {
if (e.getCause() instanceof LargeObjectException) { if (e.getCause() instanceof LargeObjectException) {
// Cache negative result so we don't need to redo expensive computations that would yield
// the same result.
fileCache.put(key, new LargeObjectTombstone());
PatchListLoader.log.warn("Error computing " + key, e); PatchListLoader.log.warn("Error computing " + key, e);
throw new PatchListNotAvailableException(e); throw new PatchListNotAvailableException(e);
} }
@@ -181,4 +188,11 @@ public class PatchListCacheImpl implements PatchListCache {
throw e; throw e;
} }
} }
/** Used to cache negative results in {@code fileCache}. */
private class LargeObjectTombstone extends PatchList {
private static final long serialVersionUID = 1L;
private LargeObjectTombstone() {}
}
} }