Skip to content

Commit

Permalink
Remove hard_limit_for_bookkeeping (#77480)
Browse files Browse the repository at this point in the history
As it turns out, it's not so easy to set aside a hard limit for the GC's bookkeeping, because some regions will be partially comitted and so will need more bookkeeping information as a percentage.

So this removes the hard_limit_for_bookkeeping and associated fields, and instead commits memory for the mark array eagerly, even if BGC is not in progress.

The issue is that GC needs at least one free region, and it needs the mark array committed. So when it tries to get a region at the end, and we cannot commit the mark array, we cannot actually get a free region. Currently we AV in this case, but there really isn't a good option to recover at this point. So it's better to keep the mark array committed and perhaps fail with an OOM exception.
  • Loading branch information
PeterSolMS committed Dec 12, 2022
1 parent fda0b35 commit 57a48c6
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 68 deletions.
76 changes: 14 additions & 62 deletions src/coreclr/gc/gc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2336,9 +2336,6 @@ size_t gc_heap::heap_hard_limit_oh[total_oh_count];

size_t gc_heap::regions_range = 0;

size_t gc_heap::heap_hard_limit_for_heap = 0;
size_t gc_heap::heap_hard_limit_for_bookkeeping = 0;

#endif //USE_REGIONS

bool affinity_config_specified_p = false;
Expand Down Expand Up @@ -6956,34 +6953,16 @@ bool gc_heap::virtual_commit (void* address, size_t size, int bucket, int h_numb

if (heap_hard_limit_oh[soh] != 0)
{
#ifdef USE_REGIONS
assert (heap_hard_limit_for_heap == 0);
assert (heap_hard_limit_for_bookkeeping == 0);
#endif //USE_REGIONS
if ((bucket < total_oh_count) && (committed_by_oh[bucket] + size) > heap_hard_limit_oh[bucket])
{
exceeded_p = true;
}
}
else
{
size_t base;
size_t limit;
#ifdef USE_REGIONS
if (h_number < 0)
{
base = current_total_committed_bookkeeping;
limit = heap_hard_limit_for_bookkeeping;
}
else
{
base = current_total_committed - current_total_committed_bookkeeping;
limit = heap_hard_limit_for_heap;
}
#else
base = current_total_committed;
limit = heap_hard_limit;
#endif //USE_REGIONS
size_t base = current_total_committed;
size_t limit = heap_hard_limit;

if ((base + size) > limit)
{
dprintf (1, ("%zd + %zd = %zd > limit %zd ", base, size, (base + size), limit));
Expand Down Expand Up @@ -11339,12 +11318,6 @@ void gc_heap::clear_region_info (heap_segment* region)
seg_deleted);

bgc_verify_mark_array_cleared (region);

if (dt_high_memory_load_p())
{
decommit_mark_array_by_seg (region);
region->flags &= ~(heap_segment_flags_ma_committed);
}
#endif //BACKGROUND_GC
}

Expand Down Expand Up @@ -13671,26 +13644,6 @@ HRESULT gc_heap::initialize_gc (size_t soh_segment_size,
#endif //BACKGROUND_GC
#endif //WRITE_WATCH

#ifdef USE_REGIONS
if (gc_heap::heap_hard_limit && gc_heap::heap_hard_limit_oh[soh] == 0)
{
size_t gc_region_size = (size_t)1 << min_segment_size_shr;
size_t sizes[total_bookkeeping_elements];
size_t bookkeeping_size_per_region = 0;
uint8_t* temp_lowest_address = (uint8_t*)gc_region_size;
gc_heap::get_card_table_element_sizes(temp_lowest_address, temp_lowest_address + gc_region_size, sizes);
for (int i = 0; i < total_bookkeeping_elements; i++)
{
bookkeeping_size_per_region += sizes[i];
}
size_t total_size_per_region = gc_region_size + bookkeeping_size_per_region;
size_t max_region_count = gc_heap::heap_hard_limit / total_size_per_region; // implictly rounded down
gc_heap::heap_hard_limit_for_heap = max_region_count * gc_region_size;
gc_heap::heap_hard_limit_for_bookkeeping = max_region_count * bookkeeping_size_per_region;
dprintf (REGIONS_LOG, ("bookkeeping_size_per_region = %zd", bookkeeping_size_per_region));
}
#endif //USE_REGIONS

#ifdef BACKGROUND_GC
// leave the first page to contain only segment info
// because otherwise we could need to revisit the first page frequently in
Expand Down Expand Up @@ -14450,6 +14403,8 @@ gc_heap::init_gc_heap (int h_number)
#endif //CARD_BUNDLE

#ifdef BACKGROUND_GC
background_saved_highest_address = nullptr;
background_saved_lowest_address = nullptr;
if (gc_can_use_concurrent)
mark_array = translate_mark_array (card_table_mark_array (&g_gc_card_table[card_word (card_of (g_gc_lowest_address))]));
else
Expand Down Expand Up @@ -20370,20 +20325,17 @@ bool gc_heap::try_get_new_free_region()
bool gc_heap::init_table_for_region (int gen_number, heap_segment* region)
{
#ifdef BACKGROUND_GC
if (is_bgc_in_progress())
dprintf (GC_TABLE_LOG, ("new seg %Ix, mark_array is %Ix",
heap_segment_mem (region), mark_array));
if (((region->flags & heap_segment_flags_ma_committed) == 0) &&
!commit_mark_array_new_seg (__this, region))
{
dprintf (GC_TABLE_LOG, ("new seg %p, mark_array is %p",
heap_segment_mem (region), mark_array));
if (((region->flags & heap_segment_flags_ma_committed) == 0) &&
!commit_mark_array_new_seg (__this, region))
{
dprintf (GC_TABLE_LOG, ("failed to commit mark array for the new region %p-%p",
get_region_start (region), heap_segment_reserved (region)));
dprintf (GC_TABLE_LOG, ("failed to commit mark array for the new region %Ix-%Ix",
get_region_start (region), heap_segment_reserved (region)));

// We don't have memory to commit the mark array so we cannot use the new region.
global_region_allocator.delete_region (get_region_start (region));
return false;
}
// We don't have memory to commit the mark array so we cannot use the new region.
global_region_allocator.delete_region (get_region_start (region));
return false;
}
if ((region->flags & heap_segment_flags_ma_committed) != 0)
{
Expand Down
6 changes: 0 additions & 6 deletions src/coreclr/gc/gcpriv.h
Original file line number Diff line number Diff line change
Expand Up @@ -4115,12 +4115,6 @@ class gc_heap
PER_HEAP_ISOLATED
size_t heap_hard_limit_oh[total_oh_count];

PER_HEAP_ISOLATED
size_t heap_hard_limit_for_heap;

PER_HEAP_ISOLATED
size_t heap_hard_limit_for_bookkeeping;

PER_HEAP_ISOLATED
CLRCriticalSection check_commit_cs;

Expand Down

0 comments on commit 57a48c6

Please sign in to comment.