Enabling memory cleanup mode causes performance issue with JUnit 3.x tests executed by Vintage engine

[marcphilipp]

Thanks for the fixes so far. I found another gotcha. When added to a Vintage test this option makes the test suite run sequentially, even though it is configured to execute tests in parallel. For Guava's testlib (JUnit 3-based) this significantly reduces performance and causes the task to timeout (normally 4m, was actually 1h+).

googleTest.jfr.zip

./gradlew caffeine:googleTest --rerun -PjavaVersion=26 -Pjfr

git diff from 26d79b0

diff --git a/caffeine/build.gradle.kts b/caffeine/build.gradle.kts
index 431aef402..1d5a1aab8 100644
--- a/caffeine/build.gradle.kts
+++ b/caffeine/build.gradle.kts
@@ -343,6 +343,7 @@ testing.suites {
     targets.configureEach {
       testTask.configure {
         useParallelJUnitJupiter()
+        systemProperty("junit.platform.execution.memory.cleanup.enabled", "true")
       }
     }
   }

stacktrace

"Test worker" #3 [4867] prio=5 os_prio=31 cpu=291921.59ms elapsed=303.50s tid=0x0000000101033fe0 nid=4867 runnable  [0x000000016f99e000]
   java.lang.Thread.State: RUNNABLE
	at java.lang.Throwable.fillInStackTrace(java.base@26/Native Method)
	at java.lang.Throwable.fillInStackTrace(java.base@26/Throwable.java:825)
	- locked <0x00000007619e79d8> (a java.lang.NoSuchMethodException)
	at java.lang.Throwable.<init>(java.base@26/Throwable.java:283)
	at java.lang.Exception.<init>(java.base@26/Exception.java:67)
	at java.lang.ReflectiveOperationException.<init>(java.base@26/ReflectiveOperationException.java:57)
	at java.lang.NoSuchMethodException.<init>(java.base@26/NoSuchMethodException.java:50)
	at java.lang.Class.getMethod(java.base@26/Class.java:2166)
	at org.junit.internal.runners.JUnit38ClassRunner.getAnnotations(JUnit38ClassRunner.java:135)
	at org.junit.internal.runners.JUnit38ClassRunner.makeDescription(JUnit38ClassRunner.java:105)
	at org.junit.internal.runners.JUnit38ClassRunner.makeDescription(JUnit38ClassRunner.java:113)
	at org.junit.internal.runners.JUnit38ClassRunner.makeDescription(JUnit38ClassRunner.java:113)
	at org.junit.internal.runners.JUnit38ClassRunner.makeDescription(JUnit38ClassRunner.java:113)
	at org.junit.internal.runners.JUnit38ClassRunner.makeDescription(JUnit38ClassRunner.java:113)
	at org.junit.internal.runners.JUnit38ClassRunner.makeDescription(JUnit38ClassRunner.java:113)
	at org.junit.internal.runners.JUnit38ClassRunner.filter(JUnit38ClassRunner.java:159)
	at org.junit.vintage.engine.descriptor.RunnerTestDescriptor.tryToFilterRunner(RunnerTestDescriptor.java:96)
	at org.junit.vintage.engine.descriptor.RunnerTestDescriptor.tryToExcludeFromRunner(RunnerTestDescriptor.java:82)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.lambda$tryToExcludeFromRunner$0(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor$$Lambda/0x00001e000058d000.apply(Unknown Source)
	at java.util.Optional.map(java.base@26/Optional.java:264)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.tryToExcludeFromRunner(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.lambda$tryToExcludeFromRunner$0(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor$$Lambda/0x00001e000058d000.apply(Unknown Source)
	at java.util.Optional.map(java.base@26/Optional.java:264)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.tryToExcludeFromRunner(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.lambda$tryToExcludeFromRunner$0(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor$$Lambda/0x00001e000058d000.apply(Unknown Source)
	at java.util.Optional.map(java.base@26/Optional.java:264)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.tryToExcludeFromRunner(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.lambda$tryToExcludeFromRunner$0(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor$$Lambda/0x00001e000058d000.apply(Unknown Source)
	at java.util.Optional.map(java.base@26/Optional.java:264)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.tryToExcludeFromRunner(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.lambda$tryToExcludeFromRunner$0(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor$$Lambda/0x00001e000058d000.apply(Unknown Source)
	at java.util.Optional.map(java.base@26/Optional.java:264)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.tryToExcludeFromRunner(VintageTestDescriptor.java:106)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.canBeRemovedFromHierarchy(VintageTestDescriptor.java:100)
	at org.junit.vintage.engine.descriptor.VintageTestDescriptor.removeFromHierarchy(VintageTestDescriptor.java:94)
	at org.junit.platform.launcher.core.MemoryCleanupListener.cleanUp(MemoryCleanupListener.java:45)
	at org.junit.platform.launcher.core.MemoryCleanupListener.executionFinished(MemoryCleanupListener.java:39)

Originally posted by @ben-manes in #5344

[marcphilipp]

Fixing the root cause (creating an exception when looking up annotations) would require changes in JUnit 4. We could create a configuration parameter (junit.platform.execution.memory.cleanup.engines.exclude or similar) for excluding certain engines by id for which we would not call removeFromHierarchy.

[codingkiddo]

Thanks for the detailed report. I’d be interested in helping with this.

From the stack trace, the expensive path appears to be triggered by MemoryCleanupListener.cleanUp() calling removeFromHierarchy(), which for Vintage descriptors can attempt to filter the underlying JUnit 3 runner repeatedly.

Would the preferred direction be to avoid hierarchy removal for Vintage/JUnit 3 descriptors when memory cleanup is enabled, or should the fix stay inside the Vintage descriptor/filtering path?

[ben-manes]

I think excluding engine ids is a reasonable approach so that if the memory cleanup ever became enabled by default it did not cause regressions or surprises by disabling known problematic engines by default.

[codingkiddo]

Thanks, that makes sense to me.

A generic engine-id exclusion mechanism seems safer than adding a Vintage-specific special case. I can try a small PR that adds an experimental configuration parameter, for example junit.platform.execution.memory.cleanup.excluded.engines, and makes MemoryCleanupListener skip cleanup for descriptors whose UniqueId belongs to an excluded engine.

For this issue, the default could include junit-vintage, so enabling memory cleanup would not regress known problematic Vintage/JUnit 3 suites.

Would this be the preferred direction, or would you rather keep the default empty and let users explicitly configure excluded engines?