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

Private API for RefreshMemoryLimit #83707

Merged
merged 5 commits into from
Apr 19, 2023

Conversation

cshung
Copy link
Member

@cshung cshung commented Mar 21, 2023

This PR implements #70601 as a private method, we wanted to do that as a private method and get it merged to facilitate testing on a broader scale while not committing to a particular API shape for everyone just yet. For scoping reasons, this is currently only tested for Windows x64, it should work on Windows x86 with a caveat that we will not establish a heap hard limit if we don't already have one. Any other platforms are not tested yet.

Before this change, we detect the physical memory (as well as the additional limits such as job objects) and configure the GC, and then we never change it. Some of these configurations are just some numeric fields on the gc_heap class, while some others are structural (e.g. number of heaps, region_range, etc ...).

This change aims at providing some flexibility on top of that init-once theme above. In particular, we allow the memory limit to change while the process is still running. It would be difficult to make structural changes, but it is relatively easy to just alter some numerical values. This change does just that. It factors out the memory-related settings into a separate function and allows them to be re-computed when the memory limit changes and the GC is notified through the RefreshMemoryLimit API call.

The technical challenge is commit accounting. If we happen to have no job object applied on the process and then one is applied later, now we have to set the heap_hard_limit, but then how would we know how much we already committed? The bulk of the refresh_memory_limit handles that by walking through the gc_heap data structure and figuring that out.

To make sure that is correct, a debug-only mode COMMITTED_BYTES_SHADOW is introduced. This mode will maintain the committed bytes all the time regardless of the existence of the heap_hard_limit, then we can assert the reconstructed value through the heap walking is producing the correct value as if we were tracking it all the time. This committed byte reconstruction and the testing knob will also be useful for other heap restructuring purposes.

@ghost
Copy link

ghost commented Mar 21, 2023

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr

Milestone: -

@cshung cshung marked this pull request as draft March 21, 2023 03:08
@cshung cshung changed the title RefreshMemoryLimit [WIP] RefreshMemoryLimit Mar 21, 2023
@cshung cshung force-pushed the public/refresh-memory-limit branch from 16fa862 to dabc733 Compare March 21, 2023 16:03
@cshung cshung force-pushed the public/refresh-memory-limit branch from f1157dd to e14399a Compare March 30, 2023 17:51
@cshung cshung changed the title [WIP] RefreshMemoryLimit Private API for RefreshMemoryLimit Mar 30, 2023
@cshung cshung marked this pull request as ready for review March 30, 2023 18:13
@cshung cshung force-pushed the public/refresh-memory-limit branch from e14399a to a69812c Compare March 30, 2023 20:01
@dotnet dotnet deleted a comment from dotnet-issue-labeler bot Mar 30, 2023
src/coreclr/gc/gcpriv.h Outdated Show resolved Hide resolved
src/coreclr/gc/gcpriv.h Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
@cshung cshung force-pushed the public/refresh-memory-limit branch 2 times, most recently from 2204863 to da4609e Compare April 10, 2023 17:40
@cshung cshung force-pushed the public/refresh-memory-limit branch 3 times, most recently from c47c5b8 to c365c40 Compare April 15, 2023 07:25
@cshung
Copy link
Member Author

cshung commented Apr 16, 2023

Here is the validation done for the PR.

  • GC Perf Sim
    • Normal Server
    • Normal Workstation
    • Large Pages Server
    • Large Pages Workstation
    • Low Memory Container
    • High Memory Load
  • Microbenchmarks
    • V8 Test
    • Top 20 Microbenchmarks
  • ASPNet Benchmarks
    • JsonMin - Windows
    • JsonMin - Linux
    • Fortunes ETF - Windows
    • Fortunes ETF - Linux

Copy link
Contributor

@PeterSolMS PeterSolMS 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 to me.

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

other than the last couple of comments, this LGTM!

@cshung cshung merged commit bb85812 into dotnet:main Apr 19, 2023
@cshung cshung deleted the public/refresh-memory-limit branch April 19, 2023 04:18
@cshung cshung added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants