Skip to content

finalize some performance-critical types - #2049

Merged
liblit merged 1 commit into
wala:masterfrom
liblit:perf/seal-or-finalize-various-types
Aug 27, 2026
Merged

finalize some performance-critical types#2049
liblit merged 1 commit into
wala:masterfrom
liblit:perf/seal-or-finalize-various-types

Conversation

@liblit

@liblit liblit commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Mark a number of heavily used fixpoint-iteration and int-set types as final where subclassing was never intended. This includes BitVectorVariable, BooleanVariable, machine states, points-to-set variables, etc.. Marking these hot types as final gives the JIT more freedom for devirtualization and inlining along the dataflow/fixpoint fast path. No behavioral change; the only externally visible effect is that the affected classes can no longer be subclassed.

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.2150 ms/op (99% CI [0.2029, 0.2272])
    • 26.7% faster, p = 0.006
  • SlicerBenchmark.computeBackwardSliceInTestList
    • baseline: 29.53 ms/op (99% CI [28.96, 30.09])
    • patched: 28.02 ms/op (99% CI [27.58, 28.46])
    • 5.1% faster, p < 0.001
  • AtomBenchmark.concatAtoms: 1.5% faster (p = 0.002)
  • AtomBenchmark.startsWith: 0.7% faster (p = 0.017)

The context-sensitive dataflow benchmark also improved by 8.7%, though that difference did not reach statistical significance with so few forks (p = 0.088). No other benchmark showed a statistically significant change 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 slicer improvement remains clearly significant (adjusted p ("q") = 0.002) and the small concatAtoms win is marginal (q = 0.085). The large dataflow improvement has q = 0.187, nominally above the conventional 0.05 cutoff because only 3-4 forks limit our statistical power, but its ~27% effect size and disjoint 99% confidence intervals leave little practical doubt that it is real.

@liblit

liblit commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Question for reviewers: are we confident that the affected types are not intended to be subtyped/subclassed by non-WALA code?

@liblit
liblit enabled auto-merge August 26, 2026 15:22
@msridhar

Copy link
Copy Markdown
Member

Interesting! I'm surprised this has such a big effect on some benchmarks in the steady state, once the JIT has warmed up. I assumed it would speculatively devirtualize / inline without making types final and the speculation checks wouldn't be too costly.

Do we think that sealing types yields an additional performance benefit beyond making types final?

Question for reviewers: are we confident that the affected types are not intended to be subtyped/subclassed by non-WALA code?

WALA leaves a lot of types open for subclassing, and I can't guarantee no one has tried to subclass these. The nice thing, though, is that these changes are very easy to revert if we get complaints.

@liblit

liblit commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Interesting! I'm surprised this has such a big effect on some benchmarks in the steady state, once the JIT has warmed up. I assumed it would speculatively devirtualize / inline without making types final and the speculation checks wouldn't be too costly.

Speculation guards are costly in two ways: the branch itself and lost opportunities for other optimizations. final eliminates the branch and also unlocks other JIT optimizations that might not be attempted if a branch were present.

Do we think that sealing types yields an additional performance benefit beyond making types final?

No. The JIT compiler does not leverage sealed as a performance hint. It's really just a static API contract. So my earlier claim that "Sealing these hot types gives the JIT more freedom for devirtualization and inlining along the dataflow/fixpoint fast path" was probably incorrect.

@msridhar

Copy link
Copy Markdown
Member

No. The JIT compiler does not leverage sealed as a performance hit. It's really just a static API contract. So my earlier claim that "Sealing these hot types gives the JIT more freedom for devirtualization and inlining along the dataflow/fixpoint fast path" was probably incorrect.

Ok, then, maybe we can remove the sealed / non-sealed changes from this PR? That limits flexibility to users, but doesn't seem to give a performance benefit. If we have a stronger justification for sealing those types later, we can do so.

(I would have thought sealed could help in a case where you're doing an invokeinterface and sealed shows there is only one implementation of the interface; but not sure.)

@liblit

liblit commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

maybe we can remove the sealed / non-sealed changes from this PR? That limits flexibility to users, but doesn't seem to give a performance benefit. If we have a stronger justification for sealing those types later, we can do so.

Agreed. Will do.

Mark a number of heavily used fixpoint-iteration and `int`-set types
as `final` where subclassing was never intended.  This includes
`BitVectorVariable`, `BooleanVariable`, machine states,
points-to-set variables, etc..  Marking these hot types as `final`
gives the JIT more freedom for devirtualization and inlining along
the dataflow/fixpoint fast path.  No behavioral change; the only
externally visible effect is that the affected classes can no longer
be subclassed.

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.2150 ms/op (99% CI [0.2029, 0.2272])
  * 26.7% faster, p = 0.006
* `SlicerBenchmark.computeBackwardSliceInTestList`
  * baseline: 29.53 ms/op (99% CI [28.96, 30.09])
  * patched: 28.02 ms/op (99% CI [27.58, 28.46])
  * 5.1% faster, p < 0.001
* `AtomBenchmark.concatAtoms`: 1.5% faster (p = 0.002)
* `AtomBenchmark.startsWith`: 0.7% faster (p = 0.017)

The context-sensitive dataflow benchmark also improved by 8.7%, though
that difference did not reach statistical significance with so few
forks (p = 0.088).  No other benchmark showed a statistically
significant change 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 slicer improvement
remains clearly significant (adjusted p ("q") = 0.002) and the small
`concatAtoms` win is marginal (q = 0.085).  The large dataflow
improvement has q = 0.187, nominally above the conventional 0.05
cutoff because only 3-4 forks limit our statistical power, but its
~27% effect size and disjoint 99% confidence intervals leave little
practical doubt that it is real.
@liblit
liblit force-pushed the perf/seal-or-finalize-various-types branch from 223fafe to eb92b0c Compare August 27, 2026 00:27
@liblit
liblit requested a review from msridhar August 27, 2026 00:27
@liblit liblit self-assigned this Aug 27, 2026
@liblit liblit changed the title seal or finalize some performance-critical types finalize some performance-critical types Aug 27, 2026
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 50.81%. Comparing base (3762b68) to head (eb92b0c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...a/org/scandroid/prefixtransfer/PrefixVariable.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2049      +/-   ##
============================================
- 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    36384       +2     
  Partials       4684     4684              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@liblit
liblit added this pull request to the merge queue Aug 27, 2026
Merged via the queue into wala:master with commit adaa50c Aug 27, 2026
11 checks passed
@liblit
liblit deleted the perf/seal-or-finalize-various-types branch August 27, 2026 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants