Skip to content

JS: add isRelevant(succ) to flowStep predicate #3070

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 1 commit into from
Mar 17, 2020

Conversation

erik-krogh
Copy link
Contributor

While evaluating #3019 I found a weird performance regression in the sqlteaching benchmark that was most notable with the js/insecure-randomness query.
It ran about 3 times slower than on master.

Turns out that all I needed to trigger the performance regression was to add 3 edges to the exploratory flow (the exploratoryFlowStep predicate), corresponding to 3 calls to array.filter().
Adding those 3 edges caused the exploratory flow to find ~200.000 data-flow nodes, instead of 44, which caused the query to run 3 times slower.
I only needed to add the edges in the exploratory flow to trigger the regression, and nowhere else.

But most of the extra time was not spend in the exploratory flow, so it seemed there was room for improvement.

With this one line change most, but not all, of the performance was regained.
(Performance went from about 245 seconds to about 95 seconds)

And the change seems to improve performance across all benchmarks (big evaluation pending).

Here are two small evaluations on some benchmarks:
https://git.semmle.com/erik/dist-compare-reports/tree/profiling-js-max.northeurope.cloudapp.azure.com_1584361183161
https://git.semmle.com/erik/dist-compare-reports/tree/profiling-js-asger.northeurope.cloudapp.azure.com_1584356931002

@erik-krogh erik-krogh added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Mar 16, 2020
@erik-krogh erik-krogh requested a review from a team as a code owner March 16, 2020 12:36
@asgerf
Copy link
Contributor

asgerf commented Mar 16, 2020

Cool! I think the convention was that each kind of step checked isRelevant on its own, but I can see this has slipped in a few places.

@erik-krogh
Copy link
Contributor Author

Evaluation looks very good.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

💪

@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 17, 2020
@semmle-qlci semmle-qlci merged commit 8792d0d into github:master Mar 17, 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