Skip to content

C++: Performance fix for cpp/cleartext-storage-buffer #11596

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
Dec 8, 2022

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Dec 7, 2022

Fix a performance issue with cpp/cleartext-storage-buffer, that was causing it to fail on several LGTM projects. The issue was that one of the constraints on the sink (that the written buffer is a SensitiveExpr) was only expressed in the query, not in the dataflow configuration. The solution is to define the sources properly. Following that and a bit more cleanup, I also believe the result is more readable than what we had before.

Improves performance on mruby_mruby locally from ~750s to ~45s.

I will also do a DCA run on this pull request.

@geoffw0 geoffw0 added the C++ label Dec 7, 2022
@geoffw0 geoffw0 requested a review from a team as a code owner December 7, 2022 15:42
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.

One comment. Otherwise, this LGTM once DCA is happy.

Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>
@geoffw0 geoffw0 added the no-change-note-required This PR does not need a change note label Dec 7, 2022
@geoffw0
Copy link
Contributor Author

geoffw0 commented Dec 8, 2022

DCA LGTM - a good number of speedups for cleartext-storage-buffer in the stage timings!

@geoffw0 geoffw0 merged commit f373b7f into github:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ 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