-
Notifications
You must be signed in to change notification settings - Fork 359
Add find-or-insert Reservation API to ConcurrentHashtable #12462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dougqh
wants to merge
13
commits into
master
Choose a base branch
from
feat/concurrenthashtable-reservation-api-v2
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
b38bab3
Add find-or-insert Reservation API to ConcurrentHashtable
dougqh b8b5037
Move RawLogMessage equality logic into matches(), have equals() delegate
dougqh fe622b3
Simplify LogCollector's find-or-insert and fix a missed increment
dougqh 14d52bc
Finish ReentrantLock migration for ConcurrentHashtable and shorten dr…
dougqh 474241a
Add JMH benchmarks for hashIterable/hashIterator allocation and drain…
dougqh 3349e9d
Add whole-sweep-lock variant to drain benchmark for A/B comparison
dougqh b632878
Prefix hashIterable call with ConcurrentHashtable in LogCollector.find
dougqh f1d5316
Note removeIf as a candidate for the same per-bucket lock tradeoff as…
dougqh fa44aac
Trim inline comments per review feedback
dougqh 4012524
Add test coverage for remaining Reservation overloads
dougqh 16b56c8
Fix duplicate-undercounting race in ConcurrentHashtable reservations
dougqh b226804
Add a near-capacity JMH benchmark for LogCollector
dougqh c87c904
Merge tryReserve and tryReserveFor into a single keyHash-aware method
dougqh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
173 changes: 173 additions & 0 deletions
173
internal-api/src/jmh/java/datadog/trace/util/ConcurrentHashtableDrainBenchmark.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| package datadog.trace.util; | ||
|
|
||
| import static java.util.concurrent.TimeUnit.MICROSECONDS; | ||
|
|
||
| import java.util.concurrent.atomic.AtomicReferenceArray; | ||
| import java.util.concurrent.locks.ReentrantLock; | ||
| import java.util.function.Consumer; | ||
| import javax.annotation.Nonnull; | ||
| import org.openjdk.jmh.annotations.Benchmark; | ||
| import org.openjdk.jmh.annotations.BenchmarkMode; | ||
| import org.openjdk.jmh.annotations.Fork; | ||
| import org.openjdk.jmh.annotations.Group; | ||
| import org.openjdk.jmh.annotations.GroupThreads; | ||
| import org.openjdk.jmh.annotations.Level; | ||
| import org.openjdk.jmh.annotations.Measurement; | ||
| import org.openjdk.jmh.annotations.Mode; | ||
| import org.openjdk.jmh.annotations.OutputTimeUnit; | ||
| import org.openjdk.jmh.annotations.Scope; | ||
| import org.openjdk.jmh.annotations.Setup; | ||
| import org.openjdk.jmh.annotations.State; | ||
| import org.openjdk.jmh.annotations.Warmup; | ||
|
|
||
| /** | ||
| * Compares writer throughput against a continuously draining table under two locking schemes: | ||
| * | ||
| * <ul> | ||
| * <li>{@code perBucketLock} -- {@link ConcurrentHashtable#drain}'s current strategy: the table | ||
| * lock is acquired and released once per bucket, so a writer can slip in between buckets | ||
| * instead of waiting out the whole sweep. | ||
| * <li>{@code wholeSweepLock} -- the strategy {@code drain()} used before this PR: the table lock | ||
| * is acquired once and held for the entire sweep. Reimplemented locally ({@link | ||
| * #drainWholeSweepLock}) using only {@code ConcurrentHashtable}'s public building blocks, so | ||
| * this file doesn't depend on checking out an older commit to get the comparison. | ||
| * </ul> | ||
| * | ||
| * <p>Both variants release capacity per-entry (one {@link | ||
| * ConcurrentHashtable.SizeManager#decrement()} per drained entry, mirroring current production | ||
| * behavior) so the only variable under test is lock granularity, not the capacity-release change | ||
| * that landed alongside it. | ||
| * | ||
| * <p>The writer mirrors {@code LogCollector.addLogMessage}'s shape: a lock-free scan over a small, | ||
| * rotating key set ({@link #N_KEYS} distinct log groups) that hits on the fast path once a key has | ||
| * been inserted, falling back to {@link ConcurrentHashtable#tryReserve} (which holds the table lock | ||
| * for the reservation's lifetime) only on a miss. Because {@code drainer} periodically empties the | ||
| * table, writers keep taking the slow, lock-holding path throughout the run instead of settling | ||
| * into steady-state lock-free hits -- this keeps occupancy well under capacity so writers aren't | ||
| * dominated by the lock-free {@code isFull()} fast-reject once full, and the throughput actually | ||
| * reflects contention for the table lock against an in-progress drain. | ||
| * | ||
| * <pre>{@code | ||
| * ./gradlew :internal-api:jmh -Pjmh.includes=ConcurrentHashtableDrainBenchmark -Pjmh.forks=1 | ||
| * }</pre> | ||
| */ | ||
| @Fork(2) | ||
| @Warmup(iterations = 2) | ||
| @Measurement(iterations = 3) | ||
| @BenchmarkMode(Mode.Throughput) | ||
| @OutputTimeUnit(MICROSECONDS) | ||
| public class ConcurrentHashtableDrainBenchmark { | ||
|
|
||
| static final int N_KEYS = 32; | ||
| static final int CAPACITY = 64; | ||
|
|
||
| static final long[] KEY_HASHES = new long[N_KEYS]; | ||
|
|
||
| static { | ||
| for (int i = 0; i < N_KEYS; ++i) { | ||
| KEY_HASHES[i] = LongHashingUtils.hash("key-" + i); | ||
| } | ||
| } | ||
|
|
||
| static final class DrainEntry extends ConcurrentHashtable.Entry<DrainEntry> { | ||
| DrainEntry(long keyHash) { | ||
| super(keyHash); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean matches(@Nonnull DrainEntry other) { | ||
| // hashIterator already filters candidates by keyHash equality before yielding them. | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The pre-PR strategy: one lock acquisition held for the entire bucket-array sweep, still | ||
| * releasing capacity per-entry (only lock granularity differs from the current {@code drain()}). | ||
| */ | ||
| private static <TEntry extends ConcurrentHashtable.Entry<TEntry>> void drainWholeSweepLock( | ||
| @Nonnull ConcurrentHashtable.State<TEntry> state, | ||
| @Nonnull Consumer<? super TEntry> drainedEntryConsumer) { | ||
| ReentrantLock lock = ConcurrentHashtable.getTableWriteLock(state); | ||
| AtomicReferenceArray<TEntry> buckets = state.buckets; | ||
| lock.lock(); | ||
| try { | ||
| for (int i = 0; i < buckets.length(); i++) { | ||
| TEntry head = buckets.get(i); | ||
| if (head == null) { | ||
| continue; | ||
| } | ||
| buckets.set(i, null); | ||
| for (TEntry e = head; e != null; e = e.next()) { | ||
| state.sizeManager.decrement(); | ||
| drainedEntryConsumer.accept(e); | ||
| } | ||
| } | ||
| state.sizeManager.release(0); // full sweep: reset the scan position | ||
| } finally { | ||
| lock.unlock(); | ||
| } | ||
| } | ||
|
|
||
| @State(Scope.Benchmark) | ||
| public static class SharedState { | ||
| ConcurrentHashtable.State<DrainEntry> table; | ||
|
|
||
| @Setup(Level.Iteration) | ||
| public void setUp() { | ||
| table = ConcurrentHashtable.createBounded(DrainEntry.class, CAPACITY); | ||
| } | ||
| } | ||
|
|
||
| @State(Scope.Thread) | ||
| public static class WriterState { | ||
| int cursor; | ||
|
|
||
| int next() { | ||
| int i = cursor; | ||
| cursor = (i + 1) & (N_KEYS - 1); | ||
| return i; | ||
| } | ||
| } | ||
|
|
||
| private static void write(ConcurrentHashtable.State<DrainEntry> table, WriterState w) { | ||
| long keyHash = KEY_HASHES[w.next()]; | ||
| for (DrainEntry entry : ConcurrentHashtable.hashIterable(table, keyHash)) { | ||
| return; // lock-free hit, mirroring LogCollector.find() | ||
| } | ||
| try (ConcurrentHashtable.Reservation<DrainEntry> r = | ||
| ConcurrentHashtable.tryReserve(table, keyHash)) { | ||
| if (r.isReserved()) { | ||
| r.tryGetOrInsertOrNull(new DrainEntry(keyHash)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Benchmark | ||
| @Group("perBucketLock") | ||
| @GroupThreads(1) | ||
| public void perBucketLockDrainer(SharedState s) { | ||
| ConcurrentHashtable.drain(s.table, entry -> {}); | ||
| } | ||
|
|
||
| @Benchmark | ||
| @Group("perBucketLock") | ||
| @GroupThreads(3) | ||
| public void perBucketLockWriter(SharedState s, WriterState w) { | ||
| write(s.table, w); | ||
| } | ||
|
|
||
| @Benchmark | ||
| @Group("wholeSweepLock") | ||
| @GroupThreads(1) | ||
| public void wholeSweepLockDrainer(SharedState s) { | ||
| drainWholeSweepLock(s.table, entry -> {}); | ||
| } | ||
|
|
||
| @Benchmark | ||
| @Group("wholeSweepLock") | ||
| @GroupThreads(3) | ||
| public void wholeSweepLockWriter(SharedState s, WriterState w) { | ||
| write(s.table, w); | ||
| } | ||
| } |
102 changes: 102 additions & 0 deletions
102
internal-api/src/jmh/java/datadog/trace/util/ConcurrentHashtableFindBenchmark.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| package datadog.trace.util; | ||
|
|
||
| import static java.util.concurrent.TimeUnit.MICROSECONDS; | ||
|
|
||
| import javax.annotation.Nonnull; | ||
| import org.openjdk.jmh.annotations.Benchmark; | ||
| import org.openjdk.jmh.annotations.BenchmarkMode; | ||
| import org.openjdk.jmh.annotations.Fork; | ||
| import org.openjdk.jmh.annotations.Level; | ||
| import org.openjdk.jmh.annotations.Measurement; | ||
| import org.openjdk.jmh.annotations.Mode; | ||
| import org.openjdk.jmh.annotations.OutputTimeUnit; | ||
| import org.openjdk.jmh.annotations.Scope; | ||
| import org.openjdk.jmh.annotations.Setup; | ||
| import org.openjdk.jmh.annotations.State; | ||
| import org.openjdk.jmh.annotations.Threads; | ||
| import org.openjdk.jmh.annotations.Warmup; | ||
|
|
||
| /** | ||
| * Measures the lock-free hash-bucket scan used by {@link LogCollector#find}: a {@code for} loop | ||
| * over {@link ConcurrentHashtable#hashIterable}, matching by key hash then a per-entry predicate. | ||
| * | ||
| * <p>Run with {@code -Pjmh.profilers=gc} to confirm the {@link Iterable}/{@link java.util.Iterator} | ||
| * allocated per call (the anonymous instances returned by {@link | ||
| * ConcurrentHashtable#hashIterable}/{@link ConcurrentHashtable#hashIterator}) are scalar-replaced | ||
| * away by escape analysis rather than landing on the heap: | ||
| * | ||
| * <pre>{@code | ||
| * ./gradlew :internal-api:jmh -Pjmh.includes=ConcurrentHashtableFindBenchmark -Pjmh.profilers=gc -Pjmh.forks=1 | ||
| * }</pre> | ||
| */ | ||
| @Fork(2) | ||
| @Warmup(iterations = 2) | ||
| @Measurement(iterations = 3) | ||
| @BenchmarkMode(Mode.Throughput) | ||
| @OutputTimeUnit(MICROSECONDS) | ||
| @Threads(8) | ||
| public class ConcurrentHashtableFindBenchmark { | ||
|
|
||
| static final int N_KEYS = 64; | ||
| static final int CAPACITY = 128; | ||
|
|
||
| static final long[] KEY_HASHES = new long[N_KEYS]; | ||
|
|
||
| static { | ||
| for (int i = 0; i < N_KEYS; ++i) { | ||
| KEY_HASHES[i] = LongHashingUtils.hash("key-" + i); | ||
| } | ||
| } | ||
|
|
||
| /** Mirrors {@code LogCollector.RawLogMessage}: a keyHash plus a payload compared on match. */ | ||
| static final class FindEntry extends ConcurrentHashtable.Entry<FindEntry> { | ||
| final int payload; | ||
|
|
||
| FindEntry(long keyHash, int payload) { | ||
| super(keyHash); | ||
| this.payload = payload; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean matches(@Nonnull FindEntry other) { | ||
| return payload == other.payload; | ||
| } | ||
| } | ||
|
|
||
| @State(Scope.Benchmark) | ||
| public static class SharedState { | ||
| ConcurrentHashtable.State<FindEntry> table; | ||
|
|
||
| @Setup(Level.Iteration) | ||
| public void setUp() { | ||
| table = ConcurrentHashtable.createBounded(FindEntry.class, CAPACITY); | ||
| for (int i = 0; i < N_KEYS; ++i) { | ||
| ConcurrentHashtable.tryReserve(table, KEY_HASHES[i]) | ||
| .tryGetOrInsertOrNull(new FindEntry(KEY_HASHES[i], i)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @State(Scope.Thread) | ||
| public static class ThreadState { | ||
| int cursor; | ||
|
|
||
| int next() { | ||
| int i = cursor; | ||
| cursor = (i + 1) & (N_KEYS - 1); | ||
| return i; | ||
| } | ||
| } | ||
|
|
||
| /** Same loop shape as {@code LogCollector.find}: scan candidates for a keyHash, match, return. */ | ||
| @Benchmark | ||
| public FindEntry find(SharedState s, ThreadState t) { | ||
| int i = t.next(); | ||
| for (FindEntry entry : ConcurrentHashtable.hashIterable(s.table, KEY_HASHES[i])) { | ||
| if (entry.payload == i) { | ||
| return entry; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| } |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.