Refactor the collector to use reference counting #74

Others · 2021-11-28T23:42:06Z

This is around a ~4x speedup, but required a lot of code changes.
In terms of public API, only AtomicGc has a significant diff.
(The old API would still work, but the changes make it work
more efficently with the new internals.)

Others · 2021-11-28T23:42:56Z

Cargo.toml

+[profile.release]
+debug = true


This needs commented back out

Others · 2021-11-28T23:44:36Z

src/collector/dropper.rs


 use crate::collector::GcData;
+use crate::concurrency::cross_thread_buffer::CrossThreadBuffer;
+
+type DropBuffer = CrossThreadBuffer<Arc<GcData>>;

 pub(crate) struct BackgroundDropper {


The changes in here are pure premature optimization. But I think it is better than the previous design

jrmuizel · 2021-11-29T14:32:07Z

A 4x speed up on what?

Others · 2021-11-30T06:07:37Z

@jrmuizel a 4x speedup on the stress test I use to benchmark the collector:
https://github.com/Others/shredder/blob/master/tests/stress.rs
On my machine it goes from ~12 to ~3 seconds.

This benchmark kinda blows, and I've had an issue to create new ones for a while (#7). But I'm pretty sure that this new design is significantly better on a number of axises anyway, as it removes the dumb parts of the collector where we keep a list of every Gc in the program.

This is around a ~4x speedup, but required a lot of code changes. In terms of public API, only `AtomicGc` has a significant diff. (The old API would still work, but the changes make it work more efficently with the new internals.)

Others · 2021-12-01T00:06:55Z

See #74

pcodes

Probably not the most thorough/helpful for critical feedback, but was able to as a bunch of questions 😄

pcodes · 2022-01-02T17:53:52Z

src/atomic.rs

+    //
+    // Taken together, this means that this pointer is always valid when executing a method on
+    // `AtomicGc`.
+    atomic_ptr: AtomicPtr<GcData>,


Not sure I fully follow this change- why is atomic_ptr no longer an arc? Did the underlying assumption about what the GcData is change?

I've changed the semantics of AtomicGc to not be sharable by default. Now you need to do DerefGc<AtomicGc<T>> if you want to share it. This is much more flexible, and equivalently performant.

pcodes · 2022-01-02T17:54:11Z

src/atomic.rs

 ///
 /// `AtomicGc` should be fairly fast, but you may not assume it does not block. In fact in the
 /// presence of an active garbage collection operation, all operations will block. Otherwise
 /// it shouldn't block.
-#[derive(Clone, Debug)]
+#[derive(Debug)]


Why did you remove the Clone trait?

Because the auto-derive doesn't work anymore, due to the changing type of atomic_ptr. Will add a manual impl

pcodes · 2022-01-02T17:57:03Z

src/atomic.rs

-        let data_ptr = Arc::as_ptr(data_arc);
-
-        let atomic_ptr = Arc::new(AtomicPtr::new(data_ptr as _));
+        let atomic_ptr = AtomicPtr::new(Arc::as_ptr(data_arc) as _);


What does the as _ mean? My rust is (very) rusty :P

"cast this to the right type but don't make me type out what type I need"

pcodes · 2022-01-02T17:57:44Z

src/atomic.rs

-        let atomic_ptr = Arc::new(AtomicPtr::new(data_ptr as _));
+        let atomic_ptr = AtomicPtr::new(Arc::as_ptr(data_arc) as _);
+        // Forget the initial data, we will absorb its reference counts
+        data.drop_preserving_reference_counts();


Why are you dropping the initial data?

Basically this method turns a Gc into an AtomicGc, and we need to get rid of the old Gc to do that. Gc is reference counted, but instead of incrementing and then decrementing, we can just absorb the reference count. This idea is used throughout this file

pcodes · 2022-01-02T18:11:16Z

src/atomic.rs

-            ptr = gc_data.scan_ptr().cast();
-            internal_handle = COLLECTOR.handle_from_data(gc_data);
+            // The count of the data going out decreases
+            COLLECTOR.decrement_reference_count(&old_arc);


Does the count decrease because you've absorbed the other pointer?

pcodes · 2022-01-02T18:29:47Z

src/collector/dropper.rs

+
+                        // Then clear and recycle the buffer
+                        to_drop.clear();
+                        // ignore recycling failures


why can you ignore them?

Because if the channel is full we don't want this whole thread to block indefinitely

pcodes · 2022-01-02T18:37:00Z

src/collector/dropper.rs

    /// Indicates to the `BackgroundDropper` that it should sync up with the calling code
    SyncUp(Sender<()>),
 }

 impl BackgroundDropper {
+    const RECYCLING_CHANNEL_SIZE: usize = 1;


Can you summarize the changes you've made here? I don't understand the overarching idea

Basically this class has learned how to recycle it's buffers so the collector can get them back (rather than allocating a big buffer every time the collector runs)

pcodes · 2022-01-02T18:48:48Z

src/collector/mod.rs

+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub(crate) enum RefCountPolicy {
+    // A transient handle doesn't increment or decrement the reference count
+    TransientHandle,


What is the point of this?

It's used in the Scan implementations of AtomicGc / DerefGc to create handles we can report to the collector without having them mess up our reference counts

pcodes · 2022-01-02T19:07:23Z

src/collector/ref_cnt.rs

+    // `total_handles` = count_positive + count_negative
+    // `count_positive` is always >= the actual count >= 0
+    // At the beginning of every collection we recalculate `count_positive`
+    count_positive: AtomicI64,
+    // `count_negative` is always <= 0
+    count_negative: AtomicI64,


Can you clarify this?

Not a specific enough question. Basically this is a complex way of letting count_positive always be equal to or greater than the real count . The reason why it is split is so that threads can decrement without using a lock

pcodes · 2022-01-02T19:11:11Z

src/concurrency/cross_thread_buffer.rs

+use std::cell::RefCell;
+use thread_local::ThreadLocal;
+
+pub(crate) struct CrossThreadBuffer<T: Send> {


What is this used for? (not sure if I missed its usage somewhere else in the PR)

It's used for the buffer that stores dead data in the collector

Others · 2022-01-22T23:15:54Z

src/atomic.rs

 ///
 /// `AtomicGc` should be fairly fast, but you may not assume it does not block. In fact in the
 /// presence of an active garbage collection operation, all operations will block. Otherwise
 /// it shouldn't block.
-#[derive(Clone, Debug)]
+#[derive(Debug)]


Because the auto-derive doesn't work anymore, due to the changing type of atomic_ptr. Will add a manual impl

Others · 2022-01-22T23:21:31Z

src/atomic.rs

+    //
+    // Taken together, this means that this pointer is always valid when executing a method on
+    // `AtomicGc`.
+    atomic_ptr: AtomicPtr<GcData>,


I've changed the semantics of AtomicGc to not be sharable by default. Now you need to do DerefGc<AtomicGc<T>> if you want to share it. This is much more flexible, and equivalently performant.

Others · 2022-01-22T23:21:51Z

src/atomic.rs

-        let data_ptr = Arc::as_ptr(data_arc);
-
-        let atomic_ptr = Arc::new(AtomicPtr::new(data_ptr as _));
+        let atomic_ptr = AtomicPtr::new(Arc::as_ptr(data_arc) as _);


"cast this to the right type but don't make me type out what type I need"

Others · 2022-01-22T23:25:41Z

src/atomic.rs

-        let atomic_ptr = Arc::new(AtomicPtr::new(data_ptr as _));
+        let atomic_ptr = AtomicPtr::new(Arc::as_ptr(data_arc) as _);
+        // Forget the initial data, we will absorb its reference counts
+        data.drop_preserving_reference_counts();


Basically this method turns a Gc into an AtomicGc, and we need to get rid of the old Gc to do that. Gc is reference counted, but instead of incrementing and then decrementing, we can just absorb the reference count. This idea is used throughout this file

Others · 2022-01-22T23:28:24Z

src/atomic.rs

+        // No need for collection blocker, as we're not modifying the graph
+        // This is safe. See comment on the `atomic_ptr` field
+        let gc_data_ptr = self.atomic_ptr.load(ordering);


I think this is a data race. What happens if we:

Read the data, but don't increment the count yet (count = 1)

Switch to another thread that overwrites the data and decrements the count

The collector runs and cleans up the "dead" data

The read completes, incrementing the count, but the data is already cleaned up and invalid

Others · 2022-01-22T23:39:47Z

src/collector/dropper.rs

+
+                        // Then clear and recycle the buffer
+                        to_drop.clear();
+                        // ignore recycling failures


Because if the channel is full we don't want this whole thread to block indefinitely

Others · 2022-01-22T23:40:19Z

src/collector/dropper.rs

    /// Indicates to the `BackgroundDropper` that it should sync up with the calling code
    SyncUp(Sender<()>),
 }

 impl BackgroundDropper {
+    const RECYCLING_CHANNEL_SIZE: usize = 1;


Basically this class has learned how to recycle it's buffers so the collector can get them back (rather than allocating a big buffer every time the collector runs)

Others · 2022-01-22T23:41:25Z

src/collector/mod.rs

+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub(crate) enum RefCountPolicy {
+    // A transient handle doesn't increment or decrement the reference count
+    TransientHandle,


It's used in the Scan implementations of AtomicGc / DerefGc to create handles we can report to the collector without having them mess up our reference counts

Others · 2022-01-23T00:19:17Z

src/collector/ref_cnt.rs

+    // `total_handles` = count_positive + count_negative
+    // `count_positive` is always >= the actual count >= 0
+    // At the beginning of every collection we recalculate `count_positive`
+    count_positive: AtomicI64,
+    // `count_negative` is always <= 0
+    count_negative: AtomicI64,


Not a specific enough question. Basically this is a complex way of letting count_positive always be equal to or greater than the real count . The reason why it is split is so that threads can decrement without using a lock

Others · 2022-01-23T00:20:17Z

src/concurrency/cross_thread_buffer.rs

+use std::cell::RefCell;
+use thread_local::ThreadLocal;
+
+pub(crate) struct CrossThreadBuffer<T: Send> {


It's used for the buffer that stores dead data in the collector

Others commented Nov 28, 2021

View reviewed changes

Refactor the collector to use reference counting

3ccd3bb

This is around a ~4x speedup, but required a lot of code changes. In terms of public API, only `AtomicGc` has a significant diff. (The old API would still work, but the changes make it work more efficently with the new internals.)

Others force-pushed the ref_cnt branch from 7bc114e to 3ccd3bb Compare November 30, 2021 07:01

pcodes reviewed Jan 2, 2022

View reviewed changes

Others commented Jan 23, 2022

View reviewed changes

		[profile.release]
		debug = true

Refactor the collector to use reference counting #74

Are you sure you want to change the base?

Refactor the collector to use reference counting #74

Conversation

Others commented Nov 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrmuizel commented Nov 29, 2021

Others commented Nov 30, 2021

Others commented Dec 1, 2021

pcodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment