Sitelet https://web.archive.org/web/20260114124509/https://github.com/github/codeql/pull/5382
Skip to content

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Mar 11, 2021 •

I looked at which predicates are often used in the last stage, and then I cached some of them.

An evaluation shows ~4% improvement on the security suite.

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 11, 2021
@github-actions github-actions bot added the JS label Mar 11, 2021
@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 11, 2021
@erik-krogh erik-krogh marked this pull request as ready for review March 11, 2021 15:10
@erik-krogh erik-krogh requested a review from a team as a code owner March 11, 2021 15:10
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Mar 11, 2021
@asgerf
Copy link
Contributor

asgerf commented Mar 12, 2021

I'd feel a lot more confident about this if we had a way to measure cache size 😓 . hasLocationInfo is pretty big, but yeah, I've also been bothered by how much time it takes up.

@esbena
Copy link
Contributor

esbena commented Mar 12, 2021

I'd feel a lot more confident about this if we had a way to measure cache size

Why don't we just do that with du or something similarly hacky on the workers?

@erik-krogh
Copy link
Contributor Author

Why don't we just do that with du or something similarly hacky on the workers?

Is that something you're planning on? Because I think that sounds like a good idea.

@erik-krogh
Copy link
Contributor Author

The cache size grows from 17% of the DB size to 19% of the DB size according to Esbens evaluation.

So no dramatic increase in cache size.

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.

Thanks for the quick turnaround @esbena!

Let's do this 🚀

@codeql-ci codeql-ci merged commit ae62fbc into github:main Mar 16, 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