From 5b96e61b442aa2cd2d922ab41d9a74107a6eec48 Mon Sep 17 00:00:00 2001 From: Michael Braun Date: Thu, 17 Sep 2026 17:58:50 +0000 Subject: [PATCH 1/2] Store AsciiSet membership as two longs instead of boolean[128] Membership moves from a boolean[128] to a pair of long bitmasks. Whole-set operations (union/intersection/diff/invert/equals/hashCode/ isEmpty/character) drop from an O(128) loop to a couple of ALU ops, and contains/containsAll go from a bounds-checked array load to a shift and mask. fromPattern builds the pair in two locals rather than a long[2], since an array indexed by a runtime value is not scalar replaced and would allocate on every call. toPattern is no longer called from the constructor. The old code built the pattern string eagerly for every instance, which is what made the set algebra slow: union was paying for a StringBuilder walk over 128 chars per call. toString now derives it on demand, which keeps that cost off construction without adding a cached field for what is a debug path. hashCode multiplies by a mixing constant rather than folding the two words through Long.hashCode. Long.hashCode xors a value's halves together and ~hi ^ ~lo == hi ^ lo, so folding directly would give every set the same hash as its complement, and a set of chars 0-63 the same hash as the same bit pattern over chars 64-127. equals is exact so no lookup was wrong, but a char class and its negation are a common pair in a compiled matcher, so they collided in Optimizer.dedupOr's hash set. The conventional 31 is too small to diffuse a raw mask before half of it is discarded; it still maps a-z onto A-Z. serialVersionUID is bumped to 2: serialization matches by field name, so an old stream would otherwise deserialize into a silently empty set. memberBit relies on JLS 15.19 implicit shift-distance masking rather than an explicit `c & 0x3F`, since x86's shrx already masks in hardware and the explicit form compiles to a redundant andl per character. That one is easy to regress, so it is called out in a comment. Adds AsciiSetBench covering the clean and dirty containsAll and replaceNonMembers paths plus the set algebra used while compiling a pattern, and a test pinning the hashCode collision behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- .../netflix/spectator/perf/AsciiSetBench.java | 131 ++++++++++++++ .../com/netflix/spectator/impl/AsciiSet.java | 170 +++++++++--------- .../netflix/spectator/impl/AsciiSetTest.java | 15 ++ 3 files changed, 235 insertions(+), 81 deletions(-) create mode 100644 spectator-api/src/jmh/java/com/netflix/spectator/perf/AsciiSetBench.java diff --git a/spectator-api/src/jmh/java/com/netflix/spectator/perf/AsciiSetBench.java b/spectator-api/src/jmh/java/com/netflix/spectator/perf/AsciiSetBench.java new file mode 100644 index 000000000..fb855d480 --- /dev/null +++ b/spectator-api/src/jmh/java/com/netflix/spectator/perf/AsciiSetBench.java @@ -0,0 +1,131 @@ +/* + * Copyright 2014-2026 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.netflix.spectator.perf; + +import com.netflix.spectator.impl.AsciiSet; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.Optional; + +/** + * Measures {@link AsciiSet} on the inputs that matter in practice: {@code containsAll} and + * {@code replaceNonMembers} cover the meter lookup path, the rest cover pattern compilation. + * + *

To A/B a change here, build two jmh jars from two git states of {@code AsciiSet.java} and + * run this benchmark against each, rather than reproducing an old implementation in this file: + * which class a method compiles as part of can shift its measured throughput more than the + * algorithm difference being isolated.

+ */ +@State(Scope.Thread) +public class AsciiSetBench { + + /** The character set the Atlas registry applies to every tag key and value. */ + private static final String VALID = "-._A-Za-z0-9~^"; + + private final AsciiSet set = AsciiSet.fromPattern(VALID); + private final AsciiSet other = AsciiSet.fromPattern("a-z"); + private final AsciiSet setCopy = AsciiSet.fromPattern(VALID); + + // Built in setup, not assigned from literals inline, to avoid constant folding. + + /** Typical tag value: short and already valid. */ + private String shortClean; + + /** Typical metric name: longer, still valid. */ + private String longClean; + + /** A value that needs fixing, with the first bad character near the end. */ + private String dirtyLate; + + /** A value that needs fixing from the very first character. */ + private String dirtyEarly; + + /** Text to build a set from, kept separate from {@code VALID} for the same reason as above. */ + private String patternText; + + @Setup + public void setup() { + shortClean = new String("us-east-1c".toCharArray()); + longClean = new String("ipc.server.call.duration.percentile".toCharArray()); + dirtyLate = new String("deploy.prod.us-east-1/cluster".toCharArray()); + dirtyEarly = new String("/api/v1/users?id=42&name=bob".toCharArray()); + patternText = new String(VALID.toCharArray()); + } + + @Benchmark public AsciiSet fromPattern() { + return AsciiSet.fromPattern(patternText); + } + + // containsAll on already valid input: the dominant case on the meter lookup path. + + @Benchmark public boolean shortClean() { + return set.containsAll(shortClean); + } + + @Benchmark public boolean longClean() { + return set.containsAll(longClean); + } + + // containsAll that fails, so the scan exits early. + + @Benchmark public boolean dirty() { + return set.containsAll(dirtyLate); + } + + // replaceNonMembers: the allocating path. + + @Benchmark public String replaceClean() { + return set.replaceNonMembers(longClean, '_'); + } + + @Benchmark public String replaceLate() { + return set.replaceNonMembers(dirtyLate, '_'); + } + + @Benchmark public String replaceEarly() { + return set.replaceNonMembers(dirtyEarly, '_'); + } + + // Single character membership, as used by the pattern matcher. + + @Benchmark public void contains(Blackhole bh) { + bh.consume(set.contains('a')); + bh.consume(set.contains('/')); + bh.consume(set.contains('é')); + } + + // Set algebra and identity, as used while compiling a pattern. + + @Benchmark public AsciiSet union() { + return set.union(other); + } + + @Benchmark public boolean equalsSet() { + return set.equals(setCopy); + } + + @Benchmark public boolean isEmpty() { + return set.isEmpty(); + } + + @Benchmark public Optional character() { + return set.character(); + } +} diff --git a/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java b/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java index f39776e9a..c39ed97c8 100644 --- a/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java +++ b/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java @@ -16,7 +16,6 @@ package com.netflix.spectator.impl; import java.io.Serializable; -import java.util.Arrays; import java.util.Optional; /** @@ -30,7 +29,16 @@ */ public final class AsciiSet implements Serializable { - private static final long serialVersionUID = 1L; + private static final long serialVersionUID = 2L; + + /** Golden ratio. A small odd multiplier like 31 leaves a raw bit mask's structure intact. */ + private static final long HASH_MIXER = 0x9E3779B97F4A7C15L; + + private static void checkAscii(char c) { + if (c >= 128) { + throw new IllegalArgumentException("invalid pattern, '" + c + "' is not ascii"); + } + } /** * Create a set containing ascii characters using a simple pattern. The pattern is similar @@ -45,73 +53,90 @@ public final class AsciiSet implements Serializable { * Set containing the characters specified in {@code pattern}. */ public static AsciiSet fromPattern(String pattern) { - final boolean[] members = new boolean[128]; + long b0 = 0L; + long b1 = 0L; final int n = pattern.length(); for (int i = 0; i < n; ++i) { final char c = pattern.charAt(i); - if (c >= members.length) { - throw new IllegalArgumentException("invalid pattern, '" + c + "' is not ascii"); - } final boolean isStartOrEnd = i == 0 || i == n - 1; if (isStartOrEnd || c != '-') { - members[c] = true; + checkAscii(c); + if (c < 64) { + b0 |= 1L << c; + } else { + b1 |= 1L << c; + } } else { final char s = pattern.charAt(i - 1); final char e = pattern.charAt(i + 1); for (char v = s; v <= e; ++v) { - members[v] = true; + checkAscii(v); + if (v < 64) { + b0 |= 1L << v; + } else { + b1 |= 1L << v; + } } } } - return new AsciiSet(members); + return new AsciiSet(b0, b1); } /** Returns a set that matches no characters. */ public static AsciiSet none() { - final boolean[] members = new boolean[128]; - return new AsciiSet(members); + return new AsciiSet(0L, 0L); } /** Returns a set that matches all ascii characters. */ public static AsciiSet all() { - final boolean[] members = new boolean[128]; - Arrays.fill(members, true); - return new AsciiSet(members); + // -1L has every bit set, so both words match every character. + return new AsciiSet(-1L, -1L); } /** Returns a set that matches ascii control characters. */ public static AsciiSet control() { - final boolean[] members = new boolean[128]; - for (char c = 0; c < members.length; ++c) { - members[c] = Character.isISOControl(c); + long b0 = 0L; + long b1 = 0L; + for (char c = 0; c < 128; ++c) { + if (Character.isISOControl(c)) { + if (c < 64) { + b0 |= 1L << c; + } else { + b1 |= 1L << c; + } + } } - return new AsciiSet(members); + return new AsciiSet(b0, b1); } - /** - * Converts the members array to a pattern string. Used to provide a user friendly toString - * implementation for the set. - */ - private static String toPattern(boolean[] members) { + private static boolean memberBit(long b0, long b1, char c) { + final long word = (c < 64) ? b0 : b1; + // Relies on JLS 15.19: a long shift masks its distance to the low 6 bits, so `c` correctly + // selects the bit within whichever word was chosen even though c can be up to 127. Masking + // explicitly (`c & 0x3F`) is slower, since x86's shrx already masks the count in hardware. + return c < 128 && ((word >>> c) & 1L) != 0L; + } + + private String toPattern() { StringBuilder buf = new StringBuilder(); - if (members['-']) { + if (contains('-')) { buf.append('-'); } boolean previous = false; char s = 0; - for (int i = 0; i < members.length; ++i) { - if (members[i] && !previous) { + for (int i = 0; i < 128; ++i) { + final boolean member = contains((char) i); + if (member && !previous) { s = (char) i; - } else if (!members[i] && previous) { + } else if (!member && previous) { final char e = (char) (i - 1); append(buf, s, e); } - previous = members[i]; + previous = member; } if (previous) { - final char e = (char) (members.length - 1); - append(buf, s, e); + append(buf, s, (char) 127); } return buf.toString(); } @@ -124,12 +149,15 @@ private static void append(StringBuilder buf, char s, char e) { } } - private final String pattern; - private final boolean[] members; + /** Membership bits for characters 0-63. */ + private final long bits0; - private AsciiSet(boolean[] members) { - this.members = Preconditions.checkNotNull(members, "members array cannot be null"); - this.pattern = toPattern(members); + /** Membership bits for characters 64-127. */ + private final long bits1; + + private AsciiSet(long bits0, long bits1) { + this.bits0 = bits0; + this.bits1 = bits1; } /** @@ -137,7 +165,7 @@ private AsciiSet(boolean[] members) { * operation. */ public boolean contains(char c) { - return c < 128 && members[c]; + return memberBit(bits0, bits1, c); } /** @@ -146,7 +174,7 @@ public boolean contains(char c) { public boolean containsAll(CharSequence str) { final int n = str.length(); for (int i = 0; i < n; ++i) { - if (!contains(str.charAt(i))) { + if (!memberBit(bits0, bits1, str.charAt(i))) { return false; } } @@ -156,7 +184,7 @@ public boolean containsAll(CharSequence str) { private int indexOfNonMember(CharSequence str) { final int n = str.length(); for (int i = 0; i < n; ++i) { - if (!contains(str.charAt(i))) { + if (!memberBit(bits0, bits1, str.charAt(i))) { return i; } } @@ -168,7 +196,7 @@ private int indexOfNonMember(CharSequence str) { */ public String replaceNonMembers(String input, char replacement) { if (!contains(replacement)) { - throw new IllegalArgumentException(replacement + " is not a member of " + pattern); + throw new IllegalArgumentException(replacement + " is not a member of " + toString()); } int i = indexOfNonMember(input); return i < input.length() ? replaceNonMembersImpl(input, i, replacement) : input; @@ -179,11 +207,7 @@ public String replaceNonMembers(String input, char replacement) { * set that is provided. */ public AsciiSet union(AsciiSet set) { - final boolean[] unionMembers = new boolean[128]; - for (int i = 0; i < unionMembers.length; ++i) { - unionMembers[i] = members[i] || set.members[i]; - } - return new AsciiSet(unionMembers); + return new AsciiSet(bits0 | set.bits0, bits1 | set.bits1); } /** @@ -191,11 +215,7 @@ public AsciiSet union(AsciiSet set) { * set that is provided. */ public AsciiSet intersection(AsciiSet set) { - final boolean[] intersectionMembers = new boolean[128]; - for (int i = 0; i < intersectionMembers.length; ++i) { - intersectionMembers[i] = members[i] && set.members[i]; - } - return new AsciiSet(intersectionMembers); + return new AsciiSet(bits0 & set.bits0, bits1 & set.bits1); } /** @@ -203,32 +223,24 @@ public AsciiSet intersection(AsciiSet set) { * set that is provided. */ public AsciiSet diff(AsciiSet set) { - final boolean[] diffMembers = new boolean[128]; - for (int i = 0; i < diffMembers.length; ++i) { - diffMembers[i] = members[i] && !set.members[i]; - } - return new AsciiSet(diffMembers); + return new AsciiSet(bits0 & ~set.bits0, bits1 & ~set.bits1); } /** * Returns a new set that will match characters that are not included this set. */ public AsciiSet invert() { - final boolean[] invertMembers = new boolean[128]; - for (int i = 0; i < invertMembers.length; ++i) { - invertMembers[i] = !members[i]; - } - return new AsciiSet(invertMembers); + // Every bit maps to a real ascii character, so there are no padding bits to mask off. + return new AsciiSet(~bits0, ~bits1); } private String replaceNonMembersImpl(String input, int start, char replacement) { - final int n = input.length(); final char[] buf = input.toCharArray(); buf[start] = replacement; - for (int i = start + 1; i < n; ++i) { - final char c = input.charAt(i); - if (!contains(c)) + for (int i = start + 1; i < buf.length; ++i) { + if (!memberBit(bits0, bits1, input.charAt(i))) { buf[i] = replacement; + } } return new String(buf); } @@ -238,39 +250,35 @@ private String replaceNonMembersImpl(String input, int start, char replacement) * Otherwise return an empty optional. */ public Optional character() { - char c = 0; - int count = 0; - for (int i = 0; i < members.length; ++i) { - if (members[i]) { - c = (char) i; - ++count; - } + if (bits1 == 0L && Long.bitCount(bits0) == 1) { + return Optional.of((char) Long.numberOfTrailingZeros(bits0)); + } + if (bits0 == 0L && Long.bitCount(bits1) == 1) { + return Optional.of((char) (64 + Long.numberOfTrailingZeros(bits1))); } - return (count == 1) ? Optional.of(c) : Optional.empty(); + return Optional.empty(); } /** Returns true if this set is isEmpty. */ public boolean isEmpty() { - for (boolean b : members) { - if (b) { - return false; - } - } - return true; + return (bits0 | bits1) == 0L; } @Override public String toString() { - return pattern; + return toPattern(); } @Override public int hashCode() { - return pattern.hashCode() + 31 * Arrays.hashCode(members); + // Long.hashCode xors a value's halves and ~hi ^ ~lo == hi ^ lo, so folding the words + // directly would hash every set like its complement. Applied twice so bits1 mixes too. + final long h = (bits0 * HASH_MIXER + bits1) * HASH_MIXER; + return (int) (h ^ (h >>> 32)); } @Override public boolean equals(Object obj) { if (this == obj) return true; - if (obj == null || !(obj instanceof AsciiSet)) return false; + if (!(obj instanceof AsciiSet)) return false; AsciiSet other = (AsciiSet) obj; - return pattern.equals(other.pattern) && Arrays.equals(members, other.members); + return bits0 == other.bits0 && bits1 == other.bits1; } } diff --git a/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java b/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java index f5d3acab4..2cc773308 100644 --- a/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java +++ b/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java @@ -158,4 +158,19 @@ public void equalsContractTest() { .suppress(Warning.NULL_FIELDS) .verify(); } + + @Test + public void hashCodeDistinguishesComplementAndHalves() { + AsciiSet lower = AsciiSet.fromPattern("a-z"); + AsciiSet digits = AsciiSet.fromPattern("0-9"); + + Assertions.assertNotEquals(lower.hashCode(), lower.invert().hashCode()); + Assertions.assertNotEquals(digits.hashCode(), digits.invert().hashCode()); + Assertions.assertNotEquals(AsciiSet.none().hashCode(), AsciiSet.all().hashCode()); + + // Pairs 64 apart share a bit position across the two words. + Assertions.assertNotEquals(lower.hashCode(), AsciiSet.fromPattern("A-Z").hashCode()); + Assertions.assertNotEquals( + AsciiSet.fromPattern("!").hashCode(), AsciiSet.fromPattern("a").hashCode()); + } } From 36d6b2248fcf1047cc1744807fb078c30b43bafb Mon Sep 17 00:00:00 2001 From: Michael Braun Date: Thu, 17 Sep 2026 19:00:56 +0000 Subject: [PATCH 2/2] Fix two hashCode collisions and make the test exhaustive (bits0 * M + bits1) * M expands to bits0*M^2 + bits1*M, and M == 1 mod 4 makes 2^62*M^2 == 2^62*M mod 2^64, so a set holding only char 62 hashed like one holding only char 126. The 63/127 pair collided for a separate reason that no choice of constant fixes: 2^63 is a fixed point of multiplication by any odd value mod 2^64, so both words map to the same 2^63 and adding them into one accumulator cannot tell them apart. An xor-shift between the two multiplies fixes both. Over a corpus of all 128 singletons, every contiguous range of 8 lengths, and the complement of each (1968 distinct sets) it now gives 1968 distinct hashes, matching what main managed by hashing the whole boolean[128]. The test now walks all 128 single-character sets instead of asserting on chosen pairs. Which pairs collide depends on how the words are combined, so the previous fixed pair (33 vs 97) went green the moment that changed, while 62/126 and 63/127 broke. Verified the new test fails against the previous hashCode. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/netflix/spectator/impl/AsciiSet.java | 9 ++++--- .../netflix/spectator/impl/AsciiSetTest.java | 27 +++++++++++++------ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java b/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java index c39ed97c8..638ac3edf 100644 --- a/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java +++ b/spectator-api/src/main/java/com/netflix/spectator/impl/AsciiSet.java @@ -269,9 +269,12 @@ public boolean isEmpty() { } @Override public int hashCode() { - // Long.hashCode xors a value's halves and ~hi ^ ~lo == hi ^ lo, so folding the words - // directly would hash every set like its complement. Applied twice so bits1 mixes too. - final long h = (bits0 * HASH_MIXER + bits1) * HASH_MIXER; + // Folding the words straight through Long.hashCode would hash every set like its + // complement, since ~hi ^ ~lo == hi ^ lo. The xor-shift between them is load bearing too: + // 2^63 is a fixed point of odd multiplication, so char 63 would hash like char 127. + long h = bits0 * HASH_MIXER; + h ^= h >>> 32; + h = (h + bits1) * HASH_MIXER; return (int) (h ^ (h >>> 32)); } diff --git a/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java b/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java index 2cc773308..602ca960a 100644 --- a/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java +++ b/spectator-api/src/test/java/com/netflix/spectator/impl/AsciiSetTest.java @@ -20,6 +20,9 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import java.util.HashMap; +import java.util.Map; + public class AsciiSetTest { @Test @@ -160,17 +163,25 @@ public void equalsContractTest() { } @Test - public void hashCodeDistinguishesComplementAndHalves() { - AsciiSet lower = AsciiSet.fromPattern("a-z"); - AsciiSet digits = AsciiSet.fromPattern("0-9"); + public void hashCodeDistinctForEverySingleCharacter() { + // Exhaustive rather than a few chosen pairs: which pairs collide depends on how the two + // words are combined, so a fixed pair stops covering the defect as soon as that changes. + Map seen = new HashMap<>(); + for (char c = 0; c < 128; ++c) { + Character previous = seen.put(AsciiSet.fromPattern(String.valueOf(c)).hashCode(), c); + if (previous != null) { + Assertions.fail("chars " + (int) previous.charValue() + " and " + (int) c + + " have the same hashCode"); + } + } + } + @Test + public void hashCodeDistinguishesSetFromComplement() { + AsciiSet lower = AsciiSet.fromPattern("a-z"); Assertions.assertNotEquals(lower.hashCode(), lower.invert().hashCode()); - Assertions.assertNotEquals(digits.hashCode(), digits.invert().hashCode()); Assertions.assertNotEquals(AsciiSet.none().hashCode(), AsciiSet.all().hashCode()); - - // Pairs 64 apart share a bit position across the two words. - Assertions.assertNotEquals(lower.hashCode(), AsciiSet.fromPattern("A-Z").hashCode()); Assertions.assertNotEquals( - AsciiSet.fromPattern("!").hashCode(), AsciiSet.fromPattern("a").hashCode()); + lower.hashCode(), AsciiSet.fromPattern("A-Z").hashCode()); } }