Skip to content

Swift: defaultImplicitTaintRead performance improvement #14386

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 3 commits into from
Oct 9, 2023

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Oct 5, 2023

Performance Improvement motivated by a discovery on a nightly run.

Despite the bindingset[node] annotation, the evaluator was attempting to bind node using the internal logic of the predicate before applying the externally derived binding (which comes from TaintTrackingImpl.qll's allowImplicitRead), causing an explosion of computation. Forcing the issue with pragma[only_bind_out] appears to entirely solve the issue.

Locally I'm seeing something like a 30s improvement on multiple queries. A DCA run should confirm...

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Oct 5, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner October 5, 2023 17:49
@MathiasVP
Copy link
Contributor

Despite the bindingset[node] annotation, the evaluator was attempting to bind node using the internal logic of the predicate before applying the externally derived binding (which comes from TaintTrackingImpl.qll's allowImplicitRead)

It sounds like the right fix for this is to also add pragma[inline_late] to the predicate. If so, I think that's a solution that's much easier to understand: it tells the compiler (and the programmer) that node should be bound before evaluating the predicate.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Oct 6, 2023

Locally:

  • using neither solution is bad
  • pragma[only_bind_out] only is good
  • pragma[inline_late] alongside pragma[only_bind_out] is good
  • pragma[inline_late] only is good

I can't discriminate based on performance, so I've switched to just inline_late.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM once DCA comes back happy!

@geoffw0
Copy link
Contributor Author

geoffw0 commented Oct 9, 2023

DCA shows a 20% analysis time reduction (1720s to 1376s), which is exactly what I was hoping for / cautiously expecting! 🎉

(we're also seeing "database run-queries" wobbles, but that's been true in recently nightly DCA runs as well - I've created a discussion on one of those)

@geoffw0 geoffw0 merged commit 57e32b4 into github:main Oct 9, 2023
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 Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants