Skip to content

Commit 3fe9af5

Browse files
dougqhclaude
andcommitted
Address review: numSlots() accessor + comment cleanup
- StringIndex: replace the public `slots` field with a `numSlots()` method on both the instance and Support (don't expose it as a field); update the two StringIndexTest call sites. - StringIndex Javadoc: drop Data from the "ways to use it" list — it's just create's multi-value return, not a usage mode. - ImmutableSetBenchmark: trim the "Key findings" prose to conclusions (the results table carries the numbers; relative comparisons kept). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9b6c445 commit 3fe9af5

3 files changed

Lines changed: 21 additions & 22 deletions

File tree

internal-api/src/jmh/java/datadog/trace/util/ImmutableSetBenchmark.java

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -63,19 +63,17 @@
6363
* <p>Key findings:
6464
*
6565
* <ul>
66-
* <li>The static {@code Support} path is the fastest membership structure — it beats {@code
67-
* HashSet} on both hit and miss, and crushes the scan/search/tree forms.
66+
* <li>The static {@code Support} path is the fastest — it beats {@code HashSet} on hit and miss
67+
* and crushes the scan/search/tree forms.
6868
* <li>{@code stringIndex} (the instance wrapper) trails {@code Support} by the field-load
69-
* indirection (~10% on hit), landing near {@code HashSet}. Off the hot path that cost is
70-
* fine; on it, prefer {@code Support}.
71-
* <li>{@link java.util.Set#copyOf} ({@code SetN}, the compact array-backed form the agent uses
72-
* for fixed config sets) is ~1.2x behind {@code Support} on hit — but it is the most
73-
* <i>compact</i> (~27% smaller than StringIndex per the JOL test): StringIndex pays for its
74-
* cached {@code int[]} hashes + the 2x-oversized table. So StringIndex's edge over {@code
75-
* SetN} is speed + the {@code indexOf}-&gt;parallel-array capability, NOT footprint; vs
76-
* {@code HashSet} it wins on both.
77-
* <li>{@code array} / {@code sortedArray} / {@code treeSet} scan, binary-search, or tree-walk per
78-
* lookup, so they trail the hashed structures, most visibly on miss.
69+
* indirection (~10% on hit), landing near {@code HashSet} — fine off the hot path, prefer
70+
* {@code Support} on it.
71+
* <li>{@link java.util.Set#copyOf} ({@code SetN}, the agent's compact fixed-set form) is ~1.2x
72+
* behind {@code Support} on hit but the most <i>compact</i> (~27% smaller — no cached hashes,
73+
* no 2x table). So StringIndex's edge over {@code SetN} is speed + the {@code
74+
* indexOf}-&gt;parallel-array capability, not footprint; over {@code HashSet} it wins both.
75+
* <li>{@code array} / {@code sortedArray} / {@code treeSet} trail the hashed structures, most on
76+
* miss.
7977
* </ul>
8078
*
8179
* <p><b>Caveat — the instance {@code stringIndex} miss is bimodal across forks</b> (confirmed at

internal-api/src/main/java/datadog/trace/util/StringIndex.java

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,19 @@
88
/**
99
* Flat open-addressed name set. Generic — it knows only names.
1010
*
11-
* <p>Three ways to use it, trading convenience for indirection:
11+
* <p>Two ways to use it, trading convenience for indirection:
1212
*
1313
* <ul>
1414
* <li>{@link Support} — static algorithm over <b>raw arrays</b>. Keep the arrays in your own
1515
* (ideally {@code static final}) fields and the JIT folds the refs to constants. The fastest
1616
* path; nothing to dereference.
17-
* <li>{@link Data} — a <b>build-time carrier</b> for the placed {@code {hashes, names}} returned
18-
* by {@link Support#create}. Pull its fields into your own and discard it.
1917
* <li>The {@code StringIndex} <b>instance</b> ({@link #of}) — a convenience wrapper holding the
2018
* arrays; {@link #indexOf}/{@link #contains} delegate to {@link Support}. Costs an
2119
* instance-field load per call (the indirection the static path removes) — fine off the hot
2220
* path.
2321
* </ul>
2422
*
25-
* <p>Consumers attach their own parallel payload arrays (ids, values, ...) sized to {@link #slots}
23+
* <p>Consumers attach their own parallel payload arrays (ids, values, ...) sized to {@link #numSlots()}
2624
* and indexed by the slot {@code indexOf} returns. {@code mapValues}/{@code mapIntValues}/{@code
2725
* mapLongValues} build such an array at construction; {@code lookup}/{@code lookupOrDefault} read
2826
* one back in a single call (slot resolve + array read).
@@ -43,12 +41,10 @@
4341
public final class StringIndex {
4442
private final int[] hashes;
4543
private final String[] names;
46-
public final int slots; // == hashes.length
4744

4845
private StringIndex(int[] hashes, String[] names) {
4946
this.hashes = hashes;
5047
this.names = names;
51-
this.slots = hashes.length;
5248
}
5349

5450
/**
@@ -69,8 +65,8 @@ public boolean contains(String name) {
6965
}
7066

7167
/** Table size — allocate parallel payload arrays of this length. */
72-
public int slots() {
73-
return this.slots;
68+
public int numSlots() {
69+
return hashes.length;
7470
}
7571

7672
// --- value mapping: build a slot-aligned parallel array (off the hot path) ---
@@ -250,6 +246,11 @@ public static int indexOf(int[] hashes, String[] names, String name) {
250246
return indexOf(hashes, names, name, hash(name));
251247
}
252248

249+
/** Number of slots — the length to size parallel payload arrays to. */
250+
public static int numSlots(int[] hashes) {
251+
return hashes.length;
252+
}
253+
253254
/** {@code data[indexOf(...)]}, or {@code null} when {@code key} is absent. */
254255
public static <T> T lookup(int[] hashes, String[] names, T[] data, String key) {
255256
int slot = indexOf(hashes, names, key);

internal-api/src/test/java/datadog/trace/util/StringIndexTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ void tableSizeFor_isPow2_andOversized() {
3535
void instance_contains_internedAndCopy_andMiss() {
3636
StringIndex set = StringIndex.of("foo", "bar", "baz");
3737

38-
assertEquals(8, set.slots()); // 3 names -> tableSizeFor(3) == 8
38+
assertEquals(8, set.numSlots()); // 3 names -> tableSizeFor(3) == 8
3939

4040
assertTrue(set.contains("foo")); // interned literal -> == fast path in eq
4141
assertTrue(set.contains(new String("bar"))); // non-interned -> .equals path
@@ -106,7 +106,7 @@ void mapIntValues_slotAligned_andLookup() {
106106
StringIndex idx = StringIndex.of("a", "b", "c");
107107
// 1-based ids; 0 stays the empty-slot / not-found sentinel.
108108
int[] ids = idx.mapIntValues(s -> s.charAt(0) - 'a' + 1);
109-
assertEquals(idx.slots(), ids.length); // sized to the table, not the name count
109+
assertEquals(idx.numSlots(), ids.length); // sized to the table, not the name count
110110

111111
assertEquals(1, idx.lookup(ids, "a"));
112112
assertEquals(2, idx.lookup(ids, "b"));

0 commit comments

Comments
 (0)