From 5f450dfc09369a83a36487d4b419cd8fb1b992df Mon Sep 17 00:00:00 2001
From: Heiko Klare <heiko.klare@vector.com>
Date: Thu, 28 Sep 2023 15:31:29 +0200
Subject: [PATCH] Remove recycling of ThreadJob in ImplicitJobs #724

The JobManager uses the ImplicitJobs implementation to spawn internal
jobs for the execution on a specific ISchedulingRule within an executed
job. This implementation creates instances of ThreadJob. These instances
are reused, such that the same ThreadJob instance is reused for
different rules and potentially different jobs. This is achieved by a
recycle functionality, which caches a ThreadJob for reuse once it is not
used anymore. This functionality results in the same job occurring with
different rules at different places and points in time. This is both
unexpected and error prone.

Since this reuse functionality only seems to be a performance
improvement whose value does nowadays not outweigh the added complexity
and risk of error, this change removes the recycling functionality.

Fixes https://github.com/eclipse-platform/eclipse.platform/issues/724.
---
 .../core/internal/jobs/ImplicitJobs.java      | 38 +------------------
 .../eclipse/core/internal/jobs/ThreadJob.java | 24 ------------
 2 files changed, 2 insertions(+), 60 deletions(-)

diff --git a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ImplicitJobs.java b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ImplicitJobs.java
index ff0bb307960..1c1350d9161 100644
--- a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ImplicitJobs.java
+++ b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ImplicitJobs.java
@@ -32,11 +32,6 @@
  */
 class ImplicitJobs {
 
-	/**
-	 * Cached unused instance that can be reused
-	 * @GuardedBy("this")
-	 */
-	private ThreadJob jobCache = null;
 	protected JobManager manager;
 
 	/**
@@ -77,9 +72,9 @@ void begin(ISchedulingRule rule, IProgressMonitor monitor, boolean suspend) {
 			//create a thread job for this thread, use the rule from the real job if it has one
 			Job realJob = manager.currentJob();
 			if (realJob != null && realJob.getRule() != null)
-				threadJob = newThreadJob(realJob.getRule());
+				threadJob = new ThreadJob(realJob.getRule());
 			else {
-				threadJob = newThreadJob(rule);
+				threadJob = new ThreadJob(rule);
 				threadJob.acquireRule = true;
 			}
 			//don't acquire rule if it is a suspended rule
@@ -172,7 +167,6 @@ private void endThreadJob(ThreadJob threadJob, boolean resume, boolean worker) {
 		//if the job was started, we need to notify job manager to end it
 		if (threadJob.isRunning())
 			manager.endJob(threadJob, Status.OK_STATUS, false, worker);
-		recycle(threadJob);
 	}
 
 	/**
@@ -188,25 +182,6 @@ private boolean isSuspended(ISchedulingRule rule) {
 		return false;
 	}
 
-	/**
-	 * Returns a new or reused ThreadJob instance.
-	 * @GuardedBy("this")
-	 */
-	private ThreadJob newThreadJob(ISchedulingRule rule) {
-		if (jobCache != null) {
-			ThreadJob job = jobCache;
-			// calling setRule will try to acquire JobManager.lock, breaking
-			// lock acquisition protocol. Since we managing this special job
-			// ourselves we can call internalSetRule
-			((InternalJob) job).internalSetRule(rule);
-			job.acquireRule = job.isRunning = false;
-			job.realJob = null;
-			jobCache = null;
-			return job;
-		}
-		return new ThreadJob(rule);
-	}
-
 	/**
 	 * A job has just finished that was holding a scheduling rule, and the
 	 * scheduling rule is now free.  Wake any blocked thread jobs so they can
@@ -218,15 +193,6 @@ void notifyWaitingThreadJobs(InternalJob job) {
 		}
 	}
 
-	/**
-	 * Indicates that a thread job is no longer in use and can be reused.
-	 * @GuardedBy("this")
-	 */
-	private void recycle(ThreadJob job) {
-		if (jobCache == null && job.recycle())
-			jobCache = job;
-	}
-
 	/**
 	 * Implements IJobManager#resume(ISchedulingRule)
 	 * @param rule
diff --git a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java
index 9f2740acd49..a7e50344b5b 100644
--- a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java
+++ b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/ThreadJob.java
@@ -417,30 +417,6 @@ void push(final ISchedulingRule rule) {
 		}
 	}
 
-	/**
-	 * Reset all of this job's fields so it can be reused.  Returns false if
-	 * reuse is not possible
-	 * 	@GuardedBy("JobManager.implicitJobs")
-	 */
-	boolean recycle() {
-		//don't recycle if still running for any reason
-		if (getState() != Job.NONE) {
-			return false;
-		}
-		//clear and reset all fields
-		acquireRule = isRunning = isBlocked = false;
-		realJob = null;
-		setRule(null);
-		setThread(null);
-		if (ruleStack.length != 2) {
-			ruleStack = new ISchedulingRule[2];
-		} else {
-			ruleStack[0] = ruleStack[1] = null;
-		}
-		top = -1;
-		return true;
-	}
-
 	@Override
 	public IStatus run(IProgressMonitor monitor) {
 		synchronized (this) {