From cdbe0608d9756bdcf17fcb8eee245e911fe7ec96 Mon Sep 17 00:00:00 2001
From: Thomas Wolf
Date: Tue, 17 Jul 2018 12:30:24 +0200
Subject: Avoid that loading the history blocks updating the commit table

Somehow setting the input on the commit graph table got blocked on
the monitor of the SWTCommitList. The only other place where that
monitor can be held is in the GenerateHistoryJob itself.

However, CommitGraphTable.setInput() is always called not only with
that SWTCommitList but also its contents copied into an array. Thus
we can use this array copy instead of the list itself to get the
contents. On the array, no synchronization is needed.

That also means that inside the GenerateHistoryJob no synchronization
on that list is needed anymore, since only that job accesses it.

Bug: 485743
Change-Id: I77007b159d8b3de0a346b7cea69599fd61d80585
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>---
 .../egit/ui/internal/history/CommitGraphTable.java | 21 ++++++------
 .../ui/internal/history/GenerateHistoryJob.java    | 38 ++++++++++------------
 2 files changed, 27 insertions(+), 32 deletions(-)

(limited to 'org.eclipse.egit.ui/src/org')

diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitGraphTable.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitGraphTable.java
index 20e644cca0..4c24d641f0 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitGraphTable.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/CommitGraphTable.java
@@ -379,11 +379,12 @@ class CommitGraphTable {
 		}
 		highlight = hFlag;
 		allCommits = list;
-		int newAllCommitsLength = allCommits.size();
+		int newAllCommitsLength = asArray == null ? 0 : asArray.length;
 		table.setInput(asArray);
-		if (asArray != null && asArray.length > 0) {
-			if (oldList != list || allCommitsLength < newAllCommitsLength)
-				initCommitsMap();
+		if (newAllCommitsLength > 0) {
+			if (oldList != list || allCommitsLength < newAllCommitsLength) {
+				initCommitsMap(asArray);
+			}
 		} else {
 			table.getTable().deselectAll();
 			// Fire an event
@@ -402,14 +403,12 @@ class CommitGraphTable {
 		this.input = input;
 	}
 
-	private void initCommitsMap() {
+	private void initCommitsMap(SWTCommit[] asArray) {
 		commitsMap = new HashMap<>();
-		// ensure that filling (GenerateHistoryJob) and reading (here)
-		// the commit list is thread safe
-		synchronized (allCommits) {
-			for (PlotCommit commit : allCommits)
-				if (commit != null)
-					commitsMap.put(commit.getId().name(), commit);
+		for (SWTCommit commit : asArray) {
+			if (commit != null) {
+				commitsMap.put(commit.getId().name(), commit);
+			}
 		}
 	}
 
diff --git a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GenerateHistoryJob.java b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GenerateHistoryJob.java
index a45a30b46a..b67e00495e 100644
--- a/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GenerateHistoryJob.java
+++ b/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/history/GenerateHistoryJob.java
@@ -25,11 +25,11 @@ import org.eclipse.egit.ui.JobFamilies;
 import org.eclipse.egit.ui.UIPreferences;
 import org.eclipse.egit.ui.internal.UIText;
 import org.eclipse.egit.ui.internal.trace.GitTraceLocation;
-import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jface.resource.ResourceManager;
+import org.eclipse.jgit.annotations.NonNull;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.osgi.util.NLS;
 import org.eclipse.swt.widgets.Control;
 import org.eclipse.swt.widgets.Display;
@@ -92,28 +92,24 @@ class GenerateHistoryJob extends Job {
 						GitTraceLocation.getTrace().trace(
 								GitTraceLocation.HISTORYVIEW.getLocation(),
 								"Filling commit list"); //$NON-NLS-1$
-					// ensure that filling (here) and reading (CommitGraphTable)
-					// the commit list is thread safe
-					synchronized (loadedCommits) {
-						if (commitToLoad != null) {
-							loadedCommits.fillTo(commitToLoad, maxCommits);
-							commitToShow = commitToLoad;
-							commitToLoad = null;
-							boolean commitFound = false;
-							for (RevCommit commit : loadedCommits) {
-								if (commit.getId().equals(commitToShow.getId())) {
-									commitFound = true;
-									break;
-								}
-							}
-							commitNotFound = !commitFound;
-						} else {
-							loadedCommits.fillTo(oldsz + BATCH_SIZE - 1);
-							if (oldsz == loadedCommits.size()) {
-								forcedRedrawsAfterListIsCompleted++;
+					if (commitToLoad != null) {
+						loadedCommits.fillTo(commitToLoad, maxCommits);
+						commitToShow = commitToLoad;
+						commitToLoad = null;
+						boolean commitFound = false;
+						for (RevCommit commit : loadedCommits) {
+							if (commit.getId().equals(commitToShow.getId())) {
+								commitFound = true;
 								break;
 							}
 						}
+						commitNotFound = !commitFound;
+					} else {
+						loadedCommits.fillTo(oldsz + BATCH_SIZE - 1);
+						if (oldsz == loadedCommits.size()) {
+							forcedRedrawsAfterListIsCompleted++;
+							break;
+						}
 					}
 					if (monitor.isCanceled())
 						return Status.CANCEL_STATUS;
-- 
cgit v1.2.3