From 5064af7ed120b1d14ad5020ea39a4a6ff2a4a4ec Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Sat, 3 Jan 2026 09:39:54 -0500 Subject: [PATCH 1/3] [ruby/mmtk] Process obj_free candidates in parallel Redos commit 544770d which seems to have accidentally been undone in b27d935. --- gc/mmtk/mmtk.c | 23 ++++++- gc/mmtk/mmtk.h | 2 +- gc/mmtk/src/api.rs | 11 +++- gc/mmtk/src/weak_proc.rs | 135 ++++++++++++++++++++++++++++----------- 4 files changed, 127 insertions(+), 44 deletions(-) diff --git a/gc/mmtk/mmtk.c b/gc/mmtk/mmtk.c index e9558f906f3242..042517e7d741b6 100644 --- a/gc/mmtk/mmtk.c +++ b/gc/mmtk/mmtk.c @@ -747,6 +747,27 @@ rb_mmtk_alloc_fast_path(struct objspace *objspace, struct MMTk_ractor_cache *rac } } +static bool +obj_can_parallel_free_p(VALUE obj) +{ + switch (RB_BUILTIN_TYPE(obj)) { + case T_ARRAY: + case T_BIGNUM: + case T_COMPLEX: + case T_FLOAT: + case T_HASH: + case T_OBJECT: + case T_RATIONAL: + case T_REGEXP: + case T_STRING: + case T_STRUCT: + case T_SYMBOL: + return true; + default: + return false; + } +} + VALUE rb_gc_impl_new_obj(void *objspace_ptr, void *cache_ptr, VALUE klass, VALUE flags, bool wb_protected, size_t alloc_size) { @@ -783,7 +804,7 @@ rb_gc_impl_new_obj(void *objspace_ptr, void *cache_ptr, VALUE klass, VALUE flags mmtk_post_alloc(ractor_cache->mutator, (void*)alloc_obj, alloc_size, MMTK_ALLOCATION_SEMANTICS_DEFAULT); // TODO: only add when object needs obj_free to be called - mmtk_add_obj_free_candidate(alloc_obj); + mmtk_add_obj_free_candidate(alloc_obj, obj_can_parallel_free_p((VALUE)alloc_obj)); objspace->total_allocated_objects++; diff --git a/gc/mmtk/mmtk.h b/gc/mmtk/mmtk.h index b7c8248718526a..21a5bf94156587 100644 --- a/gc/mmtk/mmtk.h +++ b/gc/mmtk/mmtk.h @@ -123,7 +123,7 @@ void mmtk_post_alloc(MMTk_Mutator *mutator, size_t bytes, MMTk_AllocationSemantics semantics); -void mmtk_add_obj_free_candidate(MMTk_ObjectReference object); +void mmtk_add_obj_free_candidate(MMTk_ObjectReference object, bool can_parallel_free); void mmtk_declare_weak_references(MMTk_ObjectReference object); diff --git a/gc/mmtk/src/api.rs b/gc/mmtk/src/api.rs index ec0e5bafe2289a..3515a2408b3714 100644 --- a/gc/mmtk/src/api.rs +++ b/gc/mmtk/src/api.rs @@ -198,7 +198,10 @@ pub unsafe extern "C" fn mmtk_init_binding( let mmtk_boxed = mmtk_init(&builder); let mmtk_static = Box::leak(Box::new(mmtk_boxed)); - let binding = RubyBinding::new(mmtk_static, &binding_options, upcalls); + let mut binding = RubyBinding::new(mmtk_static, &binding_options, upcalls); + binding + .weak_proc + .init_parallel_obj_free_candidates(memory_manager::num_of_workers(binding.mmtk)); crate::BINDING .set(binding) @@ -296,8 +299,10 @@ pub unsafe extern "C" fn mmtk_post_alloc( // TODO: Replace with buffered mmtk_add_obj_free_candidates #[no_mangle] -pub extern "C" fn mmtk_add_obj_free_candidate(object: ObjectReference) { - binding().weak_proc.add_obj_free_candidate(object) +pub extern "C" fn mmtk_add_obj_free_candidate(object: ObjectReference, can_parallel_free: bool) { + binding() + .weak_proc + .add_obj_free_candidate(object, can_parallel_free) } // =============== Weak references =============== diff --git a/gc/mmtk/src/weak_proc.rs b/gc/mmtk/src/weak_proc.rs index c8ad944633bd3a..19dc6a0ee1501a 100644 --- a/gc/mmtk/src/weak_proc.rs +++ b/gc/mmtk/src/weak_proc.rs @@ -1,3 +1,5 @@ +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; use std::sync::Mutex; use mmtk::scheduler::GCWork; @@ -11,10 +13,13 @@ use crate::upcalls; use crate::Ruby; pub struct WeakProcessor { + non_parallel_obj_free_candidates: Mutex>, + parallel_obj_free_candidates: Vec>>, + parallel_obj_free_candidates_counter: AtomicUsize, + /// Objects that needs `obj_free` called when dying. /// If it is a bottleneck, replace it with a lock-free data structure, /// or add candidates in batch. - obj_free_candidates: Mutex>, weak_references: Mutex>, } @@ -27,32 +32,59 @@ impl Default for WeakProcessor { impl WeakProcessor { pub fn new() -> Self { Self { - obj_free_candidates: Mutex::new(Vec::new()), + non_parallel_obj_free_candidates: Mutex::new(Vec::new()), + parallel_obj_free_candidates: vec![Mutex::new(Vec::new())], + parallel_obj_free_candidates_counter: AtomicUsize::new(0), weak_references: Mutex::new(Vec::new()), } } - /// Add an object as a candidate for `obj_free`. - /// - /// Multiple mutators can call it concurrently, so it has `&self`. - pub fn add_obj_free_candidate(&self, object: ObjectReference) { - let mut obj_free_candidates = self.obj_free_candidates.lock().unwrap(); - obj_free_candidates.push(object); + pub fn init_parallel_obj_free_candidates(&mut self, num_workers: usize) { + debug_assert_eq!(self.parallel_obj_free_candidates.len(), 1); + + for _ in 1..num_workers { + self.parallel_obj_free_candidates + .push(Mutex::new(Vec::new())); + } } - /// Add many objects as candidates for `obj_free`. + /// Add an object as a candidate for `obj_free`. /// /// Multiple mutators can call it concurrently, so it has `&self`. - pub fn add_obj_free_candidates(&self, objects: &[ObjectReference]) { - let mut obj_free_candidates = self.obj_free_candidates.lock().unwrap(); - for object in objects.iter().copied() { - obj_free_candidates.push(object); + pub fn add_obj_free_candidate(&self, object: ObjectReference, can_parallel_free: bool) { + if can_parallel_free { + // Newly allocated objects are placed in parallel_obj_free_candidates using + // round-robin. This may not be ideal for load balancing. + let idx = self + .parallel_obj_free_candidates_counter + .fetch_add(1, Ordering::Relaxed) + % self.parallel_obj_free_candidates.len(); + + self.parallel_obj_free_candidates[idx] + .lock() + .unwrap() + .push(object); + } else { + self.non_parallel_obj_free_candidates + .lock() + .unwrap() + .push(object); } } pub fn get_all_obj_free_candidates(&self) -> Vec { - let mut obj_free_candidates = self.obj_free_candidates.lock().unwrap(); - std::mem::take(obj_free_candidates.as_mut()) + // let mut obj_free_candidates = self.obj_free_candidates.lock().unwrap(); + let mut all_obj_free_candidates = self + .non_parallel_obj_free_candidates + .lock() + .unwrap() + .to_vec(); + + for candidates_mutex in &self.parallel_obj_free_candidates { + all_obj_free_candidates.extend(candidates_mutex.lock().unwrap().to_vec()); + } + + std::mem::take(all_obj_free_candidates.as_mut()) } pub fn add_weak_reference(&self, object: ObjectReference) { @@ -65,7 +97,18 @@ impl WeakProcessor { worker: &mut GCWorker, _tracer_context: impl ObjectTracerContext, ) { - worker.add_work(WorkBucketStage::VMRefClosure, ProcessObjFreeCandidates); + worker.add_work( + WorkBucketStage::VMRefClosure, + ProcessNonParallelObjFreeCanadidates {}, + ); + + for index in 0..self.parallel_obj_free_candidates.len() { + worker.add_work( + WorkBucketStage::VMRefClosure, + ProcessParallelObjFreeCandidates { index }, + ); + } + worker.add_work(WorkBucketStage::VMRefClosure, ProcessWeakReferences); worker.add_work(WorkBucketStage::Prepare, UpdateFinalizerObjIdTables); @@ -82,36 +125,50 @@ impl WeakProcessor { } } -struct ProcessObjFreeCandidates; +fn process_obj_free_candidates(obj_free_candidates: &mut Vec) { + // Process obj_free + let mut new_candidates = Vec::new(); + + for object in obj_free_candidates.iter().copied() { + if object.is_reachable() { + // Forward and add back to the candidate list. + let new_object = object.forward(); + trace!("Forwarding obj_free candidate: {object} -> {new_object}"); + new_candidates.push(new_object); + } else { + (upcalls().call_obj_free)(object); + } + } + + *obj_free_candidates = new_candidates; +} + +struct ProcessParallelObjFreeCandidates { + index: usize, +} -impl GCWork for ProcessObjFreeCandidates { +impl GCWork for ProcessParallelObjFreeCandidates { fn do_work(&mut self, _worker: &mut GCWorker, _mmtk: &'static mmtk::MMTK) { - // If it blocks, it is a bug. - let mut obj_free_candidates = crate::binding() - .weak_proc - .obj_free_candidates + let mut obj_free_candidates = crate::binding().weak_proc.parallel_obj_free_candidates + [self.index] .try_lock() - .expect("It's GC time. No mutators should hold this lock at this time."); - - let n_cands = obj_free_candidates.len(); + .expect("Lock for parallel_obj_free_candidates should not be held"); - debug!("Total: {n_cands} candidates"); + process_obj_free_candidates(&mut obj_free_candidates); + } +} - // Process obj_free - let mut new_candidates = Vec::new(); +struct ProcessNonParallelObjFreeCanadidates; - for object in obj_free_candidates.iter().copied() { - if object.is_reachable() { - // Forward and add back to the candidate list. - let new_object = object.forward(); - trace!("Forwarding obj_free candidate: {object} -> {new_object}"); - new_candidates.push(new_object); - } else { - (upcalls().call_obj_free)(object); - } - } +impl GCWork for ProcessNonParallelObjFreeCanadidates { + fn do_work(&mut self, _worker: &mut GCWorker, _mmtk: &'static mmtk::MMTK) { + let mut obj_free_candidates = crate::binding() + .weak_proc + .non_parallel_obj_free_candidates + .try_lock() + .expect("Lock for non_parallel_obj_free_candidates should not be held"); - *obj_free_candidates = new_candidates; + process_obj_free_candidates(&mut obj_free_candidates); } } From d8d41d7441aabae5e5948f89cd759bbdc3bf5532 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sun, 21 Dec 2025 23:14:22 +0900 Subject: [PATCH 2/3] [DOC] Use `Ruby::Box#require_relative` in box.md examples Based on the example, it appears that `foo.rb` and `main.rb` are expected to be in the same directory. Since Ruby 1.9, the current directory is not included in `$LOAD_PATH` by default. As a result, running `box.require('foo')` as shown in the sample code raises a `LoadError`: ```console main.rb:2:in `Ruby::Box#require': cannot load such file -- foo (LoadError) from main.rb:2:in `
' ``` To avoid this, it seems simplest to show either `box.require('./foo')` or `box.require_relative('foo')`. In this PR, `box.require('foo')` is replaced with `box.require_relative('foo')` to make the intention of using a relative path explicit. This should reduce the chance that users trying Ruby Box will run into an unexpected error. --- doc/language/box.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/language/box.md b/doc/language/box.md index f2b59e5b39cafe..4bbb70e52914f1 100644 --- a/doc/language/box.md +++ b/doc/language/box.md @@ -117,7 +117,7 @@ Foo.foo.blank? #=> false # in main.rb box = Ruby::Box.new -box.require('foo') +box.require_relative('foo') box::Foo.foo_is_blank? #=> false (#blank? called in box) @@ -151,7 +151,7 @@ end # main.rb box = Ruby::Box.new -box.require('foo') +box.require_relative('foo') box::String.foo # NoMethodError ``` @@ -174,7 +174,7 @@ Array.const_get(:V) #=> "FOO" # main.rb box = Ruby::Box.new -box.require('foo') +box.require_relative('foo') Array.instance_variable_get(:@v) #=> nil Array.class_variable_get(:@@v) # NameError @@ -197,7 +197,7 @@ p $foo #=> nil p $VERBOSE #=> false box = Ruby::Box.new -box.require('foo') # "This appears: 'foo'" +box.require_relative('foo') # "This appears: 'foo'" p $foo #=> nil p $VERBOSE #=> false @@ -216,7 +216,7 @@ Object::FOO #=> 100 # main.rb box = Ruby::Box.new -box.require('foo') +box.require_relative('foo') box::FOO #=> 100 @@ -241,7 +241,7 @@ yay #=> "foo" # main.rb box = Ruby::Box.new -box.require('foo') +box.require_relative('foo') box::Foo.say #=> "foo" From ca0fece56a3ddfa6b9ec9cf1652e414929040614 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sun, 21 Dec 2025 04:47:55 +0900 Subject: [PATCH 3/3] [DOC] Tweak an example in language/box.md Although the example code comments indicate that it returns `false`, a non-matching result for `=~` is actually `nil`. ```ruby Foo.foo.blank? #=> false "foo".blank? #=> false ``` https://github.com/ruby/ruby/blob/v4.0.0-preview3/doc/language/box.md?plain=1#L115-L122 This PR replaces `=~` with `match?` so that it returns the expected `false`. Since this makes the result a boolean, it also aligns with the expected behavior of a predicate method name like `blank?`. --- doc/language/box.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/language/box.md b/doc/language/box.md index 4bbb70e52914f1..05c0dad985cc5b 100644 --- a/doc/language/box.md +++ b/doc/language/box.md @@ -100,7 +100,7 @@ The changed definitions are visible only in the box. In other boxes, builtin cla class String BLANK_PATTERN = /\A\s*\z/ def blank? - self =~ BLANK_PATTERN + self.match?(BLANK_PATTERN) end end