Fix VirtualThreadPool benchmark methodology issue (#16174)#16370
Open
anushkagupta200615-jpg wants to merge 1 commit into
Open
Fix VirtualThreadPool benchmark methodology issue (#16174)#16370anushkagupta200615-jpg wants to merge 1 commit into
anushkagupta200615-jpg wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16370 +/- ##
============================================
- Coverage 60.86% 60.84% -0.03%
+ Complexity 11768 11762 -6
============================================
Files 1953 1953
Lines 89260 89260
Branches 13471 13471
============================================
- Hits 54329 54307 -22
- Misses 29350 29360 +10
- Partials 5581 5593 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
@funky-eyes PTAL |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the pooled VirtualThreadPool configuration to keep excess virtual threads alive long enough for ThreadLocal reuse, and adds regression tests to prevent the benchmark methodology and keep-alive settings from regressing.
Changes:
- Update pooled-mode
ThreadPoolExecutorkeep-alive from0msto60svia a shared constant andTimeUnit.SECONDS. - Strengthen
VirtualThreadPoolTestwith keep-alive assertions and a pooled-modeThreadLocalreuse regression test. - Add
VirtualThreadPoolBenchmarkTestto enforce the correct two-latch benchmark synchronization pattern (await completion latch, not start gate).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| dubbo-plugin/dubbo-plugin-loom/src/main/java/org/apache/dubbo/common/threadpool/support/loom/VirtualThreadPool.java | Fix pooled-mode keep-alive configuration and document pooled vs unpooled behavior. |
| dubbo-plugin/dubbo-plugin-loom/src/test/java/org/apache/dubbo/common/threadpool/support/loom/VirtualThreadPoolTest.java | Add keep-alive regression guard and validate pooled-mode ThreadLocal reuse behavior. |
| dubbo-plugin/dubbo-plugin-loom/src/test/java/org/apache/dubbo/common/threadpool/support/loom/VirtualThreadPoolBenchmarkTest.java | Add correctness tests to prevent future benchmark latch/timing regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+129
to
+131
| ThreadPool threadPool = new VirtualThreadPool(); | ||
| Executor executor = threadPool.getExecutor(url); | ||
|
|
| // Correct: await completionLatch (latch 2), NOT startGate (latch 1). | ||
| // Awaiting startGate here would return immediately (it is already at 0) and make | ||
| // the measurement appear artificially fast - exactly the flaw in #16174. | ||
| completionLatch.await(); |
Comment on lines
+88
to
+93
| URL url = URL.valueOf("dubbo://10.20.130.230:20880/context/path"); | ||
| ThreadPool threadPool = new VirtualThreadPool(); | ||
| Executor executor = threadPool.getExecutor(url); | ||
|
|
||
| runBenchmark(executor, TASK_COUNT, "unpooled"); | ||
| } |
Comment on lines
+106
to
+112
| URL url = URL.valueOf("dubbo://10.20.130.230:20880/context/path?" + THREADS_VIRTUAL_CORE + "=" | ||
| + Runtime.getRuntime().availableProcessors()); | ||
| ThreadPool threadPool = new VirtualThreadPool(); | ||
| Executor executor = threadPool.getExecutor(url); | ||
|
|
||
| runBenchmark(executor, TASK_COUNT, "pooled"); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change?
Fixes #16174
This PR addresses the benchmark methodology issue and the root cause of the
VirtualThreadPoolconfiguration bug introduced in #16055.Problem:
keepAliveTime: The pooled virtual thread implementation usedkeepAliveTime = 0L MILLISECONDS. Because of this, non-core threads died immediately after completing a task. This defeated the entire purpose of the pooled mode (as outlined in [Feature] Refactor VirtualThreadPool to use a pooled virtual thread model #16042), which is to keep threads warm so that libraries like FastJSON and Aerospike can reuse their heavyThreadLocalbyte buffers across requests.startGate(which was already zero) instead of thecompletionLatch. As a result, the timing window closed before the tasks were actually completed, masking thekeepAliveoverhead and falsely inflating the performance metrics of the pooled executor.Solution:
keepAliveTime: UpdatedVirtualThreadPoolto use60L SECONDSforkeepAliveTimein the pooled branch. This ensures warm virtual threads survive long enough to reuse theirThreadLocalcaches between subsequent requests.VirtualThreadPoolTestto verify thatkeepAliveTime > 0and added a testgetExecutor4_threadLocalReuseInPooledModeproving thatThreadLocalstate is actually preserved and reused across sequential tasks on the pooled executor.VirtualThreadPoolBenchmarkTest.javathat uses the correct two-latch pattern (awaiting the completion latch) to strictly verify that all submitted tasks run to completion within the benchmark window for both pooled and unpooled modes.Checklist