Skip to content

Replace jctools NonBlockingHashMap with ConcurrentHashMap #9700

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.

Sign up for GitHub

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

Merged
amarziali merged 2 commits into from
Oct 8, 2025
Merged

Replace jctools NonBlockingHashMap with ConcurrentHashMap #9700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9b2108
Replace jctools NonBlockingHashMap with ConcurrentHashMap
amarziali Oct 8, 2025
81fe323
remove map usage on metadataevent
amarziali Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions .../src/main/resources/META-INF/native-image/com.datadoghq/dd-java-agent/reflect-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,5 @@
"fields": [
{"name": "consumerIndex", "allowUnsafeAccess": true}
]
},
{
"name" : "datadog.jctools.maps.NonBlockingHashMap",
"fields": [
{"name": "_kvs", "allowUnsafeAccess": true}
]
}
]
19 changes: 0 additions & 19 deletions ...ntroller-jfr/src/main/java/com/datadog/profiling/controller/jfr/parser/MetadataEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import org.jctools.maps.NonBlockingHashMapLong;

/**
* JFR Chunk metadata
Expand All @@ -17,10 +16,6 @@ public final class MetadataEvent {
public final long duration;
public final long metadataId;

private final NonBlockingHashMapLong<String> eventTypeNameMapBacking =
new NonBlockingHashMapLong<>(256);
Copy link
Contributor

@mcculls mcculls Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One key differentiator of NonBlockingHashMapLong is it supports primitive long keys, whereas ConcurrentHashMap will box primitive keys into Long.

Could we add a benchmark comparing NonBlockingHashMapLong<String> with ConcurrentHashMap<Long, String> to see what effect that has? Note we should make sure we test long values outside of the cached range of -128 to 127

Copy link
Contributor Author

@amarziali amarziali Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Let me test the side effect of unboxing

Copy link
Contributor

@mcculls mcculls Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth checking with profiling where the getEventTypeNameMap method is used, because I can't find a caller in the current codebase. If it's not used then do we need to keep this map at all?

Copy link
Contributor Author

@amarziali amarziali Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the put operation I don't think nothing will change since I see later on:

      eventTypeNameMapBacking.put(Long.parseLong(id), name);

I cannot see any caller as well...let me ask around or dig more about this

Copy link
Contributor Author

@amarziali amarziali Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I got confirmation that it's unused. Actually the whole package can be removed apparently. Now in this PR I limited to removing that map. I will open another PR to delete the package

private final LongMapping<String> eventTypeMap;

MetadataEvent(RecordingStream stream) throws IOException {
size = (int) stream.readVarint();
long typeId = stream.readVarint();
Expand All @@ -31,16 +26,6 @@ public final class MetadataEvent {
duration = stream.readVarint();
metadataId = stream.readVarint();
readElements(stream, readStringTable(stream));
eventTypeMap = eventTypeNameMapBacking::get;
}

/**
* Lazily compute and return the mappings of event type ids to event type names
*
* @return mappings of event type ids to event type names
*/
public LongMapping<String> getEventTypeNameMap() {
return eventTypeMap;
}

private String[] readStringTable(RecordingStream stream) throws IOException {
Expand Down Expand Up @@ -76,10 +61,6 @@ private void readElements(RecordingStream stream, String[] stringConstants) thro
}
}
}
// only event types are currently collected
if (name != null && id != null && "jdk.jfr.Event".equals(superType)) {
eventTypeNameMapBacking.put(Long.parseLong(id), name);
}
// now inspect all the enclosed elements
int elemCount = (int) stream.readVarint();
for (int i = 0; i < elemCount; i++) {
Expand Down
68 changes: 68 additions & 0 deletions dd-trace-core/src/jmh/java/datadog/trace/common/NonBlockingHashMapBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package datadog.trace.common;

import static java.util.concurrent.TimeUnit.MICROSECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;

import de.thetaphi.forbiddenapis.SuppressForbidden;
import java.util.concurrent.ConcurrentHashMap;
import org.jctools.maps.NonBlockingHashMap;
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;
import org.openjdk.jmh.infra.Blackhole;

/*
JDK 1.8
Benchmark Mode Cnt Score Error Units
NonBlockingHashMapBenchmark.benchConcurrentHashMap avgt 1.153 us/op
NonBlockingHashMapBenchmark.benchNonBlockingHashMap avgt 1.457 us/op
JDK 21
Benchmark Mode Cnt Score Error Units
NonBlockingHashMapBenchmark.benchConcurrentHashMap avgt 1.088 us/op
NonBlockingHashMapBenchmark.benchNonBlockingHashMap avgt 1.278 us/op
*/
@State(Scope.Benchmark)
@Warmup(iterations = 1, time = 30, timeUnit = SECONDS)
@Measurement(iterations = 1, time = 30, timeUnit = SECONDS)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(MICROSECONDS)
@Fork(value = 1)
@SuppressForbidden
public class NonBlockingHashMapBenchmark {
private NonBlockingHashMap nonBlockingHashMap;
private ConcurrentHashMap concurrentHashMap;

@Setup(Level.Iteration)
public void setup() {
nonBlockingHashMap = new NonBlockingHashMap(512);
concurrentHashMap = new ConcurrentHashMap(512);
for (int i = 0; i < 256; i++) {
nonBlockingHashMap.put("test" + i, "test");
concurrentHashMap.put("test" + i, "test");
}
Comment on lines +45 to +52
Copy link
Contributor

@AlexeyKuznetsov-DD AlexeyKuznetsov-DD Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to test Map<MetricKey, Object> as it is an actual use case?

Copy link
Contributor Author

@amarziali amarziali Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the key influencing the test?

Copy link
Contributor

@AlexeyKuznetsov-DD AlexeyKuznetsov-DD Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but it will mimic real usage I believe...

Copy link
Contributor

@bric3 bric3 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not interesting in this case as we will measure the MetricKey code itself. While "test" + i becomes a String.

}

@Benchmark
@Threads(Threads.MAX)
public void benchNonBlockingHashMap(Blackhole blackhole) {
nonBlockingHashMap.put("test", "test");
blackhole.consume(nonBlockingHashMap.remove("test"));
}

@Benchmark
@Threads(Threads.MAX)
public void benchConcurrentHashMap(Blackhole blackhole) {
concurrentHashMap.put("test", "test");
blackhole.consume(concurrentHashMap.remove("test"));
}
}
8 changes: 4 additions & 4 deletions dd-trace-core/src/main/java/datadog/trace/common/metrics/Aggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
import java.util.Map;
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.TimeUnit;
import org.jctools.maps.NonBlockingHashMap;
import org.jctools.queues.MessagePassingQueue;
import org.jctools.queues.MpscCompoundQueue;
import org.slf4j.Logger;
Expand All @@ -24,7 +24,7 @@ final class Aggregator implements Runnable {
private final Queue<Batch> batchPool;
private final MpscCompoundQueue<InboxItem> inbox;
private final LRUCache<MetricKey, AggregateMetric> aggregates;
private final NonBlockingHashMap<MetricKey, Batch> pending;
private final ConcurrentMap<MetricKey, Batch> pending;
private final Set<MetricKey> commonKeys;
private final MetricWriter writer;
// the reporting interval controls how much history will be buffered
Expand All @@ -40,7 +40,7 @@ final class Aggregator implements Runnable {
MetricWriter writer,
Queue<Batch> batchPool,
MpscCompoundQueue<InboxItem> inbox,
NonBlockingHashMap<MetricKey, Batch> pending,
ConcurrentMap<MetricKey, Batch> pending,
final Set<MetricKey> commonKeys,
int maxAggregates,
long reportingInterval,
Expand All @@ -61,7 +61,7 @@ final class Aggregator implements Runnable {
MetricWriter writer,
Queue<Batch> batchPool,
MpscCompoundQueue<InboxItem> inbox,
NonBlockingHashMap<MetricKey, Batch> pending,
ConcurrentMap<MetricKey, Batch> pending,
final Set<MetricKey> commonKeys,
int maxAggregates,
long reportingInterval,
Expand Down
10 changes: 5 additions & 5 deletions dd-trace-core/src/main/java/datadog/trace/common/metrics/ConflatingMetricsAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import org.jctools.maps.NonBlockingHashMap;
import org.jctools.queues.MpscCompoundQueue;
import org.jctools.queues.SpmcArrayQueue;
import org.slf4j.Logger;
Expand Down Expand Up @@ -90,8 +90,8 @@ public final class ConflatingMetricsAggregator implements MetricsAggregator, Eve

private final Set<String> ignoredResources;
private final Queue<Batch> batchPool;
private final NonBlockingHashMap<MetricKey, Batch> pending;
private final NonBlockingHashMap<MetricKey, MetricKey> keys;
private final ConcurrentHashMap<MetricKey, Batch> pending;
private final ConcurrentHashMap<MetricKey, MetricKey> keys;
private final Thread thread;
private final MpscCompoundQueue<InboxItem> inbox;
private final Sink sink;
Expand Down Expand Up @@ -178,8 +178,8 @@ public ConflatingMetricsAggregator(
this.ignoredResources = ignoredResources;
this.inbox = new MpscCompoundQueue<>(queueSize);
this.batchPool = new SpmcArrayQueue<>(maxAggregates);
this.pending = new NonBlockingHashMap<>(maxAggregates * 4 / 3);
this.keys = new NonBlockingHashMap<>();
this.pending = new ConcurrentHashMap<>(maxAggregates * 4 / 3);
this.keys = new ConcurrentHashMap<>();
this.features = features;
this.healthMetrics = healthMetric;
this.sink = sink;
Expand Down