Skip to content

JS: Fix performance regression in the GetLaterAccess module.#12592

Merged
erik-krogh merged 4 commits into
github:mainfrom
erik-krogh:rhsRegress
Mar 22, 2023
Merged

JS: Fix performance regression in the GetLaterAccess module.#12592
erik-krogh merged 4 commits into
github:mainfrom
erik-krogh:rhsRegress

Conversation

@erik-krogh

@erik-krogh erik-krogh commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

Both motivated by a previous dist-upgrade, and github/codeql-action#1568 (which I think has the same issue).

1: Use SSA, that fixed the join-order.
2: Fix the join-order of the fix, because other cases started to regress.

It now performs well on all benchmarks I've tried (I don't have access to the database used in github/codeql-action#1568).
A MRVA run with the top 1000 projects finished in 25 minutes when running js/xss.

An evaluation was uneventful.

A previous attempt of doing the same thing can be found here: #12105

@github-actions github-actions Bot added the JS label Mar 20, 2023
@erik-krogh erik-krogh force-pushed the rhsRegress branch 2 times, most recently from e148214 to 02c0cd1 Compare March 21, 2023 10:43
@erik-krogh erik-krogh marked this pull request as ready for review March 21, 2023 14:23
@erik-krogh erik-krogh requested a review from a team as a code owner March 21, 2023 14:23
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Mar 21, 2023

@asgerf asgerf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One minor nit, otherwise LGTM. Though it would be nice if we could somehow verify that it actually fixes the observed problem

Comment thread javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll Outdated
Co-authored-by: Asger F <asgerf@github.com>
@erik-krogh erik-krogh requested a review from asgerf March 22, 2023 09:32
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.

2 participants