Skip to content

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 AI review requested due to automatic review settings August 18, 2025 09:17
Copy link
Contributor

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