region memory limit by Maoni0 · Pull Request #48691 · dotnet/runtime

Maoni0 · 2021-02-24T08:15:54Z

implemented the logic to handle when we are really running out of memory.
We need to be resilient to it during a GC. We might need an empty region
per heap so we try to get it up front and if we can't get it and find out
that we do need it, we resort to a special sweep mode.
also take virtual memory load into consideration
fixed the logic to decide whether we should compact based on space.
fixed various places when we can't get a region, we need to make sure we
don't thread 0 into the region list.
made allocated_since_last_gc per more_space_lock otherwise we are not doing
the accounting correctly
moved the pinning for STRESS_REGIONS into a blocking GC otherwise we can have
AV from BGC trying to mark the pinned object while it's being constructed
got rid of the places that use the hard coded REGION_SIZE
included one perf change in reset_memory

ghost · 2021-02-24T08:16:04Z

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

Issue Details

implemented the logic to handle when we are really running out of memory.
We need to be resilient to it during a GC. We might need an empty region
per heap so we try to get it up front and if we can't get it and find out
that we do need it, we resort to a special sweep mode.
also take virtual memory load into consideration
fixed the logic to decide whether we should compact based on space.
fixed various places when we can't get a region, we need to make sure we
don't thread 0 into the region list.
made allocated_since_last_gc per more_space_lock otherwise we are not doing
the accounting correctly
moved the pinning for STRESS_REGIONS into a blocking GC otherwise we can have
AV from BGC trying to mark the pinned object while it's being constructed
got rid of the places that use the hard coded REGION_SIZE
included one perf change in reset_memory

Author:	Maoni0
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

src/coreclr/gc/gc.cpp

mangod9 · 2021-02-24T18:01:59Z

src/coreclr/gc/gc.cpp

should this just be < ?

yes, will change it.

PeterSolMS · 2021-02-25T09:08:15Z

src/coreclr/gc/gc.cpp

Should the dprintf level be REGIONS_LOG here?

I will change all of the dprintf's in region_allocator to REGIONS_LOG.

PeterSolMS · 2021-02-25T09:14:47Z

src/coreclr/gc/gc.cpp

Use dprintf level REGIONS_LOG here as well?

PeterSolMS · 2021-02-25T09:22:32Z

src/coreclr/gc/gc.cpp

How about the case where init_table_for_region fails because the mark array cannot be committed? It seems that we would return true in this case, but likely still fail when we later try to get the region.

good catch! I did check for this (I set region to 0 in init_table_for_region when we can't commit) but I made a mistake when I refactored it out to its own method. should have init_table_for_region return false if we can't commit.

PeterSolMS · 2021-02-25T09:24:19Z

src/coreclr/gc/gc.cpp

Would this AV if commit_mark_array_new_seg failed above?

it wouldn't - right now the GC bookkeeping datastructure is completely committed for the region's range except for the mark array part.

but above, you set region to 0, so as heap_segment_mem dereferences it, you would AV before you even get to the brick_of part.

oh I misunderstood your intention for the question...absolutely, I shouldn't call this when the region is already set to 0

PeterSolMS · 2021-02-25T09:24:33Z

src/coreclr/gc/gc.cpp

Maoni0 · 2021-02-25T09:28:34Z

I found a problem with aging handles for the special sweep case. will fix it later today.

PeterSolMS · 2021-02-25T09:36:08Z

src/coreclr/gc/gcpriv.h

It looks like this could be a local variable instead of a field.

I specifically wanted this to be a global so it introduces some variation instead of always pinning the 1st, 3rd, etc gen0 region. but I changed it to size_t instead of int.

PeterSolMS · 2021-02-25T09:37:07Z

src/coreclr/gc/gc.cpp

It would be helpful to put a comment, e.g. "Pin one object at the beginning".

PeterSolMS · 2021-02-25T09:37:42Z

src/coreclr/gc/gc.cpp

Ditto: "Pin one object near the middle".

PeterSolMS · 2021-02-25T09:39:44Z

src/coreclr/gc/gc.cpp

I don't understand why we need a new free region here - could you explain?

it's to prepare for possibly needing to get a new region for gen0 in the later phases.

PeterSolMS · 2021-02-25T09:42:31Z

src/coreclr/gc/gc.cpp

In your comments for the PR, you said in this case we will do a special kind of sweep. The dprintf says we do a full compacting GC. Is there a contradiction, or what am I missing?

it's the "and find out that we do need it," part - we try to get a new region here just to prepare for if we need it, as long as we don't actually need it, we will be doing a full compacting GC. but if we can't get it here and later find that we actually need it that's when we absolute run out of memory and we only do the special sweep then.

Got it, thanks!

PeterSolMS · 2021-02-25T09:50:18Z

src/coreclr/gc/gc.cpp

Could you put a comment why this should be true?

PeterSolMS · 2021-02-25T09:57:49Z

src/coreclr/gc/gc.cpp

I think it would be good to put a comment explaining how the special sweep mode works, i.e. what's special about it.

+ implemented the logic to handle when we are really running out of memory. We need to be resilient to it during a GC. We might need an empty region per heap so we try to get it up front and if we can't get it and find out that we do need it, we resort to a special sweep mode. + also take virtual memory load into consideration + fixed the logic to decide whether we should compact based on space. + fixed various places when we can't get a region, we need to make sure we don't thread 0 into the region list. + made allocated_since_last_gc per more_space_lock otherwise we are not doing the accounting correctly + moved the pinning for STRESS_REGIONS into a blocking GC otherwise we can have AV from BGC trying to mark the pinned object while it's being constructed + got rid of the places that use the hard coded REGION_SIZE + included one perf change in reset_memory

ghost added the area-GC-coreclr label Feb 24, 2021

mangod9 reviewed Feb 24, 2021

View reviewed changes

src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved

mangod9 reviewed Feb 24, 2021

View reviewed changes

src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved

mangod9 reviewed Feb 24, 2021

View reviewed changes

PeterSolMS reviewed Feb 25, 2021

View reviewed changes

src/coreclr/gc/gc.cpp Outdated

Copy link

Contributor

PeterSolMS Feb 25, 2021

Choose a reason for hiding this comment

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

Use dprintf level REGIONS_LOG here as well?

PeterSolMS reviewed Feb 25, 2021

View reviewed changes

src/coreclr/gc/gc.cpp Outdated

Copy link

Contributor

PeterSolMS Feb 25, 2021

Choose a reason for hiding this comment

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

Ditto.

PeterSolMS reviewed Feb 25, 2021

View reviewed changes

src/coreclr/gc/gc.cpp Outdated

Copy link

Contributor

PeterSolMS Feb 25, 2021

Choose a reason for hiding this comment

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

Could you put a comment why this should be true?

PeterSolMS reviewed Feb 25, 2021

View reviewed changes

PeterSolMS approved these changes Feb 25, 2021

View reviewed changes

runfoapp bot mentioned this pull request Feb 25, 2021

Test failure: readytorun/coreroot_determinism/coreroot_determinism/coreroot_determinism.sh #45326

Closed

Maoni0 force-pushed the regions_mem_limit branch from 247995b to f532c41 Compare February 26, 2021 10:04

Maoni0 merged commit c41894d into dotnet:master Feb 26, 2021

ghost locked as resolved and limited conversation to collaborators Mar 30, 2021

Conversation

Maoni0 commented Feb 24, 2021

Uh oh!

ghost commented Feb 24, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maoni0 commented Feb 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants