Repository navigation
Store AsciiSet membership as two longs instead of boolean[128] - #1298
Conversation
f1e4ef0 to
1012de6
Compare
|
I like to keep PRs small and focused. Can we split the normalizeTags change to a separate PR? |
1012de6 to
ddcd4ad
Compare
|
|
||
| @Override public int hashCode() { | ||
| return pattern.hashCode() + 31 * Arrays.hashCode(members); | ||
| return 31 * Long.hashCode(bits0) + Long.hashCode(bits1); |
There was a problem hiding this comment.
Long.hashCode(x) == Long.hashCode(~x) for all x, so 31*Long.hashCode(bits0) + Long.hashCode(bits1)
is complement-invariant. Measured:
- every set collides with its own complement: 258/258 (was 0/258)
- all 26/26 lower/upper letter pairs collide;
a-zandA-Zboth hash to134217726 none(),all(), andfromPattern("")all hash to 0- distinct hashes over a 258-set corpus: 258 -> 127
equals is exact so nothing is wrong, but this propagates through CharClassMatcher.hashCode
into PatternMatcher / Query.Regex and Optimizer.dedupOr's hash set.
Fix: mix before folding.
long h = bits0 * 0x9E3779B97F4A7C15L + bits1;
return (int) (h ^ (h >>> 32));There was a problem hiding this comment.
Confirmed, and it holds for the reason you gave: Long.hashCode is (int)(v ^ (v >>> 32)), i.e. lo ^ hi, and ~lo ^ ~hi == lo ^ hi, so complementing both words is invisible to it. 200k random longs, zero counterexamples.
One thing to flag on the suggested fix though — mixing only bits0 leaves half of it in place. Any set confined to chars 64-127 has bits0 == 0, so bits0 * K + bits1 degenerates to Long.hashCode(bits1), and that fold is symmetric between the halves. a-z and A-Z still collide at 134217726 under it.
Mixing both words with distinct odd constants fixes both kinds of pair. Over a 306-set corpus (every single char, a spread of ranges, both halves, and the complement of each):
| distinct | set == its complement | a-z == A-Z |
|
|---|---|---|---|
| current | 84/306 | 153/153 | yes |
bits0 * K + bits1 |
250/306 | 0/153 | yes |
bits0 * K1 + bits1 * K2 |
300/306 | 0/153 | no |
Went with the last one. A murmur3 finalizer on top scored the same (298/306), so it didn't seem worth the extra lines.
final long h = bits0 * 0x9E3779B97F4A7C15L + bits1 * 0xC2B2AE3D27D4EB4FL;
return (int) (h ^ (h >>> 32));Added a test covering set-vs-complement and the half-swap pair so this can't quietly regress again.
There was a problem hiding this comment.
Both confirmed, and thanks — the 62/126 derivation is exactly right. 0x9E3779B97F4A7C15 ends in 0x15 = 21 ≡ 1 mod 4, so M - 1 = 4k and 2^62·M·(M-1) = 2^64·M·k ≡ 0.
My validation is what let this through: I scored candidates on a distinct-hash count and never printed the colliding pairs, so "732/740 distinct" looked fine while hiding exactly this. Enumerating instead of counting immediately surfaced it.
On the suggested fix — 31 * hi32(bits0*M) + hi32(bits1*M) — it clears 62/126 but is worse overall, 71 collisions across a 1968-set corpus against 12 for what I had. And 63/127 survives it, for a reason no constant choice fixes: 2^63 is a fixed point of multiplication by any odd value mod 2^64, so {63} (bits0 = 2^63) and {127} (bits1 = 2^63) both produce 2^63 and summing them into one accumulator cannot separate them. The same thing happens to the 32-bit form, since 31 * 2^31 ≡ 2^31 mod 2^32. Every candidate I tried that was "each word times an odd constant, then add" collided on 63/127, including two distinct constants and a murmur3 finalizer.
An xor-shift between the two multiplies breaks the fixed point and keeps one constant:
long h = bits0 * HASH_MIXER;
h ^= h >>> 32;
h = (h + bits1) * HASH_MIXER;
return (int) (h ^ (h >>> 32));Corpus of all 128 singletons, every contiguous range at 8 lengths, and the complement of each:
| singleton collisions | corpus collisions | |
|---|---|---|
(bits0*M + bits1)*M |
[62,126], [63,127] | 12 |
| two distinct constants | [63,127] | 14 |
31 * hi32 |
[63,127] | 71 |
| murmur3 finalizer | [63,127] | 15 |
| xor-shift between | none | 0 |
1968 distinct hashes for 1968 distinct sets, so it matches the dispersion main got from hashing the whole boolean[128].
| * new id when a character actually needs to be replaced; an id that is already valid is | ||
| * returned as is. | ||
| */ | ||
| @Override protected Id normalizeTags(Id id) { |
There was a problem hiding this comment.
normalizeTags is now too big to inline — on the hot path this PR set out to speed up
javap -c on both revisions: 194 -> 327 bytecodes, and FreqInlineSize = 325 on the JDK 25
this repo benchmarks with. It crosses by 2 bytes, so AbstractRegistry.normalizeId no longer
inlines it on any path, including the all-clean early return.
The duplicated else rebuild loop is ~30 of those bytecodes — deleting it gets back under, and a
split-range loop ([1, firstDirty) copy, [firstDirty, size) rebuild) is behaviorally identical
with no inner branch at all.
| */ | ||
| private final Id dirtyMiddleTag = registry.createId("test") | ||
| .withTag("a", "1").withTag("b", "2").withTag("path", "/api/v1") | ||
| .withTag("d", "4").withTag("e", "5"); |
There was a problem hiding this comment.
ArrayTagSet sorts by key, so path sorts last in both benchmark ids. Printed:
dirtyMiddleTag actual order: test:a=1:b=2:d=4:e=5:path=/api/v1 size=6 firstDirtyTag=5
dirtyLastTag actual order: test:a=1:b=2:c=3:d=4:path=/api/v1 size=6 firstDirtyTag=5
So "one dirty tag, mid-list +29%" and "one dirty tag, last +28-39%" are one case. The genuine
middle case — a clean tag sorting after the dirty one, i.e. the new
i >= firstDirtyTag-but-actually-clean branch — is unmeasured and untested. Use a dirty key
that sorts middle, e.g. b=/api/v1.
| * Cached, lazily computed pattern for {@link #toString()}; excluded from equals/hashCode and | ||
| * from serialization since it's fully derived from {@link #bits0}/{@link #bits1}. | ||
| */ | ||
| private transient String pattern; |
There was a problem hiding this comment.
toString isn't on the hot path, it is used for debugging and such. I think it might be cleaner to just remove this member variable and generate the pattern when needed.
There was a problem hiding this comment.
Agreed, removed. toString is only reached from CharClassMatcher.toString and the replaceNonMembers argument-validation message, so the cache was buying a mutable field and a safe-publication argument for a debug path.
Worth noting the win here wasn't from the cache: it was from getting toPattern out of the constructor, which the old code called eagerly on every AsciiSet. That's why the set-algebra operations were so slow before — union was paying for a StringBuilder walk over 128 chars per call. That part is unchanged, so the numbers hold.
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) <noreply@anthropic.com>
7691de5 to
5b96e61
Compare
brharrington
changed the title
(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) <noreply@anthropic.com>
michaelbraun
changed the title
2 participants
Summary
Replaces
AsciiSet'sboolean[128]membership array with a pair oflongbitmasks. Whole-setoperations drop from an O(128) loop to a couple of ALU ops, and
contains/containsAllgo from abounds-checked array load to a shift and mask.
Also gets
toPatternout of the constructor. The old code built the pattern string eagerly forevery instance, which is what made the set algebra slow —
unionwas paying for aStringBuilderwalk over 128 characters per call.
toStringderives it on demand instead.Numbers
JMH
thrpton JDK 25, x86-64. Each row is two jmh jars built from two git states ofAsciiSet.java, running the same benchmark.AsciiSetBenchat 5 forks × 12 iterations(60 samples);
lookupCostat 12 forks (144 samples), because it needs them — see the note below.Set algebra and construction — these are where the representation change pays off. They run
while a pattern matcher is being compiled, not per lookup.
unioncharacter()isEmptyequalsfromPatternScanning — this is what runs on every meter lookup, via
AtlasRegistry.normalizeTags.contains(single char)containsAll, short valid valuecontainsAll, long valid namecontainsAll, invalid charreplaceNonMembers, nothing to fixreplaceNonMembers, fix near startreplaceNonMembers, fix near endCounterIncrement.lookupCost(real meter lookup, end to end)The three regressions are real, not noise: they reproduced at the same magnitude across
independent runs at 2 and 5 forks, with non-overlapping error bars. They are the paths that scan
past an invalid character, where the bitset's word-select branch costs more than the old array's
L1 load, and the array is small enough to stay resident. The end-to-end lookup is still net
positive because the dominant case is a clean value that either scans to the end or exits on the
first character.
On the fork count:
lookupCostreported −5.1% at 2 forks and +6.1% at 6, each with ±0.2%error bars. JMH's interval only covers variance within a run, so with few forks it is precise and
wrong — the per-fork code layout is the thing that moves. Three runs at ≥6 forks agree on +5.4%
to +6.1%.
Notes
serialVersionUIDgoes to 2. Serialization matches by field name, so an old stream wouldotherwise deserialize into a silently empty set.
hashCodemultiplies before folding.Long.hashCodexors a value's halves together and~hi ^ ~lo == hi ^ lo, so folding the two words through it directly would give every set the samehash as its complement — and a char class and its negation are a common pair in a compiled
matcher, so they would collide in
Optimizer.dedupOr's hash set. The xor-shift between the twomultiplies is load bearing as well: 2^63 is a fixed point of multiplication by any odd value mod
2^64, so without it a set holding only char 63 hashes like one holding only char 127. Over a
corpus of all 128 singletons, every contiguous range at 8 lengths, and the complement of each,
this gives 1968 distinct hashes for 1968 distinct sets.
The accompanying test walks all 128 single-character sets rather than asserting on chosen pairs.
Which pairs collide is a function of how the two words are combined, so a fixed pair silently
stops covering the defect as soon as that changes — which is exactly what happened to an earlier
revision of this PR.
memberBitrelies on JLS 15.19 implicit shift-distance masking rather than an explicitc & 0x3F: x86'sshrxalready masks the count in hardware, so the explicit form compiles to aredundant
andlper character. That one is easy to reintroduce, so it is called out in a comment.The
AtlasRegistry.normalizeTagschange that was originally part of this PR is split out perreview, and will follow separately.
🤖 Generated with Claude Code