Skip to content
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

Respect scaling_reserved_ram feature flag #1760

Merged
merged 3 commits into from
Jul 10, 2023
Merged

Conversation

nickrolfe
Copy link
Contributor

The amount of RAM given to the CodeQL evaluator is the machine's total memory size, minus a reserved amount.

Currently, the reserved amount is fixed at 1 GB (or 1.5 GB on Windows). When the scaling_reserved_ram feature flag is enabled, we also add 2% of the total memory size to the reserved amount. This allows for the fact that the kernel will consume more RAM (e.g. for page tables) on machines with more physical RAM, so we should see fewer analyses getting killed by the OOM killer.

N.B. This is my first time working on the Action, or even writing TypeScript. I hope my uses of async/await make sense.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@nickrolfe nickrolfe requested a review from a team as a code owner July 7, 2023 13:54
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Looks good. It's perhaps a little cautious, but we might want to add an integration test that runs the CodeQL Action with this feature enabled. To do so, we could make a copy of pr-checks/checks/multi-language-autodetect.yml, enable the feature flag via the environment variable, and remove the language autodetection checks.

src/util.ts Outdated Show resolved Hide resolved
@henrymercer
Copy link
Contributor

Oh, do add a changelog note too! The one from 24th May is a good example.

nickrolfe added 2 commits July 7, 2023 16:46
The amount of RAM given to the CodeQL evaluator is the machine's total
memory size, minus a reserved amount. Currently, the reserved amount is
fixed at 1 GB (or 1.5 GB on Windows). When the scaling_reserved_ram
feature flag is enabled, we also add 2% of the total memory size to the
reserved amount. This allows for the fact that the kernel will consume
more RAM (e.g. for page tables) on machines with more physical RAM.
@nickrolfe nickrolfe force-pushed the nickrolfe/scaling-memory branch 2 times, most recently from e392b54 to 6b7e394 Compare July 7, 2023 15:57
@nickrolfe nickrolfe force-pushed the nickrolfe/scaling-memory branch from 6b7e394 to ab9aa50 Compare July 7, 2023 16:01
@nickrolfe
Copy link
Contributor Author

I see the new integration test is running with --ram=5769, while the multi-language autodetection test runs with --ram=5769, so I think it's working.

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Excellent! (I think you meant the multi-language autodetection test runs with --ram=5907.)

@nickrolfe
Copy link
Contributor Author

Whoops, yes, that's what I meant.

@nickrolfe nickrolfe merged commit 6a07b2a into main Jul 10, 2023
@nickrolfe nickrolfe deleted the nickrolfe/scaling-memory branch July 10, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants