Fix virtual-thread pinning in StatsDAggregator#295
Open
Conversation
Replaces per-shard synchronized(map) blocks with explicit ReentrantReadWriteLock[], eliminating the virtual-thread pinning source. Also switches aggregateMetrics from ArrayList to a plain array to avoid indirection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ReadWriteLock was overkill since only write paths exist. ReentrantLock is simpler and has lower AQS overhead. Also removed the now-uncontended synchronized(map) block from the test that was locking on an object no production code uses. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
vickenty
reviewed
Feb 26, 2026
| public static int DEFAULT_SHARDS = 4; // 4 partitions to reduce contention. | ||
|
|
||
| protected final String AGGREGATOR_THREAD_NAME = "statsd-aggregator-thread"; | ||
| protected final ArrayList<Map<Message, Message>> aggregateMetrics; |
Contributor
There was a problem hiding this comment.
Would it be possible to keep this PR about the lock only?
Contributor
|
Also, please add link to the JEP where synchronized issue and solutions are explained. |
Keep the PR focused on the lock change only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@vickenty Added relevant info sources to PR description |
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
2 participants
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 does this PR do?
Replaces
synchronized(map)blocks inStatsDAggregatorwith explicit per-shardReentrantLock, eliminating the virtual-thread carrier-thread pinning source. Also removes the now-uncontendedsynchronized(map)from the aggregation sharding test.synchronizedon a regular object monitor pins a virtual thread to its carrier thread for the duration of the lock (JEP 444).ReentrantLock(backed byAbstractQueuedSynchronizer) parks the virtual thread instead, freeing the carrier. Note: JDK 24+ resolves this at the JVM level via JEP 491, but this fix benefits all supported JDK versions.Motivation
Virtual threads pin to their carrier thread when blocked inside a
synchronizedblock. With the aggregator under load from virtual-thread callers this caused unnecessary carrier-thread starvation.Benchmarks
JMH
VirtualThreadAggregatorBenchmarkrun againstmaster(baseline) and this branch shows the fix eliminates the pinning source with no meaningful throughput regression: