Cache ExplicitCallGraph.ExplicitNode.hashCode - #2048
Merged
Conversation
The old implementation recomputed `getMethod().hashCode()` * 8681 +
`getContext().hashCode()` on every call. However, `CGNode` objects
are canonical per (method, context) pair, both fields are `final` and
never change, and `equals()` is identity-based. Therefore, caching
the hash in a `final` field is safe and avoids repeated (and
re-derived) hash computations. Every node is created through
`findOrCreateNode`, which rejects `null` methods, so eager computation
in the constructor cannot NPE.
Effect on JMH benchmarks (JDK 17; 3 forks x 6 one-second iterations
per configuration after warm-up; comparisons against `master` using
per-fork means as the unit of analysis, Welch's t-test, one-sided in
the improvement direction):
* `DataflowBenchmark.analyzeTestContextInsensitive`
* baseline: 0.2934 ms/op (99% CI [0.2792, 0.3077])
* patched: 0.2158 ms/op (99% CI [0.2027, 0.2288])
* 26.5% faster, p = 0.005
* `DataflowBenchmark.analyzeTestContextSensitive`
* baseline: 0.2233 ms/op (99% CI [0.2145, 0.2321])
* patched: 0.2136 ms/op (99% CI [0.1993, 0.2278])
* 4.4% faster, p = 0.046
No other benchmark showed a statistically significant difference in
either direction.
A note on multiple comparisons: we ran 19 hypothesis tests per branch,
each at the nominal alpha = 0.05 level. Even if this change had no
real effect anywhere, uncorrected testing would give roughly a 62%
chance of at least one spurious "significant" result (1 - 0.95^19), so
raw p-values overstate our confidence. Applying a Holm step-down
correction across those 19 tests, the context-insensitive improvement
has an adjusted p ("q") of 0.19: above the conventional 0.05 cutoff,
though its large ~27% effect size and disjoint 99% confidence
intervals leave little practical doubt that it is real. The
context-sensitive result does not survive any correction and should be
treated as suggestive rather than conclusive.
These gains are consistent with async-profiler data showing
`ExplicitNode.hashCode` as a significant fraction of dataflow-analysis
CPU; the same hash chain also feeds `LocalPointerKey.hashCode` during
pointer analysis.
No API change; backward compatible.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2048 +/- ##
============================================
- Coverage 50.81% 50.81% -0.01%
Complexity 12762 12762
============================================
Files 1368 1368
Lines 83498 83499 +1
Branches 14414 14414
============================================
- Hits 42432 42431 -1
- Misses 36382 36383 +1
- Partials 4684 4685 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The old implementation recomputed
getMethod().hashCode() * 8681 + getContext().hashCode()on every call. However,CGNodeobjects are canonical per (method, context) pair, both fields arefinaland never change, andequals()is identity-based. Therefore, caching the hash in afinalfield is safe and avoids repeated (and re-derived) hash computations. Every node is created throughfindOrCreateNode, which rejectsnullmethods, so eager computation in the constructor cannot NPE.Effect on JMH benchmarks (JDK 17; 3 forks × 6 one-second iterations per configuration after warm-up; comparisons against
masterusing per-fork means as the unit of analysis, Welch's t-test, one-sided in the improvement direction):DataflowBenchmark.analyzeTestContextInsensitiveDataflowBenchmark.analyzeTestContextSensitiveNo other benchmark showed a statistically significant difference in either direction.
A note on multiple comparisons: we ran 19 hypothesis tests per branch, each at the nominal alpha = 0.05 level. Even if this change had no real effect anywhere, uncorrected testing would give roughly a 62% chance of at least one spurious "significant" result (1 - 0.95^19), so raw p-values overstate our confidence. Applying a Holm step-down correction across those 19 tests, the context-insensitive improvement has an adjusted p ("q") of 0.19: above the conventional 0.05 cutoff, though its large ~27% effect size and disjoint 99% confidence intervals leave little practical doubt that it is real. The context-sensitive result does not survive any correction and should be treated as suggestive rather than conclusive.
These gains are consistent with async-profiler data showing
ExplicitNode.hashCodeas a significant fraction of dataflow-analysis CPU; the same hash chain also feedsLocalPointerKey.hashCodeduring pointer analysis.No API change; backward compatible.