Refactor global allocator to safer TrackingAllocator
#205
Conversation
| R: Send + 'static, | ||
| { | ||
| self.run(|| { | ||
| trigger_full_gc!(); |
There was a problem hiding this comment.
Is this just for the tracking allocator tests? Couldn't this just be used in those tests?
| for closure in receiver { | ||
| closure(); | ||
| // Wait for the main thread to finish setting up Ruby | ||
| while unsafe { STATE.load(Ordering::Acquire) != 2 } { |
There was a problem hiding this comment.
From the std::sync::Once::call_once docs:
This method will block the calling thread if another initialization routine is currently running.
so there shouldn't be any need for this redundant synchronization.
| @@ -66,7 +77,7 @@ impl RubyTestExecutor { | |||
| F: FnOnce() -> R + Send + 'static, | |||
| R: Send + 'static, | |||
| { | |||
| let (result_sender, result_receiver) = mpsc::sync_channel(1); | |||
| let (result_sender, result_receiver) = mpsc::sync_channel(8); | |||
There was a problem hiding this comment.
Why 8? This channel is only used in this single run call, so it will only have a single send to it.
There was a problem hiding this comment.
sync_channel has some strange behaviors in 1.54, was using this as a way to robustness test. Will remove.
| static INIT: Once = Once::new(); | ||
| static mut STATE: AtomicU8 = AtomicU8::new(0); |
There was a problem hiding this comment.
Why is more synchronization needed now? It looks like moving this initialization code into executor.run(|| { that precedes the executor being returned means that we shouldn't even need the INIT, let alone the extra STATE synchronization
There was a problem hiding this comment.
I was having some major synchronization headaches on Rust 1.54 (but not on stable). I threw everything at the book at it to finally make these tests stable since libruby doesn't always use atomic loads for these values.
My thinking is that since these are test helpers, it's better have too much synchronization than not enough - to avoid flakiness at almost all costs. That being said, planning on circling back to clean up the unnecessary stuff.
| let result = $e; | ||
| let after = unsafe { rb_sys::rb_gc_stat(id) }; | ||
|
|
||
| $crate::trigger_full_gc!(); |
There was a problem hiding this comment.
Shouldn't this be triggered before the first rb_gc_stat?
| pub static CAPTURE_LOCK_INIT: std::sync::Once = std::sync::Once::new(); | ||
| pub static mut CAPTURE_LOCK: Option<std::sync::Mutex<()>> = None; | ||
|
|
||
| CAPTURE_LOCK_INIT.call_once(|| unsafe { | ||
| CAPTURE_LOCK.replace(std::sync::Mutex::new(())); | ||
| }); |
There was a problem hiding this comment.
Won't each use of this macro end up with a separate lock? What is this lock protecting? Isn't #[ruby_test] already keeping multiple ruby tests from running in concurrency?
| let ret = !crate::hidden::ruby_current_vm_ptr.is_null(); | ||
|
|
||
| #[cfg(any(ruby_lte_2_4, ruby_gt_3_2))] | ||
| let ret = crate::rb_cBasicObject != 0; |
There was a problem hiding this comment.
Where does ruby rb_cBasicObject = 0? I only see it getting set in Init_class_hierarchy. Hopefully your upstream PR will make this unnecessary anyways.
There was a problem hiding this comment.
Yeah, this doesn't actually inform us about a shutdown VM, only if it has started or not. Half-measure.
There was a problem hiding this comment.
Also, Ruby doensn't explicitly set it to 0 anywhere, instead we are relying on the fact that the default initialization value of a static variable is zero in C
Previously, it was possible for the previous
RbAllocatorimplementation to deadlock and/or segfault whendeallocwas called after the Ruby VM was shut down. In particular, this can happen when thread local destructors are run, since they are run after the Ruby VM has been shut down. When the Ruby VM is shut down, calls torb_gc_adjust_memory_usagewill fail. So we need to avoid that.Bullet Points
ruby_vm_current_ptrandruby_current_ec) to check the availability of the Ruby VM before attempting to report memory usage.RbAllocatortoTrackingAllocatorto communicate the implementation a bit more accuratelyManuallyTracked<T>type which allows RAII style allocation tracking outside ofstd::system::GlobalAlloc. This is useful when you want to track memory usage from direct calls tommap(2), for example.Caveats
Unfortunately, this solution will not work with Ruby 3.3 since
ruby_current_vm_ptris not longer publicly exported (as it was private to begin with). I've opened an issue on the Ruby bug tracker to add official support for the needed behavior, though. Please chime in on the issue so we can have reliable Rust memory tracking in the next Ruby release!