From fd0df1f8c6845671c86eddd85c9bcced30501690 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Mon, 28 Aug 2023 16:34:27 -0400 Subject: [PATCH] Fix growth in minor GC when we have initial slots If initial slots is set, then during a minor GC, if we have allocatable pages but the heap is mostly full, then we will set `grow_heap` to true since `total_slots` does not count allocatable pages so it will be less than `init_slots`. This can cause `allocatable_pages` to grow to much higher than desired since it will appear that the heap is mostly full. --- gc.c | 11 +++++------ test/ruby/test_gc.rb | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/gc.c b/gc.c index e57e8cf52e..9178c9462f 100644 --- a/gc.c +++ b/gc.c @@ -5689,12 +5689,11 @@ gc_sweep_finish_size_pool(rb_objspace_t *objspace, rb_size_pool_t *size_pool) if (swept_slots < min_free_slots) { bool grow_heap = is_full_marking(objspace); - if (!is_full_marking(objspace)) { - /* The heap is a growth heap if it freed more slots than had empty - * slots and used up all of its allocatable pages. */ - bool is_growth_heap = (size_pool->empty_slots == 0 || - size_pool->freed_slots > size_pool->empty_slots) && - size_pool->allocatable_pages == 0; + /* Consider growing or starting a major GC if we are not currently in a + * major GC and we can't allocate any more pages. */ + if (!is_full_marking(objspace) && size_pool->allocatable_pages == 0) { + /* The heap is a growth heap if it freed more slots than had empty slots. */ + bool is_growth_heap = size_pool->empty_slots == 0 || size_pool->freed_slots > size_pool->empty_slots; /* Grow this heap if we haven't run at least RVALUE_OLD_AGE minor * GC since the last major GC or if this heap is smaller than the diff --git a/test/ruby/test_gc.rb b/test/ruby/test_gc.rb index c5a9731f8a..8fc511331e 100644 --- a/test/ruby/test_gc.rb +++ b/test/ruby/test_gc.rb @@ -492,6 +492,45 @@ class TestGc < Test::Unit::TestCase assert_in_epsilon(SIZES[i], total_slots, 0.01, s) end RUBY + + # Check that we don't grow the heap in minor GC if we have alloctable pages. + env["RUBY_GC_HEAP_FREE_SLOTS_MIN_RATIO"] = "0.3" + env["RUBY_GC_HEAP_FREE_SLOTS_GOAL_RATIO"] = "0.99" + env["RUBY_GC_HEAP_FREE_SLOTS_MAX_RATIO"] = "1.0" + env["RUBY_GC_HEAP_OLDOBJECT_LIMIT_FACTOR"] = "100" # Large value to disable major GC + assert_separately([env, "-W0"], __FILE__, __LINE__, <<~RUBY) + SIZES = #{sizes} + + # Run a major GC to clear out dead objects. + GC.start + + # Disable GC so we can control when GC is ran. + GC.disable + + # Run minor GC enough times so that we don't grow the heap because we + # haven't yet ran RVALUE_OLD_AGE minor GC cycles. + GC::INTERNAL_CONSTANTS[:RVALUE_OLD_AGE].times { GC.start(full_mark: false) } + + # Fill size pool 0 to over 50% full so that the number of allocatable + # pages that will be created will be over the number in heap_allocatable_pages + # (calculated using RUBY_GC_HEAP_FREE_SLOTS_MIN_RATIO). + # 70% was chosen here to guarantee that. + ary = [] + while GC.stat_heap(0, :heap_allocatable_pages) > + (GC.stat_heap(0, :heap_allocatable_pages) + GC.stat_heap(0, :heap_eden_pages)) * 0.3 + ary << Object.new + end + + GC.start(full_mark: false) + + # Check that we still have the same number of slots as initially configured. + GC.stat_heap.each do |i, s| + # Sometimes pages will have 1 less slot due to alignment, so always increase slots_per_page by 1. + slots_per_page = (s[:heap_eden_slots] / s[:heap_eden_pages]) + 1 + total_slots = s[:heap_eden_slots] + s[:heap_allocatable_pages] * slots_per_page + assert_in_epsilon(SIZES[i], total_slots, 0.01, s) + end + RUBY end def test_profiler_enabled