region memory limit #48691

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

+        num_heaps = n_heaps;
+#endif //MULTIPLE_HEAPS
+        left_in_commit /= num_heaps;
+        if (left_in_commit <= space_required)


should this just be < ?

yes, will change it.

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

src/coreclr/gc/gc.cpp

@@ -3571,7 +3571,7 @@ uint8_t* region_allocator::allocate (uint32_t num_units)
    uint32_t* current_index = region_map_start;
    uint32_t* end_index = region_map_end;

-    dprintf (1, ("\nsearcing %d->%d", (int)(current_index - region_map_start), (int)(end_index - region_map_start)));
+    dprintf (1, ("searching %d->%d", (int)(current_index - region_map_start), (int)(end_index - region_map_start)));


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

    from = align_lower_segment (from);
    end = align_on_segment (end);
-    dprintf (1, ("from: %Ix, end: %Ix, size: %Ix", from, end, sizeof (seg_mapping)*(((size_t)(end - from) >> gc_heap::min_segment_size_shr))));
+    dprintf (1, ("from: %Ix, end: %Ix, size: %Ix", from, end, 


Use dprintf level REGIONS_LOG here as well?

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

src/coreclr/gc/gc.cpp

+        region = allocate_new_region (__this, 0, false);
+        if (region)
+        {
+            init_table_for_region (0, region);


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

+
+    if (gen_number <= max_generation)
+    {
+        size_t first_brick = brick_of (heap_segment_mem (region));


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

+    }
+    else
+    {
+        assert (brick_table[brick_of (heap_segment_mem (region))] == 0);


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

@@ -3364,7 +3377,7 @@ class gc_heap
    PER_HEAP
    int pinning_seg_interval;
    PER_HEAP
-    int num_gen0_segs;
+    int num_gen0_regions;


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

+
+            if ((num_gen0_regions % pinning_seg_interval) == 0)
+            {
+                int align_const = get_alignment_constant (TRUE);


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

+                uint8_t* obj_to_pin = heap_segment_mem (gen0_region);
+                pin_by_gc (obj_to_pin);
+
+                obj_to_pin += Align (size (obj_to_pin), align_const);


Ditto: "Pin one object near the middle".

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

src/coreclr/gc/gc.cpp

@@ -18218,6 +18243,14 @@ int gc_heap::generation_to_condemn (int n_initial,
        }
    }

+#ifdef USE_REGIONS
+    if (!try_get_new_free_region())


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

+#ifdef USE_REGIONS
+    if (!try_get_new_free_region())
+    {
+        dprintf (GTC_LOG, ("can't get an empty region -> full compacting"));


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

    }
 }

 void gc_heap::process_remaining_regions (int current_plan_gen_num, generation* consing_gen)
 {
    assert ((current_plan_gen_num == 0) || (!settings.promotion && (current_plan_gen_num == -1)));

+    if (special_sweep_p)
+    {
+        assert (pinned_plug_que_empty_p());


Could you put a comment why this should be true?

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

src/coreclr/gc/gc.cpp

+                    {
+                        dprintf (REGIONS_LOG, ("h%d couldn't get a region to plan gen0, special sweep on",
+                            heap_number));
+                        special_sweep_p = true;


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

dotnet-issue-labeler bot 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

}

else

{

assert (brick_table[brick_of (heap_segment_mem (region))] == 0);

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

region memory limit #48691

region memory limit #48691

Maoni0 commented Feb 24, 2021

ghost commented Feb 24, 2021

mangod9 Feb 24, 2021

Maoni0 Feb 24, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 25, 2021

PeterSolMS Feb 25, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 25, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 25, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 26, 2021

PeterSolMS Feb 25, 2021

Maoni0 commented Feb 25, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 26, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 26, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 26, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 26, 2021

PeterSolMS Feb 25, 2021

Maoni0 Feb 26, 2021

PeterSolMS Feb 28, 2021

PeterSolMS Feb 25, 2021

PeterSolMS Feb 25, 2021

region memory limit #48691

region memory limit #48691

Conversation

Maoni0 commented Feb 24, 2021

ghost commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maoni0 commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment