Skip to content

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Feb 22, 2021

Evaluation on default/TaintedPath.ql.

Makes various predicates run faster by changing the join order.
I found them by running TaintedPath.ql on vscode, and then looking through the clause timing reports and tuple counts.

It also makes the globalVarRef predicate non-recursive.
So we no longer treat e.g. window.globalThis.global.window as being a reference to the global object.
(The performance benefit of that change is minor, so let me know if you want it reverted).

Summary of how how time is saved by each commit (TaintedPath.ql running on VSCode).

  • rearange ArrayIndexingStep to avoid #shared predicate - saves about 5s
  • cache AstNode::getParent - saves a few seconds
  • make globalVarRef non recursive - saves about 200ms
  • remove magic from inGuard - saves about 1-2 seconds
  • outline callee computation - to avoid many joins on getACall - saves about 300ms
  • change join order for summarizedHigherOrderCall - saves about 11s
  • change join order for SystemCommandExecutors - and use ApiGraphs::getACall - saves about 5s

@erik-krogh erik-krogh added WIP This is a work-in-progress, do not merge yet! Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Feb 22, 2021
@github-actions github-actions bot added the JS label Feb 22, 2021
@erik-krogh erik-krogh added no-change-note-required This PR does not need a change note and removed Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish WIP This is a work-in-progress, do not merge yet! labels Feb 23, 2021
@erik-krogh erik-krogh force-pushed the vsPerf branch 2 times, most recently from 80ce22e to 8599331 Compare February 23, 2021 09:06
@erik-krogh erik-krogh marked this pull request as ready for review February 23, 2021 11:54
@erik-krogh erik-krogh requested a review from a team as a code owner February 23, 2021 11:54
@asgerf
Copy link
Contributor

asgerf commented Feb 23, 2021

Nice!

Could you run evaluations of the whole security suite to make sure we aren't breaking performance somewhere else?

@erik-krogh
Copy link
Contributor Author

Nice!

Could you run evaluations of the whole security suite to make sure we aren't breaking performance somewhere else?

Looks OK: https://github.com/dsp-testing/erik-krogh-BCQS/tree/auto/data/workflow/vsperf-nigtly-validation/reports

@codeql-ci codeql-ci merged commit d2816b3 into github:main Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS 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.

4 participants