Skip to content
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

Rust/Swift: Cache Element.toString #18968

Merged
merged 2 commits into from
Mar 17, 2025
Merged

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Mar 11, 2025

Sadly, we cannot put the cached annotation directly on the existing toString predicate, as that introduces spurious non-monotonicity errors (the non-monotonicity checker considers cached predicates to be black-boxes). Instead, we add an intermediate toStringImpl predicate, which acts like the previous toString predicate, and then cache the new toString predicate, which simply forwards to toStringImpl.

DCA shows a nice average 7% analysis time speedup.

@github-actions github-actions bot added Rust Pull requests that update Rust code Swift labels Mar 11, 2025
@@ -4,7 +4,7 @@

module Impl {
class Decl extends Generated::Decl {
override string toString() { result = super.toString() }
override string toStringImpl() { result = super.toStringImpl() }

Check warning

Code scanning / CodeQL

Redundant override Warning

Redundant override of
this predicate
.
@hvitved hvitved force-pushed the rust/cache-to-string branch 3 times, most recently from 5ab442f to 03a87de Compare March 13, 2025 12:07
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Mar 13, 2025
@hvitved hvitved marked this pull request as ready for review March 13, 2025 19:00
@hvitved hvitved requested a review from a team as a code owner March 13, 2025 19:00
@hvitved hvitved changed the title Rust: Cache Element.toString Rust/Swift: Cache Element.toString Mar 13, 2025
@hvitved hvitved requested a review from redsun82 March 13, 2025 19:16
redsun82
redsun82 previously approved these changes Mar 14, 2025
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

This is nice, thanks!

@hvitved
Copy link
Contributor Author

hvitved commented Mar 14, 2025

Rebased to resolve merge conflict.

@hvitved hvitved requested a review from redsun82 March 14, 2025 11:38
@redsun82
Copy link
Contributor

Swift integration tests failure is a timeout flake, merging this now

@redsun82 redsun82 merged commit a2851f7 into github:main Mar 17, 2025
24 of 25 checks passed
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 Rust Pull requests that update Rust code Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants