[performance] BatchImageBuilder: write .class files in batches #2948

jukzi · 2024-09-12T10:38:50Z

ProcessTaskManager

use java.util.concurrent for queue
signal Cancel/Exception/Stop via the queue
implements AutoCloseable for try-with-resource
drain as much Elements as possible from the queue

Improves the performance of "Clean all projects"

For example building platform workspace on Windows AbstractImageBuilder.compile(): 120 sec -> 91 sec

With this change the Compiler is actually waiting for parsing most time and not for the write to FileSystem anymore.

jukzi · 2024-09-12T10:39:31Z

needs eclipse-platform/eclipse.platform#1549

jukzi · 2024-09-12T10:48:46Z

Example sampling. Before:

With this PR:

jukzi · 2024-09-17T10:37:29Z

If anyone wants to review please let me know. Otherwise i plan to submit tomorrow.

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java

jukzi · 2024-09-17T11:17:06Z

Sampling from latest revision. writing the class files took only 11sec :-) :

double checked:

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java

iloveeclipse · 2024-09-17T12:29:55Z

@szarnekow : would be good if you could take a look here too.

iloveeclipse

I will be available next week for further review comments.

It would be nice if someone else also would review this PR if you plan to submit this before next week.

iloveeclipse · 2024-09-17T11:30:16Z

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java

+	private final CompilationGroup compilationGroup;
+
+	/*  leave 2 threads for compiler + reader.*/
+	private static final ExecutorService WRITER_EXECUTOR = Executors.newFixedThreadPool(Math.max(1, Runtime.getRuntime().availableProcessors() - 2), r -> {


Could we have more convenient name like WRITER_SERVICE?

We have also UI thread that shouldn't compete with compiler for CPU time. So leave 2 threads for compiler + reader should be leave 3 threads for compiler + reader + UI ?

WRITER_SERVICE ok

By now the UI does not lag. the CPU is <<100% since most time the writer thread is waiting anyway.

iloveeclipse · 2024-09-17T11:54:48Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

 	}
-}

 // add unit to the queue - wait if no space is available


wait if no space is available is obsoleted now

java.util.concurrent.BlockingQueue.put(E):
Inserts the specified element into this queue, waiting if necessary for space to become available.

So comment is not needed anyway, as it is just javadoc copy

Well, the caller should not need to dig into the implementation. I think it's good to keep as an explanation.

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

iloveeclipse · 2024-09-17T12:12:25Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

-			// ignore
+			this.units.put(newElement);
+		} catch (InterruptedException interrupt) {
+			throw new RuntimeException(interrupt);


Why is this wrapped? Usually if interrupted exception is handled, it should be flagged by Thread.interrupted() call, but neither this code nor caller does that.

Shouldn't this be AbortCompilation exception if we encounter problems during queue writing?

InterruptedException is a checked exception. that is not supported by executor.submit().
By now nothing should try to interrupt the executor, so it would be an error to receive one!
All other places in JDT just ignore InterruptedException. we could do that here too but that would need an endless loop until not interrupted. AbortCompilation is not supported here either as nothing reads the future. Only close() does - but that happens anyway only at the end.

iloveeclipse · 2024-09-17T12:18:25Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

+					return elements;
+				}
+			} catch (InterruptedException interrupt) {
+				throw new RuntimeException(interrupt);


Same here regarding InterruptedException

in this thread we can use throw new AbortCompilation(true/* silent */, new RuntimeException(interrupt));

iloveeclipse · 2024-09-17T12:24:40Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

-						throw (RuntimeException) this.caughtException;
-					}
-					return null;
+	/** returns null when no more elements can be expected **/


Can we return empty collection instead? It is always more convenient on the caller side to check for empty and not for null/not null

Currently the call blocks until some elements are returned or end reached. But the idea was that if the queue is empty the caller could meanwhile flush the result. I forgot why it is not needed that anymore. I may think about it, should not be a relevant difference.

I am happy you asked that. Since the current implementation returns as many CUs as available we know the next call would block until new units available=> the CPU has time to flush. I added flush after every call. This way it looks like the flush would take longer (more often only few files flushed), but the overall build time gets even faster, as the latency reduces:

iloveeclipse · 2024-09-17T12:26:00Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

+			if (next == STOP_SIGNAL) {
+				return null;
+			}
+			throw new IllegalStateException(String.valueOf(next));


Could we have a speaking exception message like "Received unexpected element to process: " + next

we should never see it, but ok.

iloveeclipse · 2024-09-17T12:35:48Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

+						try {
+							this.compiler.process(unitToProcess, index);
+						} catch (AbortCompilation abortCompilation) {
+							unitToProcess.cleanUp();


unitToProcess.cleanUp(); seem to be added here - why? It is done in finally below.

good question, i forgot why. i will remove it

iloveeclipse · 2024-09-17T12:43:22Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

+		try {
+			this.processingTask.get(250, TimeUnit.MILLISECONDS); // do not wait forever
+		} catch (InterruptedException | ExecutionException | TimeoutException ignored) {
+			// ignore


Not changed in this PR, but shouldn't Thread.interrupted() flag been cleared here?

If you want some consistent InterruptedException behaviour you would need to change it everywhere. by now just ignoring is the pattern used all over in jdt.

iloveeclipse · 2024-09-17T12:53:41Z

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

+	@Override
+	public void close() {
+		try {
+			this.processingTask.get(250, TimeUnit.MILLISECONDS); // do not wait forever


should this be this.processingTask.cancel(true)?

Maybe. That would interrupt the processing() thread, possibly interrupting addNextUnit() which is waiting for a overfilled queue-> RuntimeException -> Future ends. Can you elaborate why would we do so? I don't know the usecase when this should be relevant. should be similar to this.units.clear(). But normally that queue is emptied anyway at this stage. may only make a difference on exception handling.

I tested: both units.clear() and cancel(true) let addNextUnit() progress. However cancel would only work if not anything else in the progress catches the interrupt. So i added both.

iloveeclipse · 2024-09-17T18:21:02Z

In debugger I see 30 idle threads staying after compilation (I have 32 core workstation). I wonder if they could be shut down after some time?

jukzi · 2024-09-17T18:43:06Z

I wonder if they could be shut down after some time?

https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/concurrent/ThreadPoolExecutor.html#%3Cinit%3E(int,int,long,java.util.concurrent.TimeUnit,java.util.concurrent.BlockingQueue)
has a keepAliveTime. But you did not complain about the "Source File Reader" threads either so i wonder what is the benefit of shutting the threads down?

jukzi · 2024-09-17T18:43:51Z

@iloveeclipse take your time next week, this can wait

iloveeclipse · 2024-09-17T18:55:19Z

We often have to read thread dumps and having 30+ more threads there would make my life harder as it should be. Beside this, I don't like to waste resources (memory/system) if not needed. At the end, the number of (real) threads is limited per process.

jukzi · 2024-09-17T19:01:45Z

there was a request from @laeubi #1937 to reuse threads. What keepAliveTime would you suggest?

iloveeclipse · 2024-09-17T19:03:51Z

there was a request from @laeubi #1937 to reuse threads. What keepAliveTime would you suggest?

Let say 5 minutes.

jukzi · 2024-09-18T06:01:37Z

Let say 5 minutes.

done

jukzi · 2024-09-24T10:53:50Z

@iloveeclipse do plan to continue review?

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java

iloveeclipse

I wonder we don't have tests showing startingIndex + 1.

Also would be cool to have a test that validates that one not written class file from the batch will be reported as compilation error.

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ReadManager.java

ProcessTaskManager * use java.util.concurrent for queue * signal Cancel/Exception/Stop via the queue * implements AutoCloseable for try-with-resource * drain as much Elements as possible from the queue Improves the performance of "Clean all projects" For example building platform workspace on Windows AbstractImageBuilder.compile(): 120 sec -> 80 sec With this change the Compiler is actually waiting for parsing most time and not for the write to FileSystem anymore.

iloveeclipse

OK, thanks for the updated thread pool fix.

jukzi force-pushed the batch_write branch from c9812d0 to 6d5fbf2 Compare September 17, 2024 09:44

iloveeclipse reviewed Sep 17, 2024

View reviewed changes

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java Outdated Show resolved Hide resolved

iloveeclipse reviewed Sep 17, 2024

View reviewed changes

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java Outdated Show resolved Hide resolved

iloveeclipse reviewed Sep 17, 2024

View reviewed changes

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java Outdated Show resolved Hide resolved

jukzi force-pushed the batch_write branch from 6d5fbf2 to 1f51198 Compare September 17, 2024 11:28

iloveeclipse reviewed Sep 17, 2024

View reviewed changes

jukzi force-pushed the batch_write branch 4 times, most recently from c721d56 to af89d66 Compare September 17, 2024 18:00

jukzi force-pushed the batch_write branch from af89d66 to f9dea87 Compare September 18, 2024 06:01

jukzi requested a review from iloveeclipse September 24, 2024 10:53

iloveeclipse reviewed Sep 24, 2024

View reviewed changes

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java Outdated Show resolved Hide resolved

iloveeclipse reviewed Sep 24, 2024

View reviewed changes

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java Show resolved Hide resolved

iloveeclipse reviewed Sep 24, 2024

View reviewed changes

...clipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ProcessTaskManager.java Show resolved Hide resolved

iloveeclipse requested changes Sep 24, 2024

View reviewed changes

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/BatchImageBuilder.java Outdated Show resolved Hide resolved

jukzi force-pushed the batch_write branch from f9dea87 to 308ded5 Compare September 24, 2024 15:06

jukzi requested a review from iloveeclipse September 24, 2024 15:11

iloveeclipse requested changes Sep 24, 2024

View reviewed changes

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ReadManager.java Outdated Show resolved Hide resolved

jukzi force-pushed the batch_write branch from 308ded5 to b330810 Compare September 25, 2024 07:45

jukzi requested a review from iloveeclipse September 25, 2024 08:56

iloveeclipse approved these changes Sep 25, 2024

View reviewed changes

iloveeclipse merged commit 334703d into eclipse-jdt:master Sep 25, 2024
9 checks passed

[performance] BatchImageBuilder: write .class files in batches #2948

[performance] BatchImageBuilder: write .class files in batches #2948

Conversation

jukzi commented Sep 12, 2024

jukzi commented Sep 12, 2024

jukzi commented Sep 12, 2024

jukzi commented Sep 17, 2024

jukzi commented Sep 17, 2024

iloveeclipse commented Sep 17, 2024

iloveeclipse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iloveeclipse commented Sep 17, 2024

jukzi commented Sep 17, 2024

jukzi commented Sep 17, 2024

iloveeclipse commented Sep 17, 2024

jukzi commented Sep 17, 2024

iloveeclipse commented Sep 17, 2024

jukzi commented Sep 18, 2024

jukzi commented Sep 24, 2024

iloveeclipse left a comment

Choose a reason for hiding this comment

iloveeclipse left a comment

Choose a reason for hiding this comment