From 4f0d58260a7f506ead198064d12a967edd11fe5e Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Wed, 30 Aug 2023 15:28:22 -0400 Subject: [PATCH] Correctly calculate initial pages The old algorithm could calculate an undercount for the initial pages due to two issues: 1. It did not take into account that some heap pages will have one less slot due to alignment. It assumed that every heap page would be able to be fully filled with slots. Pages that are unaligned with the slot size will lose one slot. The new algorithm assumes that every page will be unaligned. 2. It performed integer division, which truncates down. This means that the number of pages might not actually satisfy the number of slots. This can cause the heap to grow in `gc_sweep_finish_size_pool` after allocating all of the allocatable pages because the total number of slots would be less than the initial configured number of slots. --- gc.c | 30 ++++++++++++++++++++--------- test/ruby/test_gc.rb | 46 ++++++++++++++++++++++++++++++-------------- 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/gc.c b/gc.c index e2c2779b47..59c507369e 100644 --- a/gc.c +++ b/gc.c @@ -2305,11 +2305,22 @@ heap_add_pages(rb_objspace_t *objspace, rb_size_pool_t *size_pool, rb_heap_t *he #endif static size_t -minimum_pages_for_size_pool(rb_objspace_t *objspace, int size_pool_idx) +slots_to_pages_for_size_pool(rb_objspace_t *objspace, rb_size_pool_t *size_pool, size_t slots) { - rb_size_pool_t *size_pool = &size_pools[size_pool_idx]; - int multiple = size_pool->slot_size / BASE_SLOT_SIZE; - return gc_params.size_pool_init_slots[size_pool_idx] * multiple / HEAP_PAGE_OBJ_LIMIT; + size_t multiple = size_pool->slot_size / BASE_SLOT_SIZE; + /* Due to alignment, heap pages may have one less slot. We should + * ensure there is enough pages to guarantee that we will have at + * least the required number of slots after allocating all the pages. */ + size_t slots_per_page = (HEAP_PAGE_OBJ_LIMIT / multiple) - 1; + return CEILDIV(slots, slots_per_page); +} + +static size_t +minimum_pages_for_size_pool(rb_objspace_t *objspace, rb_size_pool_t *size_pool) +{ + size_t size_pool_idx = size_pool - size_pools; + size_t init_slots = gc_params.size_pool_init_slots[size_pool_idx]; + return slots_to_pages_for_size_pool(objspace, size_pool, init_slots); } static size_t @@ -2322,7 +2333,7 @@ heap_extend_pages(rb_objspace_t *objspace, rb_size_pool_t *size_pool, size_t fre next_used = (size_t)(used * gc_params.growth_factor); } else if (total_slots == 0) { - next_used = minimum_pages_for_size_pool(objspace, (int)(size_pool - size_pools)); + next_used = minimum_pages_for_size_pool(objspace, size_pool); } else { /* Find `f' where free_slots = f * total_slots * goal_ratio @@ -3728,10 +3739,12 @@ Init_heap(void) /* Set size pools allocatable pages. */ for (int i = 0; i < SIZE_POOL_COUNT; i++) { + rb_size_pool_t *size_pool = &size_pools[i]; + /* Set the default value of size_pool_init_slots. */ gc_params.size_pool_init_slots[i] = GC_HEAP_INIT_SLOTS; - size_pools[i].allocatable_pages = minimum_pages_for_size_pool(objspace, i); + size_pool->allocatable_pages = minimum_pages_for_size_pool(objspace, size_pool); } heap_pages_expand_sorted(objspace); @@ -10829,7 +10842,7 @@ gc_verify_compaction_references(rb_execution_context_t *ec, VALUE self, VALUE do size_t minimum_pages = 0; if (RTEST(expand_heap)) { - minimum_pages = minimum_pages_for_size_pool(objspace, i); + minimum_pages = minimum_pages_for_size_pool(objspace, size_pool); } heap_add_pages(objspace, size_pool, heap, MAX(minimum_pages, heap->total_pages)); @@ -11643,8 +11656,7 @@ gc_set_initial_pages(rb_objspace_t *objspace) if (size_pool_init_slots > size_pool->eden_heap.total_slots) { size_t slots = size_pool_init_slots - size_pool->eden_heap.total_slots; - int multiple = size_pool->slot_size / BASE_SLOT_SIZE; - size_pool->allocatable_pages = slots * multiple / HEAP_PAGE_OBJ_LIMIT; + size_pool->allocatable_pages = slots_to_pages_for_size_pool(objspace, size_pool, slots); } else { /* We already have more slots than size_pool_init_slots allows, so diff --git a/test/ruby/test_gc.rb b/test/ruby/test_gc.rb index 3967ed54bc..0b4062e99f 100644 --- a/test/ruby/test_gc.rb +++ b/test/ruby/test_gc.rb @@ -444,12 +444,12 @@ class TestGc < Test::Unit::TestCase # Constant from gc.c. GC_HEAP_INIT_SLOTS = 10_000 GC.stat_heap.each do |_, 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 + multiple = s[:slot_size] / (GC::INTERNAL_CONSTANTS[:BASE_SLOT_SIZE] + GC::INTERNAL_CONSTANTS[:RVALUE_OVERHEAD]) + # Allocatable pages are assumed to have lost 1 slot due to alignment. + slots_per_page = (GC::INTERNAL_CONSTANTS[:HEAP_PAGE_OBJ_LIMIT] / multiple) - 1 + total_slots = s[:heap_eden_slots] + s[:heap_allocatable_pages] * slots_per_page - # Give a 0.9x delta because integer division in minimum_pages_for_size_pool can sometimes cause number to be - # less than GC_HEAP_INIT_SLOTS. - assert_operator(total_slots, :>=, GC_HEAP_INIT_SLOTS * 0.9, s) + assert_operator(total_slots, :>=, GC_HEAP_INIT_SLOTS, s) end RUBY @@ -462,10 +462,16 @@ class TestGc < Test::Unit::TestCase assert_separately([env, "-W0"], __FILE__, __LINE__, <<~RUBY) SIZES = #{sizes} 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 + multiple = s[:slot_size] / (GC::INTERNAL_CONSTANTS[:BASE_SLOT_SIZE] + GC::INTERNAL_CONSTANTS[:RVALUE_OVERHEAD]) + # Allocatable pages are assumed to have lost 1 slot due to alignment. + slots_per_page = (GC::INTERNAL_CONSTANTS[:HEAP_PAGE_OBJ_LIMIT] / multiple) - 1 + total_slots = s[:heap_eden_slots] + s[:heap_allocatable_pages] * slots_per_page - assert_in_epsilon(SIZES[i], total_slots, 0.01, s) + + # The delta is calculated as follows: + # - For allocated pages, each page can vary by 1 slot due to alignment. + # - For allocatable pages, we can end up with at most 1 extra page of slots. + assert_in_delta(SIZES[i], total_slots, s[:heap_eden_pages] + slots_per_page, s) end RUBY @@ -486,10 +492,16 @@ class TestGc < Test::Unit::TestCase # 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 + multiple = s[:slot_size] / (GC::INTERNAL_CONSTANTS[:BASE_SLOT_SIZE] + GC::INTERNAL_CONSTANTS[:RVALUE_OVERHEAD]) + # Allocatable pages are assumed to have lost 1 slot due to alignment. + slots_per_page = (GC::INTERNAL_CONSTANTS[:HEAP_PAGE_OBJ_LIMIT] / multiple) - 1 + total_slots = s[:heap_eden_slots] + s[:heap_allocatable_pages] * slots_per_page - assert_in_epsilon(SIZES[i], total_slots, 0.01, s) + + # The delta is calculated as follows: + # - For allocated pages, each page can vary by 1 slot due to alignment. + # - For allocatable pages, we can end up with at most 1 extra page of slots. + assert_in_delta(SIZES[i], total_slots, s[:heap_eden_pages] + slots_per_page, s) end RUBY @@ -525,10 +537,16 @@ class TestGc < Test::Unit::TestCase # 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 + multiple = s[:slot_size] / (GC::INTERNAL_CONSTANTS[:BASE_SLOT_SIZE] + GC::INTERNAL_CONSTANTS[:RVALUE_OVERHEAD]) + # Allocatable pages are assumed to have lost 1 slot due to alignment. + slots_per_page = (GC::INTERNAL_CONSTANTS[:HEAP_PAGE_OBJ_LIMIT] / multiple) - 1 + total_slots = s[:heap_eden_slots] + s[:heap_allocatable_pages] * slots_per_page - assert_in_epsilon(SIZES[i], total_slots, 0.01, s) + + # The delta is calculated as follows: + # - For allocated pages, each page can vary by 1 slot due to alignment. + # - For allocatable pages, we can end up with at most 1 extra page of slots. + assert_in_delta(SIZES[i], total_slots, s[:heap_eden_pages] + slots_per_page, s) end RUBY end