Skip to content

JS: push more context into load/store steps from the exploratory flow-analysis #10986

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
Nov 1, 2022

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Oct 26, 2022

Saves an additional 15s (out of ~6 minutes) when running the code-scanning suite on the database from #10937

Evaluation looks somewhat uneventful.
I'm pretty sure the cache size is just noise.

@github-actions github-actions bot added the JS label Oct 26, 2022
@erik-krogh erik-krogh marked this pull request as ready for review October 26, 2022 13:12
@erik-krogh erik-krogh requested a review from a team as a code owner October 26, 2022 13:12
@esbena
Copy link
Contributor

esbena commented Oct 26, 2022

When the cache size is reported to be exactly '1.032' (as it is here), we suspect an unknown cache cleanup step to have happened. This has happened ~3 times in a few months.

@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Oct 26, 2022
@calumgrant calumgrant requested a review from asgerf October 31, 2022 09:28
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.

Implementation LGTM but the evaluation looks a bit underwhelming now that the other PR has been merged. It's leaning towards a slodown, with a 17s increase in vscode being the worst offender. Maybe that was just an unlucky run, but if it's really just trading a speedup in one project for a slowdown in another, I'm not sure we should merge it.

@erik-krogh
Copy link
Contributor Author

It's leaning towards a slowdown

I did a repeat evaluation: https://github.com/github/codeql-dca-main/tree/data/erik-krogh/pr-10986-21e7e2__default__code-scanning__1/reports
It shows much more neutral performance.

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.

Great, that looks more reasonable 👍

@erik-krogh erik-krogh merged commit ff2a5e8 into github:main Nov 1, 2022
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.

3 participants