Skip to content

C++: Fix data-flow dispatch perf with globals 1.24 #3650

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

Closed
wants to merge 1 commit into from

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Jun 9, 2020

This is a 1.24 backport of #3637.

Do not merge without the approval of the release manager, @alexet.

@jbj jbj added the C++ label Jun 9, 2020
@jbj jbj added this to the 1.24 milestone Jun 9, 2020
@geoffw0
Copy link
Contributor

geoffw0 commented Jun 9, 2020

As extra checking, I tested this with two IR queries (RedundantNullCheckSimple.ql to warm up, then CgXss.ql) on a wxWidgets snapshot. Performance was similar (249s -> 244s and 250s -> 250s). DataFlowDispatch::VirtualDispatch::DataSensitiveCall::flowsFrom has the same number of tuples (447615) in the latter query.

I've also confirmed the speedup on kamailio with the same setup. CgXss.ql speeds up significantly (2058s -> 1425s) and the flowsFrom row count matches (304194). The number of actual results is also unchanged across all of these runs (in most cases there were 0 results).

👍

@alexet
Copy link
Contributor

alexet commented Jun 15, 2020

Is it likely that customers will run into this or is this a weird case with a specific database?

@jbj
Copy link
Contributor Author

jbj commented Jun 15, 2020

The problem wasn't visible on our 11 test projects in CPP-Differences.

@jbj
Copy link
Contributor Author

jbj commented Jun 15, 2020

It became visible when deploying to lgtm.com, but even there it didn't cause timeouts.

@jbj
Copy link
Contributor Author

jbj commented Jun 16, 2020

We should perhaps let this PR wait until #3637 has been deployed to lgtm.com in the next dist upgrade. Then we'll better be able to understand the risk.

@jbj
Copy link
Contributor Author

jbj commented Aug 13, 2020

Given that no customer has complained about this performance issue and that 1.25 is getting close, I'll close this PR.

@jbj jbj closed this Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants