Bug 500332 - Store JobInfo in the JobMonitor
I prevents accessing a concurrent hash map when it's not needed
(monitor's methods are only invoked from the job's thread).
Change-Id: Icaeb468daf4ac55cff861413fd1718d7ed257264
Signed-off-by: Mikael Barbero <mikael@eclipse.org>
diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java
index c0ff8a1..78dd550 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressManager.java
@@ -96,7 +96,7 @@
private static ProgressManager singleton;
- final private ConcurrentMap<Job, JobInfo> jobs = new ConcurrentHashMap<>();
+ final private Set<Job> managedJobs = ConcurrentHashMap.newKeySet();
final private Map<Object, Collection<IJobBusyListener>> familyListeners = Collections
.synchronizedMap(new LinkedHashMap<>());
@@ -207,6 +207,7 @@
*/
public class JobMonitor implements IProgressMonitorWithBlocking {
Job job;
+ JobInfo info;
String currentTaskName;
Set<IProgressMonitorWithBlocking> monitors = Collections.emptySet();
@@ -215,8 +216,13 @@
*
* @param newJob
*/
- JobMonitor(Job newJob) {
+ JobMonitor(Job newJob, JobInfo jobInfo) {
job = newJob;
+ info = jobInfo;
+ }
+
+ public JobInfo getJobInfo() {
+ return info;
}
/**
@@ -229,7 +235,6 @@
Set<IProgressMonitorWithBlocking> newSet = new LinkedHashSet<>(monitors);
newSet.add(monitor);
this.monitors = Collections.unmodifiableSet(newSet);
- JobInfo info = getJobInfo(job);
TaskInfo currentTask = info.getTaskInfo();
if (currentTask != null) {
monitor.beginTask(currentTaskName, currentTask.totalWork);
@@ -245,7 +250,6 @@
@Override
public void beginTask(String taskName, int totalWork) {
- JobInfo info = getJobInfo(job);
info.beginTask(taskName, totalWork);
refreshJobInfo(info);
currentTaskName = taskName;
@@ -254,16 +258,13 @@
@Override
public void done() {
- JobInfo info = getJobInfo(job);
info.clearTaskInfo();
info.clearChildren();
- runnableMonitors.remove(job);
monitors.stream().forEach(IProgressMonitorWithBlocking::done);
}
@Override
public void internalWorked(double work) {
- JobInfo info = getJobInfo(job);
if (info.hasTaskInfo()) {
info.addWork(work);
refreshJobInfo(info);
@@ -273,17 +274,11 @@
@Override
public boolean isCanceled() {
- // Use the internal get so we don't create a Job Info for
- // a job that is not running (see bug 149857).
- JobInfo info = internalGetJobInfo(job);
- if (info == null)
- return false;
return info.isCanceled();
}
@Override
public void setCanceled(boolean value) {
- JobInfo info = getJobInfo(job);
// Don't bother canceling twice.
if (value && !info.isCanceled()) {
info.cancel();
@@ -294,7 +289,6 @@
@Override
public void setTaskName(String taskName) {
- JobInfo info = getJobInfo(job);
if (info.hasTaskInfo()) {
info.setTaskName(taskName);
} else {
@@ -312,7 +306,6 @@
if (name == null) {
return;
}
- JobInfo info = getJobInfo(job);
info.clearChildren();
info.addSubTask(name);
refreshJobInfo(info);
@@ -326,7 +319,6 @@
@Override
public void clearBlocked() {
- JobInfo info = getJobInfo(job);
info.setBlockedStatus(null);
refreshJobInfo(info);
monitors.stream().forEach(IProgressMonitorWithBlocking::clearBlocked);
@@ -334,7 +326,6 @@
@Override
public void setBlocked(IStatus reason) {
- JobInfo info = getJobInfo(job);
info.setBlockedStatus(reason);
refreshJobInfo(info);
monitors.stream().forEach(listener -> listener.setBlocked(reason));
@@ -357,44 +348,44 @@
Job.getJobManager().addJobChangeListener(this.changeListener);
StatusManager.getManager().addListener(notificationListener);
- uiRefreshThrottler = new Throttler(Display.getDefault(), Duration.ofMillis(100), () -> {
- Set<GroupInfo> localPendingGroupUpdates, localPendingGroupRemoval;
- Map<JobInfo, Set<IJobProgressManagerListener>> localPendingJobUpdates, localPendingJobAddition,
- localPendingJobRemoval;
- synchronized (pendingUpdatesMutex) {
- localPendingJobUpdates = pendingJobUpdates;
- pendingJobUpdates = new LinkedHashMap<>();
- localPendingGroupUpdates = pendingGroupUpdates;
- pendingGroupUpdates = new LinkedHashSet<>();
- localPendingJobRemoval = pendingJobRemoval;
- pendingJobRemoval = new LinkedHashMap<>();
- localPendingGroupRemoval = pendingGroupRemoval;
- pendingGroupRemoval = new LinkedHashSet<>();
- localPendingJobAddition = pendingJobAddition;
- pendingJobAddition = new LinkedHashMap<>();
- }
+ uiRefreshThrottler = new Throttler(Display.getDefault(), Duration.ofMillis(100), this::notifyListeners);
+ }
- localPendingJobAddition.entrySet()
- .forEach(e -> e.getValue().forEach(listener -> listener.addJob(e.getKey())));
+ /* Visible for testing */ public void notifyListeners() {
+ Set<GroupInfo> localPendingGroupUpdates, localPendingGroupRemoval;
+ Map<JobInfo, Set<IJobProgressManagerListener>> localPendingJobUpdates, localPendingJobAddition,
+ localPendingJobRemoval;
+ synchronized (pendingUpdatesMutex) {
+ localPendingJobUpdates = pendingJobUpdates;
+ pendingJobUpdates = new LinkedHashMap<>();
+ localPendingGroupUpdates = pendingGroupUpdates;
+ pendingGroupUpdates = new LinkedHashSet<>();
+ localPendingJobRemoval = pendingJobRemoval;
+ pendingJobRemoval = new LinkedHashMap<>();
+ localPendingGroupRemoval = pendingGroupRemoval;
+ pendingGroupRemoval = new LinkedHashSet<>();
+ localPendingJobAddition = pendingJobAddition;
+ pendingJobAddition = new LinkedHashMap<>();
+ }
- // Adds all non null JobInfo#getGroupInfo to the list of groups to
- // be refreshed
- localPendingJobUpdates.entrySet().stream().map(e -> e.getKey().getGroupInfo()).filter(Objects::nonNull)
- .forEach(localPendingGroupUpdates::add);
+ localPendingJobAddition.entrySet().forEach(e -> e.getValue().forEach(listener -> listener.addJob(e.getKey())));
- localPendingJobUpdates.entrySet()
- .forEach(e -> e.getValue().forEach(listener -> listener.refreshJobInfo(e.getKey())));
+ // Adds all non null JobInfo#getGroupInfo to the list of groups to
+ // be refreshed
+ localPendingJobUpdates.entrySet().stream().map(e -> e.getKey().getGroupInfo()).filter(Objects::nonNull)
+ .forEach(localPendingGroupUpdates::add);
- // refresh groups
- localPendingGroupUpdates
- .forEach(groupInfo -> listeners.forEach(listener -> listener.refreshGroup(groupInfo)));
+ localPendingJobUpdates.entrySet()
+ .forEach(e -> e.getValue().forEach(listener -> listener.refreshJobInfo(e.getKey())));
- localPendingJobRemoval.entrySet()
- .forEach(e -> e.getValue().forEach(listener -> listener.removeJob(e.getKey())));
+ // refresh groups
+ localPendingGroupUpdates.forEach(groupInfo -> listeners.forEach(listener -> listener.refreshGroup(groupInfo)));
- localPendingGroupRemoval.forEach(group -> {
- listeners.forEach(listener -> listener.removeGroup(group));
- });
+ localPendingJobRemoval.entrySet()
+ .forEach(e -> e.getValue().forEach(listener -> listener.removeJob(e.getKey())));
+
+ localPendingGroupRemoval.forEach(group -> {
+ listeners.forEach(listener -> listener.removeGroup(group));
});
}
@@ -432,7 +423,7 @@
return new JobChangeAdapter() {
@Override
public void aboutToRun(IJobChangeEvent event) {
- JobInfo info = getJobInfo(event.getJob());
+ JobInfo info = progressFor(event.getJob()).getJobInfo();
refreshJobInfo(info);
Iterator<IJobBusyListener> startListeners = busyListenersForJob(event.getJob()).iterator();
while (startListeners.hasNext()) {
@@ -452,7 +443,7 @@
next.decrementBusy(event.getJob());
}
- final JobInfo info = getJobInfo(event.getJob());
+ final JobInfo info = progressFor(event.getJob()).getJobInfo();
removeJobInfo(info);
/*
@@ -510,10 +501,10 @@
* @param event
*/
private void updateFor(IJobChangeEvent event) {
- if (jobs.containsKey(event.getJob())) {
- refreshJobInfo(getJobInfo(event.getJob()));
+ if (managedJobs.contains(event.getJob())) {
+ refreshJobInfo(progressFor(event.getJob()).getJobInfo());
} else {
- addJobInfo(new JobInfo(event.getJob()));
+ addJobInfo(progressFor(event.getJob()).getJobInfo());
}
}
@@ -524,9 +515,8 @@
@Override
public void sleeping(IJobChangeEvent event) {
-
- if (jobs.containsKey(event.getJob()))// Are we showing this?
- sleepJobInfo(getJobInfo(event.getJob()));
+ if (managedJobs.contains(event.getJob()))// Are we showing this?
+ sleepJobInfo(progressFor(event.getJob()).getJobInfo());
}
};
}
@@ -614,7 +604,9 @@
* @return IProgressMonitor
*/
public JobMonitor progressFor(Job job) {
- return runnableMonitors.computeIfAbsent(job, JobMonitor::new);
+ return runnableMonitors.computeIfAbsent(job, (j) -> {
+ return new JobMonitor(j, new JobInfo(j));
+ });
}
/**
@@ -637,27 +629,6 @@
}
/**
- * Returns the JobInfo for the job. If it does not exist create it.
- *
- * @param job
- * @return JobInfo
- */
- JobInfo getJobInfo(Job job) {
- return jobs.computeIfAbsent(job, JobInfo::new);
- }
-
- /**
- * Returns an existing job info for the given Job or <code>null</code> if
- * there isn't one.
- *
- * @param job
- * @return JobInfo
- */
- JobInfo internalGetJobInfo(Job job) {
- return jobs.get(job);
- }
-
- /**
* Refreshes the IJobProgressManagerListeners as a result of a change in
* info.
*
@@ -691,7 +662,7 @@
*/
public void removeJobInfo(JobInfo info) {
Job job = info.getJob();
- jobs.remove(job);
+ managedJobs.remove(job);
synchronized (pendingUpdatesMutex) {
rememberListenersForJob(info, pendingJobRemoval);
}
@@ -723,7 +694,7 @@
refreshGroup(group);
}
- jobs.put(info.getJob(), info);
+ managedJobs.add(info.getJob());
synchronized (pendingUpdatesMutex) {
rememberListenersForJob(info, pendingJobAddition);
}
@@ -772,7 +743,8 @@
* @return JobInfo[]
*/
public JobInfo[] getJobInfos(boolean debug) {
- return jobs.entrySet().stream().filter(entry -> !isCurrentDisplaying(entry.getKey(), debug))
+ return managedJobs.stream().filter(job -> !isCurrentDisplaying(job, debug))
+ .map(job -> progressFor(job).getJobInfo())
.toArray(JobInfo[]::new);
}
@@ -783,8 +755,8 @@
* @return JobTreeElement[]
*/
public JobTreeElement[] getRootElements(boolean debug) {
- return jobs.entrySet().stream().filter(entry -> !isCurrentDisplaying(entry.getKey(), debug)).map(entry -> {
- JobInfo jobInfo = entry.getValue();
+ return managedJobs.stream().filter(job -> !isCurrentDisplaying(job, debug)).map(job -> {
+ JobInfo jobInfo = progressFor(job).getJobInfo();
GroupInfo group = jobInfo.getGroupInfo();
if (group == null) {
return jobInfo;
@@ -799,7 +771,7 @@
* @return boolean
*/
public boolean hasJobInfos() {
- return !jobs.isEmpty();
+ return !managedJobs.isEmpty();
}
/**
@@ -924,7 +896,7 @@
JobMonitor monitor = progressFor(job);
if (group instanceof GroupInfo) {
GroupInfo groupInfo = (GroupInfo) group;
- JobInfo jobInfo = getJobInfo(job);
+ JobInfo jobInfo = monitor.getJobInfo();
jobInfo.setGroupInfo(groupInfo);
jobInfo.setTicks(ticks);
groupInfo.addJobInfo(jobInfo);
@@ -1105,7 +1077,7 @@
*/
boolean checkForStaleness(Job job) {
if (job.getState() == Job.NONE) {
- removeJobInfo(getJobInfo(job));
+ removeJobInfo(progressFor(job).getJobInfo());
return true;
}
return false;
diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressMonitorFocusJobDialog.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressMonitorFocusJobDialog.java
index f6fbedb..af251c1 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressMonitorFocusJobDialog.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/progress/ProgressMonitorFocusJobDialog.java
@@ -255,7 +255,7 @@
Control area = super.createDialogArea(parent);
// Give the job info as the initial details
getProgressMonitor().setTaskName(
- ProgressManager.getInstance().getJobInfo(this.job)
+ ProgressManager.getInstance().progressFor(this.job).getJobInfo()
.getDisplayString());
return area;
}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/DummyJob.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/DummyJob.java
index 8e0a042..d5253c0 100644
--- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/DummyJob.java
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/DummyJob.java
@@ -30,7 +30,7 @@
public boolean inProgress = false;
/** if false, infinite until changed or job is cancelled */
- public boolean shouldFinish = true;
+ public volatile boolean shouldFinish = true;
public DummyJob(String name, IStatus status) {
super(name);
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressAnimationItemTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressAnimationItemTest.java
index e17068c..385431c 100644
--- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressAnimationItemTest.java
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/progress/ProgressAnimationItemTest.java
@@ -61,8 +61,7 @@
public void testSingleJobRefreshOnce() throws Exception {
createAndScheduleJob();
- waitAndReadAndDispatch();
-
+ ProgressManager.getInstance().notifyListeners();
refresh();
assertSingleAccessibleListener();
@@ -73,8 +72,7 @@
createAndScheduleJob();
createAndScheduleJob();
- waitAndReadAndDispatch();
-
+ ProgressManager.getInstance().notifyListeners();
refresh();
assertSingleAccessibleListener();
@@ -84,19 +82,13 @@
public void testSingleJobRefreshTwice() throws Exception {
createAndScheduleJob();
- waitAndReadAndDispatch();
-
+ ProgressManager.getInstance().notifyListeners();
refresh();
refresh();
assertSingleAccessibleListener();
}
- private void waitAndReadAndDispatch() throws InterruptedException {
- while (PlatformUI.getWorkbench().getDisplay().readAndDispatch()) {
- }
- }
-
private ProgressAnimationItem createProgressAnimationItem(Composite composite) {
ProgressRegion progressRegion = new ProgressRegion();
progressRegion.createContents(composite);
@@ -106,8 +98,6 @@
private static void createAndScheduleJob() throws InterruptedException {
DummyJob job = new DummyJob("Keep me", Status.OK_STATUS);
job.setProperty(IProgressConstants.KEEP_PROPERTY, true);
- ExtendedJobInfo info = new ExtendedJobInfo(job);
- ProgressManager.getInstance().addJobInfo(info);
job.schedule();
job.join();
}
