Collect live bytes during GC #768

qinsoon · 2023-03-07T06:29:10Z

This PR adds a feature count_live_bytes_in_gc. When enabled, each worker will maintain a counter for live bytes when scanning an object, and in Release, we get live bytes from all the workers, and sum it up. We also provide memory_manager::live_bytes_in_last_gc() for users to query this value. This is usually used to debug fragmentation.

caizixian · 2023-03-08T01:21:00Z

src/scheduler/worker.rs

+    /// objects, we increase the live bytes. We get this value from each worker
+    /// at the end of a GC, and reset this counter.
+    #[cfg(feature = "count_live_bytes_in_gc")]
+    live_bytes: AtomicUsize,


Do we need atomic for this? It's worker-local right?

Do we need atomic for this? It's worker-local right?

It is in the shared part of a worker. We cannot get a mutable reference to it. It needs to be in the shared part, as we need to iterate through all workers, and sum it up.

Ah OK. I was just thinking if we can make it cheap enough by not using atomic for worker local stats, we could enable this feature by default.

Why not make it worker-local and each worker reports the live bytes after their transitive closure? Then we don't have to explicitly iterate through all the workers, we just collate the numbers we get from the transitive closure.

It could be any stage after the transitive closure stages (including weak reference processing stages).

Maybe it's easier to defer the "live byte count" to each policy as opposed to each worker? But then it stops being worker-local so it would need synchronization.

Maybe it's easier to defer the "live byte count" to each policy as opposed to each worker? But then it stops being worker-local so it would need synchronization.

Yeah. It need at least the same level synchronization as the current code. And the counting code is scattered to each policy.

We could use AtomicUsize::as_mut_ptr and do non-atomic add with unsafe code when we do the counting. But I still question if we want to make this enabled by default.

I personally don't have a strong opinion. Keeping track of fragmentation is definitely useful though. If it's possible to do it cheaply for every GC, may as well do it. If it's not possible then I don't think should spend more time on it.

@qinsoon Sorry about the typo. I meant ReleaseCollector. Yes. It will be a problem for MarkCompact. If we make the counter a private field of a Worker, it will require another rendezvous (designated works) between the marking phase and the forwarding phase. Given this situation, I don't mind if we use AtomicUsize for now.

wks · 2023-03-08T02:32:54Z

src/scheduler/gc_work.rs

+    // This Also exposes worker to the callback function. This is not a public method.
+    fn with_tracer_and_worker<R, F>(&self, worker: &mut GCWorker<E::VM>, func: F) -> R
+    where
+        F: FnOnce(&mut ProcessEdgesWorkTracer<E>, &mut GCWorker<E::VM>) -> R,


Strictly speaking, it is unsafe to give the user access to both the tracer and the worker. The tracer is a closure that calls trace_object underneath, and the worker is also one of the arguments of trace_object. I planned to forbid this by adding a lifetime annotation, but I couldn't do it without the General Associated Type (GAT) feature before Rust 1.65.

See:

mmtk-core/src/vm/scanning.rs

Line 53 in 813fa99

/// FIXME: The current code works because of the unsafe method `ProcessEdgesWork::set_worker`.

Another problem with this is that VM bindings can use ObjectTracerContext to get temporary access to trace_object during Scanning::process_weak_refs and Scanning::forward_weak_refs. Adding a with_tracer_and_worker will allow ScanObjects to record object sizes, but not for VM-specific weak reference processing.

with_tracer takes a mutable reference of worker, so I cannot use worker any more in ScanObjects::do_work_common. We need to a way to get the worker back from the tracer. Another option is to expose some methods from ObjectTracer so we can get the worker or directly increase the counter.

Another problem with this is that VM bindings can use ObjectTracerContext to get temporary access to trace_object during Scanning::process_weak_refs and Scanning::forward_weak_refs. Adding a with_tracer_and_worker will allow ScanObjects to record object sizes, but not for VM-specific weak reference processing.

The remaining question is how to count live bytes for weak references. As the weak reference processing is totally at the binding side, we may have to expose a method and let the binding to call it when they scan weak refs.

The remaining question is how to count live bytes for weak references. As the weak reference processing is totally at the binding side, we may have to expose a method and let the binding to call it when they scan weak refs.

If we count the object size at ScanObjectsWork, it will not be a problem. The binding keeps objects alive during the weak reference processing stage using trace_object (via ObjectTracer::trace_object, which is implemented with ProcessEdgesWork::trace_object underneath and eventually gets to the space-specific trace_object). Any object visited by trace_object will eventually be visited by ScanObjectsWork.

wks

We could count the object sizes in one loop instead of splitting the objects into those that supports edge enqueuing and those that don't. By doing this, the with_tracer_and_worker is also unnecessary.

src/scheduler/gc_work.rs

object tracer

wks

Only a small problem remains.

wks · 2023-03-08T06:47:04Z

src/plan/tracing.rs

@@ -74,7 +74,7 @@ impl ObjectQueue for VectorQueue<ObjectReference> {
 /// A transitive closure visitor to collect all the edges of an object.
 pub struct ObjectsClosure<'a, E: ProcessEdgesWork> {
    buffer: VectorQueue<EdgeOf<E>>,
-    worker: &'a mut GCWorker<E::VM>,
+    pub(crate) worker: &'a mut GCWorker<E::VM>,


This no longer needs to be pub(crate).

I think we still need this. When we do worker.shared.increase_live_bytes() in ScanObjectsWork::do_work_common(), we already created ObjectsClosure which takes &mut GCWorker. We can't use worker directly, and we have to use it through ObjectsClosure.

Oh, sorry. I didn't notice that.

wks

LGTM

caizixian · 2023-08-06T08:17:04Z

I think we should merge this. We can also add a tracepoint for this.

qinsoon added 2 commits March 7, 2023 06:10

Collect live bytes during GC.

622ac80

Avoid changing ObjectTracerContext

18f677f

qinsoon marked this pull request as ready for review March 7, 2023 22:10

qinsoon requested a review from wks March 7, 2023 22:10

caizixian reviewed Mar 8, 2023

View reviewed changes

wks reviewed Mar 8, 2023

View reviewed changes

wks requested changes Mar 8, 2023

View reviewed changes

src/scheduler/gc_work.rs Outdated Show resolved Hide resolved

src/scheduler/gc_work.rs Outdated Show resolved Hide resolved

qinsoon added 2 commits March 8, 2023 04:21

Count live bytes at the beginning of the loop. Revert changes about

845a8e7

object tracer

Merge branch 'master' into live-bytes

9218e23

wks requested changes Mar 8, 2023

View reviewed changes

wks approved these changes Mar 9, 2023

View reviewed changes

qinsoon and others added 2 commits May 31, 2023 12:09

Merge branch 'master' into live-bytes

fd80eb8

Merge branch 'master' into live-bytes

21284ab

qinsoon added 2 commits August 8, 2023 01:15

Fix build issue

76dc0fc

Merge branch 'master' into live-bytes

b772d27

qinsoon merged commit cacb8f6 into mmtk:master Aug 8, 2023

qinsoon mentioned this pull request Aug 23, 2023

Incorrect collection_pages calculation in mem balancer #918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect live bytes during GC #768

Collect live bytes during GC #768

qinsoon commented Mar 7, 2023

caizixian Mar 8, 2023

qinsoon Mar 8, 2023 •

edited

Loading

caizixian Mar 8, 2023

k-sareen Mar 8, 2023 •

edited

Loading

wks Mar 8, 2023 •

edited

Loading

k-sareen Mar 8, 2023

qinsoon Mar 8, 2023

qinsoon Mar 8, 2023

k-sareen Mar 8, 2023

wks Mar 8, 2023

wks Mar 8, 2023

wks Mar 8, 2023

qinsoon Mar 8, 2023

qinsoon Mar 8, 2023

wks Mar 8, 2023

wks left a comment

wks left a comment

wks Mar 8, 2023

qinsoon Mar 8, 2023

wks Mar 9, 2023

wks left a comment

caizixian commented Aug 6, 2023

Collect live bytes during GC #768

Collect live bytes during GC #768

Conversation

qinsoon commented Mar 7, 2023

Choose a reason for hiding this comment

qinsoon Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-sareen Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

wks Mar 8, 2023 • edited Loading

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

wks left a comment

Choose a reason for hiding this comment

wks 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

wks left a comment

Choose a reason for hiding this comment

caizixian commented Aug 6, 2023

qinsoon Mar 8, 2023 •

edited

Loading

k-sareen Mar 8, 2023 •

edited

Loading

wks Mar 8, 2023 •

edited

Loading