Optimize CachedThreadPool usage. #357
Conversation
* Implement a maximum number of threads to exist in the pool. * Implement a threshold to stop creating platform threads and leverage virtual threads instead. * Implement a core size of threads, those of which never leave the pool to buffer inactivity.
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CachedThreadPoolDispatcher
participant ThreadPoolExecutor
participant Thread (Platform/Virtual)
Caller->>CachedThreadPoolDispatcher: Submit coroutine/task
CachedThreadPoolDispatcher->>ThreadPoolExecutor: Offer task
alt Active thread count > threshold
ThreadPoolExecutor->>Thread: Spawn virtual thread
else Active thread count ≤ threshold
ThreadPoolExecutor->>Thread: Spawn daemon platform thread
end
Thread->>Caller: Execute coroutine/task
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt (1)
30-48: Well-documented constants with reasonable defaults.The constants provide good control over thread pool behavior:
- Core pool ensures baseline availability
- Max limit prevents resource exhaustion
- Virtual thread threshold balances thread types effectively
Consider monitoring actual thread usage patterns in production to validate these values are optimal for your workload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt (3)
8-10: LGTM!The imports are appropriate for the custom ThreadPoolExecutor implementation.
76-82: LGTM!The formatting changes improve readability without altering functionality.
49-72: Verify thread safety and consider task queuing behavior.The implementation looks good overall, but please verify:
- The initialization pattern with
lateinit varinside arunblock is safe for concurrent access- Consider the implications of using
SynchronousQueue- tasks will be rejected if all threads are busy and max pool size is reachedThe dynamic thread type selection is a clever optimization. The daemon flag on platform threads is appropriate.
Run the following script to verify the thread naming API usage:
#!/bin/bash # Description: Verify the Thread.name() method signature and usage # Search for Thread.name method usage patterns in the codebase ast-grep --pattern 'Thread.$_().name($$$)' # Check if there are other usages of thread naming to ensure consistency rg -A 2 'Thread\.(of|name)' --type kotlin
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Show resolved
Hide resolved
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Show resolved
Hide resolved
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Outdated
Show resolved
Hide resolved
* Removed MAX_THREAD_POOL_SIZE and VIRTUAL_THREAD_THRESHOLD, replacing them with MAX_PLATFORM_THREADS and MAX_VIRTUAL_THREADS. * Improve documentation to include @sees for java implementation specifics. Took 7 minutes
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt (1)
77-77: Thread naming implementation is correctThis addresses the past review concern about thread naming. The
Thread.Builder.name()method with index parameter will automatically append unique numbers to distinguish threads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt (4)
8-10: LGTM: Required imports for custom thread pool implementationThe new imports are necessary for the custom
ThreadPoolExecutorimplementation and are correctly added.
39-39: Good implementation of CPU-based scalingThis addresses the past review feedback about making values dynamic based on available processors. The
* 2multiplier aligns with gabber235's suggestion.
49-49: Virtual thread scaling looks appropriateThe
* 10multiplier for virtual threads follows the previous discussion and should provide good scaling for virtual thread workloads.
87-93: LGTM: Formatting improvements with no logic changesThe extension function formatting has been improved without altering the underlying logic.
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Show resolved
Hide resolved
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Show resolved
Hide resolved
…FORM_THREADS and MAX_VIRTUAL_THREADS. * I forgot. Took 4 minutes
|
May want to add more documentation but it is really not that important, since this system is so low-level. No extension developers will likely ever see these fields. |
2 participants
Implement a maximum number of threads to exist in the pool.
Implement a threshold to stop creating platform threads and leverage virtual threads instead.
Implement a core size of threads, those of which never leave the pool to buffer inactivity.
Summary by CodeRabbit
New Features
Improvements