Updated ImmutableSetBenchmark and ImmutableMapBenchmark to be more realistic - #12294
Updated ImmutableSetBenchmark and ImmutableMapBenchmark to be more realistic#12294dougqh wants to merge 7 commits into
ImmutableSetBenchmark and ImmutableMapBenchmark to be more realistic#12294Conversation
dougqh
added
comp: core
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
dougqh
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b55310a4a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The production StringIndex change appears sound. Four benchmark-method defects make the new performance tables and guidance unreliable.
🤖 Datadog Autotest · Commit b55310a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
The benchmark changes look generally reasonable, but I think a few of the existing PR comments are worth addressing -- particularly in regards to Java 8 results and re-working the benchmarks to avoid re-using the same objects when running / comparing.
Also I think the generated code comments can be simplified and made more readable, especially in the Benchmarking files. Telling Claude to keep things concise, avoid redundant and verbose comments, etc. helps.
bric3
changed the title
ImmutableSetBenchmark and ImmutableMapBenchmark to be more realistic
|
It looks good benchmark wise, albeit I reworded most of the javadoc, in a human form, removing the claude's inference verbiage. |
|
@sarahchen6 @bric3 — pushed the wording fixes from your review (all threads replied to and resolved). Ready for another look / approval when you have a chance. |
Mirrors StringIndex#contains for the static-arrays path, so callers of the embedded/parallel-array form don't need to spell out indexOf(...) >= 0 themselves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hmark Reusing the same interned/cached-hash key instances for both building a structure and measuring hit lookups against it understates real hit cost -- String#equals's == fast path and String#hashCode's cached result pay nothing extra for a key already touched during setUp. The new hitFresh scenario looks up separate, never-touched String instances instead, alongside the existing hit (interned literal) and miss (already representative) scenarios. Also pulls in BenchmarkUtils#polluteHashDispatch so the shared hashCode/equals call sites are already megamorphic before the StringSet arms measure their own lookups, matching production where those call sites are hit by every hash-based structure in the JVM. Replaced the javadoc's stale, partial-run tables/findings with a full hit/hitFresh/miss re-run across all six structures together (Fork(5), Threads(8)) -- confirms hitFresh is the slowest of the three scenarios for every hash-based structure, not merely slower than hit as previously guessed from partial data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Recompute the slot index each iteration as (h + probes) & mask instead of carrying a separately-incremented cursor, so the loop has one canonical induction variable for the JIT to reason about.
…ains-only helpers populateTypeProfile() drives only contains(), so it works against both mutable and immutable collections (including the collection instance under test itself); populateTypeProfileMutable() keeps the old add()+contains() behavior for callers that need add() dispatch polluted too.
The instance wrapper reliably beats HashSet on hit (its actual design case) but not on miss/hitFresh, where both are bimodal across forks and the mean already sits at or below HashSet's steady figure. Point miss/fresh-key-heavy callers at EmbeddingSupport directly instead of assuming the wrapper is strictly better than a plain Set.
Object.hashCode()/equals() are JVM-wide shared call sites hit by every hash-based structure in the process; leaving them monomorphic for a single-key-type run understates real dispatch cost, same reasoning as ImmutableSetBenchmark. Rerun and update the javadoc results table with pollution in effect -- StringIndex's get win over HashMap/TagMap/MapN holds up unconditionally here, unlike the access-pattern-dependent Set case.
Apply the reviewer-suggested rewordings for BenchmarkUtils, ImmutableSetBenchmark, ImmutableMapBenchmark, and StringIndex: trim narration and restate a couple of claims (fresh-string hash caching, contains() contract) more precisely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5242852 to
bc1a97a
Compare
3 participants
What Does This Do
Adds coverage of
StringIndextoImmutableSetBenchmarkandImmutableMapBenchmarkFixes a systemic error in benchmarking of
HashSet/HashMapby introducing type profile pollutionDemonstrating that the real performance of
HashSetis ~20% worse than previously indicated.Motivation
In a real system, HashSet, HashMap, etc are sharing among many different pieces of code.
That causes their methods to have dirty megamorphic profiles that aren't reflected in a simple benchmark.
BenchmarkingUtils aims to solve that by setting up those classes with more realistic profiles.
Additional Notes
EmbeddingSupport.contains(hashes, names, name)toStringIndex, mirroring the existing instanceStringIndex#contains, so callers of the static-arrays/embedded form don't need to spell outindexOf(...) >= 0themselves. Used byImmutableSetBenchmark'sstringIndex_embedded_*arms.ImmutableSetBenchmarkto add ahitFreshscenario alongside the existinghit/missscenarios:hitreuses the same interned literals used to build each structure (soString#equals's==fast path andString#hashCode's cached result pay nothing extra), whilehitFreshlooks up separate, never-touchedStringinstances to measure a real, uncached hit.BenchmarkUtils#polluteHashDispatch()so the JVM-sharedhashCode/equalscall sites are already megamorphic before each structure's own lookups are measured, matching production.hit/hitFresh/missre-run across all six structures together (@Fork(5),@Threads(8)), which corrected an earlier guess:hitFreshturns out to be the slowest of the three scenarios for every hash-based structure (not merely slower thanhit), because a miss usually short-circuits on the first hash mismatch while a fresh hit must probe to a match and pay a real, uncachedequals().StringIndex.EmbeddingSupport.put/indexOfaround a single induction variable (i = (h + probes) & maskrecomputed each iteration, instead of a separately-incremented cursor) for a more canonical loop shape. Investigated as a candidate fix forhitFresh's cross-fork bimodality; empirically it made no difference (root cause is a concurrent-warmup profile race governing whether C2 hoists the instancefinalfield loads, not loop shape), but the cleaner induction-variable form is kept on its own merits.ImmutableSetBenchmarkjavadoc's StringIndex-as-Set guidance: the instance wrapper reliably beatsHashSetonhit(its actual design case -- repeated lookups of a known, fixed name set), but onmiss/hitFreshit is bimodal across forks and its mean already sits at or belowHashSet's steady figure. Miss- or fresh-key-heavy callers should reach forEmbeddingSupportdirectly rather than assume the wrapper is strictly better than a plainSet. This doesn't apply to StringIndex's parallel-value (map) use case, covered next.BenchmarkUtils#polluteHashDispatchintopopulateTypeProfile(drives onlycontains(), so it works against the actual collection instance under test, mutable or immutable) andpopulateTypeProfileMutable(keeps the oldadd()+contains()behavior for callers that needadd()dispatch polluted too).polluteHashDispatch()treatment toImmutableMapBenchmark(previously missing it) and reruns the full suite. Refreshed javadoc results table, reorganized one row per collection / one column per benchmark arm. Unlike the Set case, StringIndex'sgetwin overHashMap/TagMap/Map.copyOf(MapN) holds up unconditionally here -- no bimodality, on both theequals()and identity-fast-path lookups.Test plan
./gradlew :internal-api:test --tests StringIndexTest./gradlew :internal-api:jmh -Pjmh.includes=ImmutableSetBenchmarkrun to completion and results folded into the javadoc./gradlew :internal-api:jmh -Pjmh.includes=ImmutableMapBenchmarkrun to completion and results folded into the javadoc./gradlew spotlessApply/spotlessCheck🤖 Generated with Claude Code