Skip to content

investigation: repeated NewClassTree visits in Nullness checker #1345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zyf265600 wants to merge 7 commits into from
Closed

investigation: repeated NewClassTree visits in Nullness checker #1345

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac5e60f
perf: add tree processing cache to avoid redundant computations
zyf265600 Jul 18, 2025
6caaa38
Add fast-path in NullnessPropagationTreeAnnotator.visitNewClass: retu…
zyf265600 Aug 13, 2025
e8671e9
Merge branch 'master' into performance-optimize-001
zyf265600 Aug 13, 2025
da837a3
performance comparison
zyf265600 Sep 11, 2025
5bf54a6
remove redundant changes and files
zyf265600 Sep 11, 2025
588ed47
refine code with Formatter
zyf265600 Sep 29, 2025
4bb4e8d
Merge branch 'master' into performance-optimize-001
wmdietl Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions ...c/main/java/org/checkerframework/checker/nullness/NullnessNoInitAnnotatedTypeFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,8 +734,9 @@ public Void visitUnary(UnaryTree tree, AnnotatedTypeMirror type) {
// explicit nullable annotations are left intact for the visitor to inspect.
@Override
public Void visitNewClass(NewClassTree tree, AnnotatedTypeMirror type) {
// The constructor return type should already be NONNULL, so in most cases this will do
// nothing.
if (type.hasEffectiveAnnotation(NONNULL)) {
return null;
}
type.addMissingAnnotation(NONNULL);

@wmdietl wmdietl Sep 1, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addMissingAnnotation already checks whether the annotation is present, so this change doesn't achieve anything.

Do you actually see any performance differences?

@zyf265600 zyf265600 Sep 11, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed discussion.

I added a small perf harness to measure the effect of the extra hasEffectiveAnnotation(NONNULL) fast-path.

  • Test file: checker/jtreg/nullness/perf/NewClassPerf.java
    This generates a synthetic source with thousands of new expressions and compares two variants:

    • A = fast-path enabled (extra if)
    • B = fast-path disabled (-Dcf.skipNonnullFastPath=true)

  • How to run:

    ../jtreg/bin/jtreg \
  -workDir:checker/build/jtreg/perf/work \
  -reportDir:checker/build/jtreg/perf/report \
  -verbose:summary -samevm '-keywords:!ignore' \
  "-javacoptions:-classpath checker/dist/checker.jar" \
  "-vmoptions:-classpath checker/dist/checker.jar -Xms2g -Xmx2g \
    -Dperf.runs=10 -Dperf.groups=400" \
  -timeoutFactor:4 \
  checker/jtreg/nullness/perf/NewClassPerf.java

  • Results (10×400 runs, fixed 2 GB heap, JDK 17, macOS arm64):
    Across both AB and BA interleaved protocols, variant B was consistently as fast or slightly faster (≈2–3% median). In other words, the new if does not bring a measurable benefit, as you mentioned addMissingAnnotation already checks for existing annotations, so the extra condition only adds overhead.

Final conclusion: After all these investigations, it shows that the repeated visitNewClass calls are expected: multiple sub-checkers walk the same AST. This is not a defect, and at this point we don’t see a straightforward or sound optimization to reduce it. I’ll therefore retitle this PR as an investigation and propose to close it.

return null;
}
Expand All @@ -745,7 +746,9 @@ public Void visitNewClass(NewClassTree tree, AnnotatedTypeMirror type) {
@Override
public Void visitNewArray(NewArrayTree tree, AnnotatedTypeMirror type) {
super.visitNewArray(tree, type);
type.addMissingAnnotation(NONNULL);
if (!type.hasEffectiveAnnotation(NONNULL)) {
type.addMissingAnnotation(NONNULL);
}
return null;
}

Expand Down