Skip to content

Guards: Cache nullGuard predicate. #20237

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.

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

Merged
merged 1 commit into from
Aug 19, 2025

Conversation

aschackmull
Copy link
Contributor

A recent QA run highlighted a performance regression on a single repo: total analysis time went from 3mins to 41mins. I tracked down the cause to a poor join-order in a non-recursive instance of the forex in the wrapperGuard predicate. The predicate is actually recursive, but was being recomputed in a non-recursive setting since its recursion goes through cached dependencies but was (transitively) used in the non-cached nullGuard predicate, which is publicly exposed.

This PR fixes the problem by including the nullGuard predicate in the set of cached predicates in the Guards library.

I could reproduce the performance problem locally and verified that this PR fixed the issue.

Ignore white-space when reviewing to minimize the diff.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Aug 18, 2025
@Copilot Copilot AI review requested due to automatic review settings August 18, 2025 09:17
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a performance regression where analysis time increased from 3 minutes to 41 minutes by caching the nullGuard predicate. The issue was caused by the nullGuard predicate being used in a non-cached context while transitively depending on cached predicates, leading to poor join-order optimization.

  • Moved the ImpliesTC module and nullGuard predicate into a new Cached module
  • Added cached annotation to the nullGuard predicate
  • Created a public alias for the cached nullGuard predicate

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@aschackmull
Copy link
Contributor Author

dca is uneventful

@aschackmull aschackmull merged commit a8f394f into github:main Aug 19, 2025
38 checks passed
@aschackmull aschackmull deleted the guards/nullguard-caching branch August 19, 2025 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants