Skip to content

Commit 3eef23b

Browse files
committed
fix underflow possibility and nearby overflow one
decide_on_decommit_strategy dst -> dest to match majority of file Rename remove_surplus_regions->trim_region_list and add_regions->grow_region_list. Also rename and reorder parameters to dest/src and drop references to "surplus" because these are general utilities. Drop a few comments that aren't adding anything beyond the code. dfr comment
1 parent 8e81835 commit 3eef23b

File tree

2 files changed

+64
-27
lines changed

2 files changed

+64
-27
lines changed

src/coreclr/gc/gc.cpp

Lines changed: 62 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -12798,35 +12798,29 @@ void gc_heap::rearrange_heap_segments(BOOL compacting)
1279812798
#endif //!USE_REGIONS
1279912799

1280012800
#if defined(USE_REGIONS)
12801-
// trim down the list of free regions pointed at by free_list down to target_count, moving the extra ones to surplus_list
12802-
static void remove_surplus_regions (region_free_list* free_list, region_free_list* surplus_list, size_t target_count)
12801+
// trim down the list of regions pointed at by src down to target_count, moving the extra ones to dest
12802+
static void trim_region_list (region_free_list* dest, region_free_list* src, size_t target_count)
1280312803
{
12804-
while (free_list->get_num_free_regions() > target_count)
12804+
while (src->get_num_free_regions() > target_count)
1280512805
{
12806-
// remove one region from the heap's free list
12807-
heap_segment* region = free_list->unlink_region_front();
12808-
12809-
// and put it on the surplus list
12810-
surplus_list->add_region_front (region);
12806+
heap_segment* region = src->unlink_region_front();
12807+
dest->add_region_front (region);
1281112808
}
1281212809
}
1281312810

12814-
// add regions from surplus_list to free_list, trying to reach target_count
12815-
static int64_t add_regions (region_free_list* free_list, region_free_list* surplus_list, size_t target_count)
12811+
// add regions from src to dest, trying to grow the size of dest to target_count
12812+
static int64_t grow_region_list (region_free_list* dest, region_free_list* src, size_t target_count)
1281612813
{
1281712814
int64_t added_count = 0;
12818-
while (free_list->get_num_free_regions() < target_count)
12815+
while (dest->get_num_free_regions() < target_count)
1281912816
{
12820-
if (surplus_list->get_num_free_regions() == 0)
12817+
if (src->get_num_free_regions() == 0)
1282112818
break;
1282212819

1282312820
added_count++;
1282412821

12825-
// remove one region from the surplus list
12826-
heap_segment* region = surplus_list->unlink_region_front();
12827-
12828-
// and put it on the heap's free list
12829-
free_list->add_region_front (region);
12822+
heap_segment* region = src->unlink_region_front();
12823+
dest->add_region_front (region);
1283012824
}
1283112825
return added_count;
1283212826
}
@@ -13344,6 +13338,49 @@ void gc_heap::age_free_regions (const char* msg)
1334413338
}
1334513339
}
1334613340

13341+
// distribute_free_regions is called during all blocking GCs and in the start of the BGC mark phase
13342+
// unless we already called it during an ephemeral GC right before the BGC.
13343+
//
13344+
// Free regions are stored on the following permanent lists:
13345+
// - global_regions_to_decommit
13346+
// - global_free_huge_regions
13347+
// - (per-heap) free_regions
13348+
// and the following lists that are local to distribute_free_regions:
13349+
// - aged_regions
13350+
// - surplus_regions
13351+
//
13352+
// For reason_induced_aggressive GCs, we decommit all regions. Therefore, the below description is
13353+
// for other GC types.
13354+
//
13355+
// distribute_free_regions steps:
13356+
//
13357+
// 1. Process region ages
13358+
// a. Move all huge regions from free_regions to global_free_huge_regions.
13359+
// (The intention is that free_regions shouldn't contain any huge regions outside of the period
13360+
// where a GC reclaims them and distribute_free_regions moves them to global_free_huge_regions,
13361+
// though perhaps BGC can leave them there. Future work could verify and assert this.)
13362+
// b. Move any basic region in global_regions_to_decommit (which means we intended to decommit them
13363+
// but haven't done so yet) to surplus_regions
13364+
// b. Move all huge regions that are past the age threshold from global_free_huge_regions to aged_regions
13365+
// c. Move all basic/large regions that are past the age threshold from free_regions to aged_regions
13366+
// 2. Move all regions from aged_regions to global_regions_to_decommit. Note that the intention is to
13367+
// combine this with move_highest_free_regions in a future change, which is why we don't just do this
13368+
// in steps 1b/1c.
13369+
// 3. Compute the required per-heap budgets for SOH (basic regions) and the balance. The budget for LOH
13370+
// (large/huge) is zero as we are using an entirely age-based approach.
13371+
// balance = (number of free regions) - budget
13372+
// 4. Decide if we are going to distribute or decommit a nonzero balance. To distribute, we adjust the
13373+
// per-heap budgets, so after this step the LOH (large/huge) budgets can be positive.
13374+
// a. A negative balance (deficit) must be distributed since decommitting a negative number of regions
13375+
// doesn't make sense. A negative balance isn't possible for LOH since the budgets start at zero.
13376+
// b. For SOH (basic), we will decommit surplus regions unless we are in a foreground GC during BGC.
13377+
// c. For LOH (large/huge), we will distribute surplus regions since we are using an entirely age-based
13378+
// approach. However, if we are in a high-memory-usage scenario, we will decommit.
13379+
// 5. Implement the distribute-or-decommit strategy. To distribute, we simply move regions across heaps,
13380+
// using surplus_regions as a holding space. To decommit, for server GC we generally leave them on the
13381+
// global_regions_to_decommit list and decommit them over time. However, in high-memory-usage scenarios,
13382+
// we will immediately decommit some or all of these regions. For workstation GC, we decommit a limited
13383+
// amount and move the rest back to the (one) heap's free_list.
1334713384
void gc_heap::distribute_free_regions()
1334813385
{
1334913386
#ifdef MULTIPLE_HEAPS
@@ -13582,7 +13619,7 @@ void gc_heap::distribute_free_regions()
1358213619
hp->free_regions[kind].get_num_free_regions(),
1358313620
heap_budget_in_region_units[kind][i]));
1358413621

13585-
remove_surplus_regions (&hp->free_regions[kind], &surplus_regions[kind], heap_budget_in_region_units[kind][i]);
13622+
trim_region_list (&surplus_regions[kind], &hp->free_regions[kind], heap_budget_in_region_units[kind][i]);
1358613623
}
1358713624
}
1358813625
// finally go through all the heaps and distribute any surplus regions to heaps having too few free regions
@@ -13598,7 +13635,7 @@ void gc_heap::distribute_free_regions()
1359813635
// second pass: fill all the regions having less than budget
1359913636
if (hp->free_regions[kind].get_num_free_regions() < heap_budget_in_region_units[kind][i])
1360013637
{
13601-
int64_t num_added_regions = add_regions (&hp->free_regions[kind], &surplus_regions[kind], heap_budget_in_region_units[kind][i]);
13638+
int64_t num_added_regions = grow_region_list (&hp->free_regions[kind], &surplus_regions[kind], heap_budget_in_region_units[kind][i]);
1360213639
dprintf (REGIONS_LOG, ("added %zd %s regions to heap %d - now has %zd, budget is %zd",
1360313640
(size_t)num_added_regions,
1360413641
free_region_kind_name[kind],
@@ -13616,7 +13653,7 @@ void gc_heap::distribute_free_regions()
1361613653
}
1361713654
}
1361813655

13619-
decide_decommit_strategy(aggressive_decommit_large_p);
13656+
decide_on_decommit_strategy(aggressive_decommit_large_p);
1362013657
}
1362113658

1362213659
void gc_heap::move_all_aged_regions(size_t total_num_free_regions[count_distributed_free_region_kinds], region_free_list aged_regions[count_free_region_kinds], bool joined_last_gc_before_oom)
@@ -13640,7 +13677,7 @@ void gc_heap::move_all_aged_regions(size_t total_num_free_regions[count_distribu
1364013677
}
1364113678
}
1364213679

13643-
void gc_heap::move_aged_regions(region_free_list dst[count_free_region_kinds], region_free_list& src, free_region_kind kind, bool joined_last_gc_before_oom)
13680+
void gc_heap::move_aged_regions(region_free_list dest[count_free_region_kinds], region_free_list& src, free_region_kind kind, bool joined_last_gc_before_oom)
1364413681
{
1364513682
heap_segment* next_region = nullptr;
1364613683
for (heap_segment* region = src.get_first_free_region(); region != nullptr; region = next_region)
@@ -13651,7 +13688,7 @@ void gc_heap::move_aged_regions(region_free_list dst[count_free_region_kinds], r
1365113688
((get_region_committed_size (region) == GC_PAGE_SIZE) && joined_last_gc_before_oom))
1365213689
{
1365313690
region_free_list::unlink_region (region);
13654-
region_free_list::add_region (region, dst);
13691+
region_free_list::add_region (region, dest);
1365513692
}
1365613693
}
1365713694
}
@@ -13788,7 +13825,7 @@ bool gc_heap::distribute_surplus_p(ptrdiff_t balance, int kind, bool aggressive_
1378813825
return !aggressive_decommit_large_p;
1378913826
}
1379013827

13791-
void gc_heap::decide_decommit_strategy(bool joined_last_gc_before_oom)
13828+
void gc_heap::decide_on_decommit_strategy(bool joined_last_gc_before_oom)
1379213829
{
1379313830
#ifdef MULTIPLE_HEAPS
1379413831

@@ -53440,7 +53477,7 @@ bool gc_heap::compute_memory_settings(bool is_initialization, uint32_t& nhp, uin
5344053477
if (highmem_th_from_config)
5344153478
{
5344253479
high_memory_load_th = min (99u, highmem_th_from_config);
53443-
v_high_memory_load_th = min (99u, (highmem_th_from_config + 7));
53480+
v_high_memory_load_th = min (99u, (high_memory_load_th + 7));
5344453481
#ifdef FEATURE_EVENT_TRACE
5344553482
high_mem_percent_from_config = highmem_th_from_config;
5344653483
#endif //FEATURE_EVENT_TRACE
@@ -53465,7 +53502,7 @@ bool gc_heap::compute_memory_settings(bool is_initialization, uint32_t& nhp, uin
5346553502
}
5346653503

5346753504
m_high_memory_load_th = min ((high_memory_load_th + 5), v_high_memory_load_th);
53468-
almost_high_memory_load_th = max((high_memory_load_th - 5), 1u);
53505+
almost_high_memory_load_th = (high_memory_load_th > 5) ? (high_memory_load_th - 5) : 1; // avoid underflow of high_memory_load_th - 5
5346953506

5347053507
return true;
5347153508
}

src/coreclr/gc/gcpriv.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1734,13 +1734,13 @@ class gc_heap
17341734

17351735
PER_HEAP_ISOLATED_METHOD void distribute_free_regions();
17361736
PER_HEAP_ISOLATED_METHOD void move_all_aged_regions(size_t total_num_free_regions[count_distributed_free_region_kinds], region_free_list aged_regions[count_free_region_kinds], bool joined_last_gc_before_oom);
1737-
PER_HEAP_ISOLATED_METHOD void move_aged_regions(region_free_list dst[count_free_region_kinds], region_free_list& src, free_region_kind kind, bool joined_last_gc_before_oom);
1737+
PER_HEAP_ISOLATED_METHOD void move_aged_regions(region_free_list dest[count_free_region_kinds], region_free_list& src, free_region_kind kind, bool joined_last_gc_before_oom);
17381738
PER_HEAP_ISOLATED_METHOD bool aged_region_p(heap_segment* region, free_region_kind kind);
17391739
PER_HEAP_ISOLATED_METHOD void move_regions_to_decommit(region_free_list oregions[count_free_region_kinds]);
17401740
PER_HEAP_ISOLATED_METHOD size_t compute_basic_region_budgets(size_t heap_basic_budget_in_region_units[MAX_SUPPORTED_CPUS], size_t min_heap_basic_budget_in_region_units[MAX_SUPPORTED_CPUS], size_t total_basic_free_regions);
17411741
PER_HEAP_ISOLATED_METHOD bool near_heap_hard_limit_p();
17421742
PER_HEAP_ISOLATED_METHOD bool distribute_surplus_p(ptrdiff_t balance, int kind, bool aggressive_decommit_large_p);
1743-
PER_HEAP_ISOLATED_METHOD void decide_decommit_strategy(bool joined_last_gc_before_oom);
1743+
PER_HEAP_ISOLATED_METHOD void decide_on_decommit_strategy(bool joined_last_gc_before_oom);
17441744

17451745
PER_HEAP_ISOLATED_METHOD void age_free_regions (const char* msg);
17461746

0 commit comments

Comments
 (0)