Address severe performance regression in dataflow analysis #1328
Conversation
WalkthroughThis PR threads an invocation TreePath through generics-inference helpers and uses it when computing nullness for methods whose return type is a type variable in JSpecify mode — it locates and verifies an invocationPath before calling getGenericReturnNullnessAtInvocation. It adds a cache-tracking set alreadyRunAnalyses to DataFlow to avoid re-running identical analyses and clears that set in invalidateCaches. Tests were updated to reflect the changed inference behavior. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
1583-1623: Bug:getEnclosingTypeForCallExpressionignorespathfor implicitthis.When
methodSelectis an Identifier, use the providedpath(if non-null) to locate the correct enclosing class; relying onstate.getPath()can be wrong when the current leaf isn’t the invocation.Apply:
- if (methodSelect instanceof IdentifierTree) { + if (methodSelect instanceof IdentifierTree) { // implicit this parameter, or a super call. in either case, use the type of the enclosing // class. - ClassTree enclosingClassTree = - ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class); + TreePath basePath = (path != null) ? path : state.getPath(); + ClassTree enclosingClassTree = + ASTHelpers.findEnclosingNode(basePath, ClassTree.class); if (enclosingClassTree != null) { enclosingType = castToNonNull(ASTHelpers.getType(enclosingClassTree)); } } else if (methodSelect instanceof MemberSelectTree) {
🧹 Nitpick comments (6)
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (1)
173-183: Minor: source level check for diamond in anonymous class.
new Result<>() { ... }requires JDK 9+. If you need to keep source-8 compatibility, expand the type parameters explicitly. Otherwise, ignore.nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2727-2739: Prefer explicit type overvarhere (source-level compatibility).Use
TreePathexplicitly to avoid requiring JDK 10+.- var invocationPath = TreePath.getPath(path, invocationTree); + TreePath invocationPath = TreePath.getPath(path, invocationTree);What is the current
--release/source level for this module?nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (4)
1078-1096: Pass invocation path when available (consistency).You can pass
state.getPath()instead ofnullto improve robustness in nested contexts:- Type enclosingType = getEnclosingTypeForCallExpression(methodSymbol, tree, null, state, false); + Type enclosingType = getEnclosingTypeForCallExpression(methodSymbol, tree, state.getPath(), state, false);
1119-1126: Also pass path to inference call.- inferGenericMethodCallType( - state, - (MethodInvocationTree) currentActualParam, - null, + inferGenericMethodCallType( + state, + (MethodInvocationTree) currentActualParam, + state.getPath(), formalParameter, false, false);
1563-1570: Pass path when getting parameter nullness at invocation.- Type enclosingType = - getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, null, state, false); + Type enclosingType = + getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state.getPath(), state, false);
3-9: Unify Verify usage/imports (minor).Use either static import (
verify(...)) everywhere orVerify.verify(...), and drop the unused import accordingly.-import static com.google.common.base.Verify.verify; -import com.google.common.base.Verify; +import static com.google.common.base.Verify.verify;- Verify.verify(isGenericCallNeedingInference(invocationTree)); + verify(isGenericCallNeedingInference(invocationTree));- Verify.verify(tree instanceof NewClassTree); + verify(tree instanceof NewClassTree);Also applies to: 567-568, 1617-1619
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(3 hunks)nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java(4 hunks)nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(11 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/NullAway.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (3)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (1)
1274-1274: Tests updated to match path-aware inference; looks good.Switch to makeHelper() and the updated diagnostic for the parenthesized return align with the new call-site check.
Also applies to: 1302-1304
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
25-26: Verify import/usage is consistent.Static import for verify and usage in getSymbolForMethodInvocation are fine.
Also applies to: 449-463
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (1)
98-99: Run-once guard is correct; mutable transfer function state requires verification.The cache guard correctly prevents redundant
performAnalysis()on identicalAnalysisParamsand properly clears withinvalidateCaches(). However,AccessPathNullnessPropagationcontains a mutable, non-final fieldcodeAnnotationInfo(line 776) that is lazily initialized and cached within the transfer function instance. If transfer functions are reused across multiple analyses with differentVisitorStatecontexts, this field could retain stale values. Verify that:
- Transfer function instances are created fresh for each analysis call, OR
- The transfer function state is reset/cleared by
invalidateCaches(), OR- The mutable state doesn't affect correctness when reused
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1328 +/- ##
============================================
- Coverage 88.44% 88.43% -0.01%
- Complexity 2563 2566 +3
============================================
Files 96 96
Lines 8594 8605 +11
Branches 1705 1707 +2
============================================
+ Hits 7601 7610 +9
- Misses 499 500 +1
- Partials 494 495 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Main Branch: With This PR: |
| " Object x = new Object(); x.toString();", | ||
| " Object y = null;", | ||
| " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", | ||
| " // BUG: Diagnostic contains: passing @Nullable parameter 'y' where @NonNull is required", |
There was a problem hiding this comment.
This error message now makes sense at least if you know NullAway internals. Inference for the id(y) call fails, so we fall back on treating its parameter and return types as non-null. So, we get an error that we can't pass in y.
msridhar
changed the title
|
@jeffrey-easyesi any chance you could build a local snapshot with this change (pull the branch and run |
|
Yes - I've confirmed compiling our code with this branch takes about the same time as 0.12.10, and still has the same output as 0.12.11. |
Minor follow-up for #1328. When available, using `path` is more correct than `state.getPath()`.
The cache (introduced in #1328) was not size-bounded in the same way as the other caches in the `Dataflow` class, which could lead to very subtle bugs. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved internal caching mechanism for analysis tracking to enhance performance and memory efficiency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Fixes #1327 (I hope)
#1309 introduced a severe performance regression in dataflow analysis, as the result of running the dataflow analysis was no longer being cached. This caused a >100X slowdown in our dataflow micro-benchmark! (Which we really should have measured before landing #1309...)
This PR re-introduces the cache for analysis results (by tracking when we have already run the analysis). This fix exposed some other bugs in generic method inference where we weren't passing around the right
TreePathto enable discovery of appropriate assignment contexts and containing methods; we fix those bugs here too.Summary by CodeRabbit
New Features
Bug Fixes
Tests