diff options
12 files changed, 335 insertions, 37 deletions
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 098bfffb2d..2fbe58ca76 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -397,6 +397,35 @@ See also link:cmd-flush-caches.html[gerrit flush-caches]. [[cache_options]]Cache Options ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +cache.diff_intraline.maxIdleWorkers:: ++ +Number of idle worker threads to maintain for the intraline difference +computations. There is no upper bound on how many concurrent requests +can occur at once, if additional threads are started to handle a peak +load, only this many will remaining idle afterwards. ++ +Default is 1.5x number of available CPUs. + +cache.diff_intraline.timeout:: ++ +Maximum number of milliseconds to wait for intraline difference data +before giving up and disabling it for a particular file pair. This is +a work around for an infinite loop bug in the intraline difference +implementation. If computation takes longer than the timeout the +worker thread is terminated and no intraline difference is displayed. ++ +Values should use common unit suffixes to express their setting: ++ +* ms, milliseconds +* s, sec, second, seconds +* m, min, minute, minutes +* h, hr, hour, hours + ++ +If a unit suffix is not specified, `milliseconds` is assumed. ++ +Default is 5 seconds. + cache.diff_intraline.enabled:: + Boolean to enable or disable the computation of intraline differences diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchScript.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchScript.java index e24405fe53..24cdee4452 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchScript.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchScript.java @@ -55,6 +55,7 @@ public class PatchScript { protected List<Patch> history; protected boolean hugeFile; protected boolean intralineDifference; + protected boolean intralineFailure; public PatchScript(final Change.Key ck, final ChangeType ct, final String on, final String nn, final FileMode om, final FileMode nm, @@ -62,7 +63,7 @@ public class PatchScript { final SparseFileContent ca, final SparseFileContent cb, final List<Edit> e, final DisplayMethod ma, final DisplayMethod mb, final CommentDetail cd, final List<Patch> hist, final boolean hf, - final boolean id) { + final boolean id, final boolean idf) { changeId = ck; changeType = ct; oldName = on; @@ -80,6 +81,7 @@ public class PatchScript { history = hist; hugeFile = hf; intralineDifference = id; + intralineFailure = idf; } protected PatchScript() { @@ -149,6 +151,10 @@ public class PatchScript { return intralineDifference; } + public boolean hasIntralineFailure() { + return intralineFailure; + } + public SparseFileContent getA() { return a; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java index 93e8deb44a..dbbc9c3fa9 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java @@ -31,6 +31,7 @@ public interface PatchConstants extends Constants { String patchHistoryTitle(); String disabledOnLargeFiles(); + String intralineFailure(); String upToChange(); String linePrev(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties index 90def1d307..590007dc06 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties @@ -13,6 +13,7 @@ patchHeaderOld = Old Version patchHeaderNew = New Version patchHistoryTitle = Patch History disabledOnLargeFiles = Disabled on very large source files. +intralineFailure = Intraline difference not available due to server error. upToChange = Up to change linePrev = Previous line diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java index 812a74ee72..cbe037b7ee 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java @@ -15,6 +15,7 @@ package com.google.gerrit.client.patches; import com.google.gerrit.client.Dispatcher; +import com.google.gerrit.client.ErrorDialog; import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.RpcStatus; import com.google.gerrit.client.changes.CommitMessageBlock; @@ -140,6 +141,7 @@ public abstract class PatchScreen extends Screen implements /** Keys that cause an action on this screen */ private KeyCommandSet keysNavigation; private HandlerRegistration regNavigation; + private boolean intralineFailure; /** * How this patch should be displayed in the patch screen. @@ -461,6 +463,17 @@ public abstract class PatchScreen extends Screen implements settingsPanel.getReviewedCheckBox().setValue(true); setReviewedByCurrentUser(true /* reviewed */); } + + intralineFailure = isFirst && script.hasIntralineFailure(); + } + + @Override + public void onShowView() { + super.onShowView(); + if (intralineFailure) { + intralineFailure = false; + new ErrorDialog(PatchUtil.C.intralineFailure()).show(); + } } private void showPatch(final boolean showPatch) { diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java index 2604d92609..b7b3dd2fa6 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptBuilder.java @@ -20,12 +20,14 @@ import com.google.gerrit.common.data.PatchScript.DisplayMethod; import com.google.gerrit.prettify.common.EditList; import com.google.gerrit.prettify.common.SparseFileContent; import com.google.gerrit.reviewdb.AccountDiffPreference; +import com.google.gerrit.reviewdb.AccountDiffPreference.Whitespace; import com.google.gerrit.reviewdb.Change; import com.google.gerrit.reviewdb.Patch; import com.google.gerrit.reviewdb.PatchLineComment; -import com.google.gerrit.reviewdb.AccountDiffPreference.Whitespace; +import com.google.gerrit.reviewdb.Project; import com.google.gerrit.server.FileTypeRegistry; import com.google.gerrit.server.patch.IntraLineDiff; +import com.google.gerrit.server.patch.IntraLineDiffKey; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.Text; @@ -65,6 +67,7 @@ class PatchScriptBuilder { }; private Repository db; + private Project.NameKey projectKey; private ObjectReader reader; private Change change; private AccountDiffPreference diffPrefs; @@ -88,8 +91,9 @@ class PatchScriptBuilder { patchListCache = plc; } - void setRepository(final Repository r) { - db = r; + void setRepository(Repository r, Project.NameKey projectKey) { + this.db = r; + this.projectKey = projectKey; } void setChange(final Change c) { @@ -128,6 +132,7 @@ class PatchScriptBuilder { final CommentDetail comments, final List<Patch> history) throws IOException { boolean intralineDifference = diffPrefs.isIntralineDifference(); + boolean intralineFailure = false; a.path = oldName(content); b.path = newName(content); @@ -137,17 +142,29 @@ class PatchScriptBuilder { edits = new ArrayList<Edit>(content.getEdits()); - if (intralineDifference) { - if (isModify(content)) { - IntraLineDiff d = - patchListCache.getIntraLineDiff(a.id, a.src, b.id, b.src, edits); - if (d != null) { - edits = new ArrayList<Edit>(d.getEdits()); - } else { - intralineDifference = false; + if (intralineDifference && isModify(content)) { + IntraLineDiff d = + patchListCache.getIntraLineDiff(new IntraLineDiffKey(a.id, a.src, + b.id, b.src, edits, projectKey, bId, b.path)); + if (d != null) { + switch (d.getStatus()) { + case EDIT_LIST: + edits = new ArrayList<Edit>(d.getEdits()); + break; + + case DISABLED: + intralineDifference = false; + break; + + case ERROR: + case TIMEOUT: + intralineDifference = false; + intralineFailure = true; + break; } } else { intralineDifference = false; + intralineFailure = true; } } @@ -188,10 +205,11 @@ class PatchScriptBuilder { packContent(diffPrefs.getIgnoreWhitespace() != Whitespace.IGNORE_NONE); } - return new PatchScript(change.getKey(), content.getChangeType(), content - .getOldName(), content.getNewName(), a.fileMode, b.fileMode, content - .getHeaderLines(), diffPrefs, a.dst, b.dst, edits, a.displayMethod, - b.displayMethod, comments, history, hugeFile, intralineDifference); + return new PatchScript(change.getKey(), content.getChangeType(), + content.getOldName(), content.getNewName(), a.fileMode, b.fileMode, + content.getHeaderLines(), diffPrefs, a.dst, b.dst, edits, + a.displayMethod, b.displayMethod, comments, history, hugeFile, + intralineDifference, intralineFailure); } private static boolean isModify(PatchListEntry content) { diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java index 377cf49c5b..fcaa0c50eb 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java @@ -173,7 +173,7 @@ class PatchScriptFactory extends Handler<PatchScript> { private PatchScriptBuilder newBuilder(final PatchList list, Repository git) { final AccountDiffPreference dp = new AccountDiffPreference(diffPrefs); final PatchScriptBuilder b = builderFactory.get(); - b.setRepository(git); + b.setRepository(git, projectKey); b.setChange(change); b.setDiffPrefs(dp); b.setTrees(list.isAgainstParent(), list.getOldId(), list.getNewId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiff.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiff.java index d69472462e..3805f8f6b3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiff.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiff.java @@ -14,9 +14,13 @@ package com.google.gerrit.server.patch; +import static com.google.gerrit.server.ioutil.BasicSerialization.readEnum; import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32; +import static com.google.gerrit.server.ioutil.BasicSerialization.writeEnum; import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32; +import com.google.gerrit.reviewdb.CodedEnum; + import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.diff.ReplaceEdit; @@ -33,17 +37,44 @@ import java.util.List; public class IntraLineDiff implements Serializable { static final long serialVersionUID = IntraLineDiffKey.serialVersionUID; + public static enum Status implements CodedEnum { + EDIT_LIST('e'), DISABLED('D'), TIMEOUT('T'), ERROR('E'); + + private final char code; + + Status(char code) { + this.code = code; + } + + @Override + public char getCode() { + return code; + } + } + + private transient Status status; private transient List<Edit> edits; + IntraLineDiff(Status status) { + this.status = status; + this.edits = Collections.emptyList(); + } + IntraLineDiff(List<Edit> edits) { + this.status = Status.EDIT_LIST; this.edits = Collections.unmodifiableList(edits); } + public Status getStatus() { + return status; + } + public List<Edit> getEdits() { return edits; } private void writeObject(final ObjectOutputStream out) throws IOException { + writeEnum(out, status); writeVarInt32(out, edits.size()); for (Edit e : edits) { writeEdit(out, e); @@ -61,6 +92,7 @@ public class IntraLineDiff implements Serializable { } private void readObject(final ObjectInputStream in) throws IOException { + status = readEnum(in, Status.values()); int editCount = readVarInt32(in); Edit[] editArray = new Edit[editCount]; for (int i = 0; i < editCount; i++) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java index 61004ddc24..a8d62fcd37 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java @@ -17,6 +17,8 @@ package com.google.gerrit.server.patch; import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull; import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull; +import com.google.gerrit.reviewdb.Project; + import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.lib.ObjectId; @@ -27,7 +29,7 @@ import java.io.Serializable; import java.util.List; public class IntraLineDiffKey implements Serializable { - static final long serialVersionUID = 2L; + static final long serialVersionUID = 3L; private transient ObjectId aId; private transient ObjectId bId; @@ -38,14 +40,22 @@ public class IntraLineDiffKey implements Serializable { private transient Text bText; private transient List<Edit> edits; - IntraLineDiffKey(ObjectId aId, Text aText, ObjectId bId, Text bText, - List<Edit> edits) { + private transient Project.NameKey projectKey; + private transient ObjectId commit; + private transient String path; + + public IntraLineDiffKey(ObjectId aId, Text aText, ObjectId bId, Text bText, + List<Edit> edits, Project.NameKey projectKey, ObjectId commit, String path) { this.aId = aId; this.bId = bId; this.aText = aText; this.bText = bText; this.edits = edits; + + this.projectKey = projectKey; + this.commit = commit; + this.path = path; } Text getTextA() { @@ -60,6 +70,26 @@ public class IntraLineDiffKey implements Serializable { return edits; } + ObjectId getBlobA() { + return aId; + } + + ObjectId getBlobB() { + return bId; + } + + Project.NameKey getProject() { + return projectKey; + } + + ObjectId getCommit() { + return commit; + } + + String getPath() { + return path; + } + @Override public int hashCode() { int h = 0; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java index 0d3afdeed4..0ac1af2a18 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java @@ -16,24 +16,200 @@ package com.google.gerrit.server.patch; import com.google.gerrit.server.cache.EntryCreator; +import com.google.gerrit.server.config.ConfigUtil; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.Inject; import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.diff.MyersDiff; import org.eclipse.jgit.diff.ReplaceEdit; +import org.eclipse.jgit.lib.Config; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Pattern; class IntraLineLoader extends EntryCreator<IntraLineDiffKey, IntraLineDiff> { + private static final Logger log = LoggerFactory + .getLogger(IntraLineLoader.class); + private static final Pattern BLANK_LINE_RE = Pattern .compile("^[ \\t]*(|[{}]|/\\*\\*?|\\*)[ \\t]*$"); private static final Pattern CONTROL_BLOCK_START_RE = Pattern .compile("[{:][ \\t]*$"); + private final BlockingQueue<Worker> workerPool; + private final long timeoutMillis; + + @Inject + IntraLineLoader(final @GerritServerConfig Config cfg) { + final int workers = + cfg.getInt("cache", PatchListCacheImpl.INTRA_NAME, "maxIdleWorkers", + Runtime.getRuntime().availableProcessors() * 3 / 2); + workerPool = new ArrayBlockingQueue<Worker>(workers, true /* fair */); + + timeoutMillis = + ConfigUtil.getTimeUnit(cfg, "cache", PatchListCacheImpl.INTRA_NAME, + "timeout", TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS), + TimeUnit.MILLISECONDS); + } + @Override public IntraLineDiff createEntry(IntraLineDiffKey key) throws Exception { + Worker w = workerPool.poll(); + if (w == null) { + w = new Worker(); + } + + Worker.Result r = w.computeWithTimeout(key, timeoutMillis); + + if (r == Worker.Result.TIMEOUT) { + // Don't keep this thread. We have to murder it unsafely, which + // means its unable to be reused in the future. Return back a + // null result, indicating the cache cannot load this key. + // + return new IntraLineDiff(IntraLineDiff.Status.TIMEOUT); + } + + if (!workerPool.offer(w)) { + // If the idle worker pool is full, terminate this thread. + // + w.end(); + } + + if (r.error != null) { + // If there was an error computing the result, carry it + // up to the caller so the cache knows this key is invalid. + // + throw r.error; + } + + return r.diff; + } + + private static class Worker { + private static final AtomicInteger count = new AtomicInteger(1); + + private final ArrayBlockingQueue<Input> input; + private final ArrayBlockingQueue<Result> result; + private final Thread thread; + + Worker() { + input = new ArrayBlockingQueue<Input>(1); + result = new ArrayBlockingQueue<Result>(1); + + thread = new Thread(new Runnable() { + public void run() { + workerLoop(); + } + }); + thread.setName("IntraLineDiff-" + count.getAndIncrement()); + thread.setDaemon(true); + thread.start(); + } + + Result computeWithTimeout(IntraLineDiffKey key, long timeoutMillis) + throws Exception { + if (!input.offer(new Input(key))) { + log.error("Cannot enqueue task to thread " + thread.getName()); + return null; + } + + Result r = result.poll(timeoutMillis, TimeUnit.MILLISECONDS); + if (r != null) { + return r; + } else { + log.warn(timeoutMillis + " ms timeout reached for IntraLineDiff" + + " in project " + key.getProject().get() // + + " on commit " + key.getCommit().name() // + + " for path " + key.getPath() // + + " comparing " + key.getBlobA().name() // + + ".." + key.getBlobB().name() // + + ". Killing " + thread.getName()); + try { + thread.stop(); + } catch (Throwable error) { + // Ignore any reason the thread won't stop. + log.error("Cannot stop runaway thread " + thread.getName(), error); + } + return Result.TIMEOUT; + } + } + + void end() { + if (!input.offer(Input.END_THREAD)) { + log.error("Cannot gracefully stop thread " + thread.getName()); + } + } + + private void workerLoop() { + try { + for (;;) { + Input in; + try { + in = input.take(); + } catch (InterruptedException e) { + log.error("Unexpected interrupt on " + thread.getName()); + continue; + } + + if (in == Input.END_THREAD) { + return; + } + + Result r; + try { + r = new Result(IntraLineLoader.compute(in.key)); + } catch (Exception error) { + r = new Result(error); + } + + if (!result.offer(r)) { + log.error("Cannot return result from " + thread.getName()); + } + } + } catch (ThreadDeath iHaveBeenShot) { + // Handle thread death by gracefully returning to the caller, + // allowing the thread to be destroyed. + } + } + + private static class Input { + static final Input END_THREAD = new Input(null); + + final IntraLineDiffKey key; + + Input(IntraLineDiffKey key) { + this.key = key; + } + } + + static class Result { + static final Result TIMEOUT = new Result((IntraLineDiff) null); + + final IntraLineDiff diff; + final Exception error; + + Result(IntraLineDiff diff) { + this.diff = diff; + this.error = null; + } + + Result(Exception error) { + this.diff = null; + this.error = error; + } + } + } + + private static IntraLineDiff compute(IntraLineDiffKey key) throws Exception { List<Edit> edits = new ArrayList<Edit>(key.getEdits()); Text aContent = key.getTextA(); Text bContent = key.getTextB(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java index c7bb08b4c8..a7cf10aa27 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java @@ -17,17 +17,11 @@ package com.google.gerrit.server.patch; import com.google.gerrit.reviewdb.Change; import com.google.gerrit.reviewdb.PatchSet; -import org.eclipse.jgit.diff.Edit; -import org.eclipse.jgit.lib.ObjectId; - -import java.util.List; - /** Provides a cached list of {@link PatchListEntry}. */ public interface PatchListCache { public PatchList get(PatchListKey key); public PatchList get(Change change, PatchSet patchSet); - public IntraLineDiff getIntraLineDiff(ObjectId aId, Text aText, ObjectId bId, - Text bText, List<Edit> edits); + public IntraLineDiff getIntraLineDiff(IntraLineDiffKey key); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index 285420c9e6..e1a0a40ed0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -30,17 +30,14 @@ import com.google.inject.Singleton; import com.google.inject.TypeLiteral; import com.google.inject.name.Named; -import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; -import java.util.List; - /** Provides a cached list of {@link PatchListEntry}. */ @Singleton public class PatchListCacheImpl implements PatchListCache { private static final String FILE_NAME = "diff"; - private static final String INTRA_NAME = "diff_intraline"; + static final String INTRA_NAME = "diff_intraline"; public static Module module() { return new CacheModule() { @@ -98,14 +95,15 @@ public class PatchListCacheImpl implements PatchListCache { } @Override - public IntraLineDiff getIntraLineDiff(ObjectId aId, Text aText, ObjectId bId, - Text bText, List<Edit> edits) { + public IntraLineDiff getIntraLineDiff(IntraLineDiffKey key) { if (computeIntraline) { - IntraLineDiffKey key = - new IntraLineDiffKey(aId, aText, bId, bText, edits); - return intraCache.get(key); + IntraLineDiff d = intraCache.get(key); + if (d == null) { + d = new IntraLineDiff(IntraLineDiff.Status.ERROR); + } + return d; } else { - return null; + return new IntraLineDiff(IntraLineDiff.Status.DISABLED); } } } |