From a6dd859affc42b667279e513bb94fb75cfb133c1 Mon Sep 17 00:00:00 2001 From: Matt Valentine-House Date: Thu, 7 Jul 2022 21:52:05 +0100 Subject: [PATCH] Add expand_heap option to GC.verify_compaction_references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to reliably test compaction we need to be able to move objects between size pools. In order for this to happen there must be pages in a size pool into which we can allocate. The existing implementation of `double_heap` only doubled the existing number of pages in the heap, so if a size pool had a low number of pages (or 0) it's not guaranteed that enough space will be created to move objects into that size pool. This commit deprecates the `double_heap` option and replaces it with `expand_heap` instead. expand heap will expand each heap by enough pages to hold a number of slots defined by `GC_HEAP_INIT_SLOTS` or by `heap->total_pags` whichever is larger. If both `double_heap` and `expand_heap` are present, a deprecation warning will be shown for `double_heap` and the `expand_heap` behaviour will take precedence Given that this is an API intended for debugging and testing GC compaction I'm not concerned about the extra memory usage or time taken to create the pages. However, for completeness: Running the following `test.rb` and using `time` on my Macbook Pro shows the following memory usage and time impact: pp "RSS (kb): #{`ps -o rss #{Process.pid}`.lines.last.to_i}" GC.verify_compaction_references(double_heap: true, toward: :empty) pp "RSS (kb): #{`ps -o rss #{Process.pid}`.lines.last.to_i}" ❯ time make run ./miniruby -I./lib -I. -I.ext/common -r./arm64-darwin21-fake ./test.rb "RSS (kb): 24000" :251: warning: double_heap is deprecated and will be removed "RSS (kb): 25232" ________________________________________________________ Executed in 124.37 millis fish external usr time 82.22 millis 0.09 millis 82.12 millis sys time 28.76 millis 2.61 millis 26.15 millis ❯ time make run ./miniruby -I./lib -I. -I.ext/common -r./arm64-darwin21-fake ./test.rb "RSS (kb): 24000" "RSS (kb): 49040" ________________________________________________________ Executed in 150.13 millis fish external usr time 103.32 millis 0.10 millis 103.22 millis sys time 35.73 millis 2.59 millis 33.14 millis --- gc.c | 21 +++++++++++++++++---- gc.rb | 6 +++--- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/gc.c b/gc.c index a1dc054414..4741cb7b04 100644 --- a/gc.c +++ b/gc.c @@ -10703,22 +10703,35 @@ gc_compact(VALUE self) #if GC_CAN_COMPILE_COMPACTION static VALUE -gc_verify_compaction_references(rb_execution_context_t *ec, VALUE self, VALUE double_heap, VALUE toward_empty) +gc_verify_compaction_references(rb_execution_context_t *ec, VALUE self, VALUE double_heap, VALUE expand_heap, VALUE toward_empty) { rb_objspace_t *objspace = &rb_objspace; /* Clear the heap. */ gc_start_internal(NULL, self, Qtrue, Qtrue, Qtrue, Qfalse); + size_t growth_slots = gc_params.heap_init_slots; + + if (RTEST(double_heap)) { + rb_warn("double_heap is deprecated, please use expand_heap instead"); + } RB_VM_LOCK_ENTER(); { gc_rest(objspace); - if (RTEST(double_heap)) { + /* if both double_heap and expand_heap are set, expand_heap takes precedence */ + if (RTEST(double_heap) || RTEST(expand_heap)) { for (int i = 0; i < SIZE_POOL_COUNT; i++) { rb_size_pool_t *size_pool = &size_pools[i]; rb_heap_t *heap = SIZE_POOL_EDEN_HEAP(size_pool); - heap_add_pages(objspace, size_pool, heap, heap->total_pages); + + if (RTEST(expand_heap)) { + size_t required_pages = growth_slots / size_pool->slot_size; + heap_add_pages(objspace, size_pool, heap, MAX(required_pages, heap->total_pages)); + } + else { + heap_add_pages(objspace, size_pool, heap, heap->total_pages); + } } } @@ -10736,7 +10749,7 @@ gc_verify_compaction_references(rb_execution_context_t *ec, VALUE self, VALUE do return gc_compact_stats(self); } #else -# define gc_verify_compaction_references (rb_builtin_arity2_function_type)rb_f_notimplement +# define gc_verify_compaction_references (rb_builtin_arity3_function_type)rb_f_notimplement #endif VALUE diff --git a/gc.rb b/gc.rb index d46b7a7abe..9144a96603 100644 --- a/gc.rb +++ b/gc.rb @@ -242,13 +242,13 @@ module GC # were moved are replaced with T_MOVED objects. No object should have a # reference to a T_MOVED object after compaction. # - # This function doubles the heap to ensure room to move all objects, + # This function expands the heap to ensure room to move all objects, # compacts the heap to make sure everything moves, updates all references, # then performs a full GC. If any object contains a reference to a T_MOVED # object, that object should be pushed on the mark stack, and will # make a SEGV. - def self.verify_compaction_references(toward: nil, double_heap: false) - Primitive.gc_verify_compaction_references(double_heap, toward == :empty) + def self.verify_compaction_references(toward: nil, double_heap: false, expand_heap: false) + Primitive.gc_verify_compaction_references(double_heap, expand_heap, toward == :empty) end end