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

Decommit memory as needed #79912

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Conversation

cshung
Copy link
Member

@cshung cshung commented Dec 22, 2022

This change makes sure:

  1. The mark array is decommitted when we decommit the region, and
  2. The region is decommitted if we fail to commit the corresponding mark array.

Fixes #79882

@cshung cshung self-assigned this Dec 22, 2022
@ghost
Copy link

ghost commented Dec 22, 2022

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 force-pushed the public/decommit-mark-array branch from 101c2a0 to 697d713 Compare December 23, 2022 00:03
@cshung cshung changed the title Decommit mark array when decommitting region Decommit memory as needed Dec 23, 2022
@cshung cshung marked this pull request as draft December 23, 2022 00:58
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
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.

also it would be nice to add a comment to delete_region to specify the invariant which is when you return a region to the global region allocator, memory should be decommitted (unless it's large pages) and mark array (if committed).

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jan 5, 2023
@cshung cshung force-pushed the public/decommit-mark-array branch from 2d2cea0 to 44450f1 Compare January 8, 2023 19:31
@PeterSolMS
Copy link
Contributor

Looks good to me.

Regarding Maoni's comment, I wonder if we can add an assert to delete_region to actually enforce the invariant. To make that work, decommit_region would need to set heap_segment_committed for the region, so we know its memory is decommitted.

@cshung cshung force-pushed the public/decommit-mark-array branch from 44450f1 to 3497400 Compare January 10, 2023 23:32
@cshung cshung marked this pull request as ready for review January 11, 2023 19:39
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.

LGTM!

@cshung cshung merged commit 6da8e2a into dotnet:main Jan 11, 2023
@cshung cshung deleted the public/decommit-mark-array branch January 11, 2023 23:25
@sebastienros
Copy link
Member

With these commits (2ca7cf7...6aaaaaa) aspnet apps crash at runtime in the benchmarks CI. I am flagging the PR which had the potential to explain it.

Crank command if you can run with and without this PR changes:

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/src/BenchmarksApps/Mvc/benchmarks.certapi.yml  --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --scenario NoMvcAuth --profile intel-win-app --profile intel-load2-load --profile intel-db-db --application.framework net8.0 --application.aspNetCoreVersion 8.0.0-alpha.1.23062.23 --application.runtimeVersion 8.0.0-alpha.1.23061.12 --application.sdkVersion 8.0.100-alpha.1.23061.8

eerhardt added a commit to eerhardt/runtime that referenced this pull request Jan 13, 2023
kasperk81 added a commit to kasperk81/runtime that referenced this pull request Jan 16, 2023
jkotas added a commit that referenced this pull request Jan 17, 2023
jkotas added a commit that referenced this pull request Jan 17, 2023
cshung added a commit to cshung/runtime that referenced this pull request Jan 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 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.

Array might not be zero-initialized
4 participants