Skip to content

Bound size of alreadyRunAnalyses cache to be consistent#1334

Merged
msridhar merged 1 commit intomasterfrom
make-caching-consistent
Nov 7, 2025
Merged

Bound size of alreadyRunAnalyses cache to be consistent#1334
msridhar merged 1 commit intomasterfrom
make-caching-consistent

Conversation

@msridhar
Copy link
Copy Markdown
Collaborator

@msridhar msridhar commented Nov 7, 2025 •
edited by coderabbitai bot
Loading

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.

Summary by CodeRabbit

  • Refactor
    • Improved internal caching mechanism for analysis tracking to enhance performance and memory efficiency.

@msridhar msridhar requested a review from yuxincs November 7, 2025 21:36
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 7, 2025

Walkthrough

The DataFlow class was modified to replace its HashSet-based tracking mechanism with a Guava Cache for storing already-run analyses. The alreadyRunAnalyses field was converted from Set<AnalysisParams> to Cache<AnalysisParams, Boolean>, initialized with CacheBuilder configuration including a maximum size constraint. Associated logic in the dataflow() method now uses cache query operations instead of set containment checks, while invalidateCaches() performs cache invalidation rather than set clearing. Import statements were updated to reflect these changes.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: bounding the size of the alreadyRunAnalyses cache for consistency with other Dataflow caches.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch make-caching-consistent
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e75b94 and 83af926.

📒 Files selected for processing (1)
  • nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/dataflow/DataFlow.java
📚 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/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 windows-latest
  • GitHub Check: Build and test on ubuntu-latest
  • GitHub Check: Build spring-framework with snapshot
  • GitHub Check: Build caffeine with snapshot
  • GitHub Check: Build and test on macos-latest
🔇 Additional comments (3)
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (3)

101-102: LGTM! Cache size bound correctly applied.

The conversion from an unbounded Set to a bounded Cache with MAX_CACHE_SIZE (50) is correct and makes this cache consistent with analysisCache and cfgCache. Using Boolean as the value type is a standard pattern for implementing a bounded set with LRU eviction.

172-174: LGTM! Cache API usage is correct.

The cache query operations are properly implemented:

  • getIfPresent(aparams) == null correctly checks for cache miss
  • put(aparams, Boolean.TRUE) correctly adds the entry

Using the Boolean.TRUE constant is efficient as it avoids autoboxing.

325-325: LGTM! Cache invalidation is consistent.

Using invalidateAll() is correct and consistent with how the other caches (cfgCache and analysisCache) are cleared in this method.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 7, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.41%. Comparing base (3e75b94) to head (83af926).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff            @@
##             master    #1334   +/-   ##
=========================================
  Coverage     88.41%   88.41%           
  Complexity     2574     2574           
=========================================
  Files            97       97           
  Lines          8617     8618    +1     
  Branches       1713     1713           
=========================================
+ Hits           7619     7620    +1     
  Misses          501      501           
  Partials        497      497

☔ View full report in Codecov by Sentry.
📢 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.

@msridhar msridhar merged commit 2662df2 into master Nov 7, 2025
10 of 11 checks passed
@msridhar msridhar deleted the make-caching-consistent branch November 7, 2025 22:14
@msridhar msridhar mentioned this pull request Nov 25, 2025
Closed
@coderabbitai coderabbitai bot mentioned this pull request Nov 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuxincs yuxincs yuxincs approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@msridhar @yuxincs