-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collect live bytes during GC #768
Changes from all commits
622ac80
18f677f
845a8e7
9218e23
fd80eb8
21284ab
76dc0fc
b772d27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ use atomic::Atomic; | |
use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut}; | ||
use crossbeam::deque::{self, Stealer}; | ||
use crossbeam::queue::ArrayQueue; | ||
#[cfg(feature = "count_live_bytes_in_gc")] | ||
use std::sync::atomic::AtomicUsize; | ||
use std::sync::atomic::Ordering; | ||
use std::sync::{Arc, Condvar, Mutex}; | ||
|
||
|
@@ -30,6 +32,11 @@ pub fn current_worker_ordinal() -> Option<ThreadId> { | |
pub struct GCWorkerShared<VM: VMBinding> { | ||
/// Worker-local statistics data. | ||
stat: AtomicRefCell<WorkerLocalStat<VM>>, | ||
/// Accumulated bytes for live objects in this GC. When each worker scans | ||
/// 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need atomic for this? It's worker-local right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be any stage after the transitive closure stages (including weak reference processing stages). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah. It need at least the same level synchronization as the current code. And the counting code is scattered to each policy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @qinsoon Sorry about the typo. I meant |
||
/// A queue of GCWork that can only be processed by the owned thread. | ||
/// | ||
/// Note: Currently, designated work cannot be added from the GC controller thread, or | ||
|
@@ -44,10 +51,22 @@ impl<VM: VMBinding> GCWorkerShared<VM> { | |
pub fn new(stealer: Option<Stealer<Box<dyn GCWork<VM>>>>) -> Self { | ||
Self { | ||
stat: Default::default(), | ||
#[cfg(feature = "count_live_bytes_in_gc")] | ||
live_bytes: AtomicUsize::new(0), | ||
designated_work: ArrayQueue::new(16), | ||
stealer, | ||
} | ||
} | ||
|
||
#[cfg(feature = "count_live_bytes_in_gc")] | ||
pub(crate) fn increase_live_bytes(&self, bytes: usize) { | ||
self.live_bytes.fetch_add(bytes, Ordering::Relaxed); | ||
} | ||
|
||
#[cfg(feature = "count_live_bytes_in_gc")] | ||
pub(crate) fn get_and_clear_live_bytes(&self) -> usize { | ||
self.live_bytes.swap(0, Ordering::SeqCst) | ||
} | ||
} | ||
|
||
/// Used to synchronize mutually exclusive operations between workers and controller, | ||
|
@@ -427,4 +446,12 @@ impl<VM: VMBinding> WorkerGroup<VM> { | |
.iter() | ||
.any(|w| !w.designated_work.is_empty()) | ||
} | ||
|
||
#[cfg(feature = "count_live_bytes_in_gc")] | ||
pub fn get_and_clear_worker_live_bytes(&self) -> usize { | ||
self.workers_shared | ||
.iter() | ||
.map(|w| w.get_and_clear_live_bytes()) | ||
.sum() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer needs to be
pub(crate)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need this. When we do
worker.shared.increase_live_bytes()
inScanObjectsWork::do_work_common()
, we already createdObjectsClosure
which takes&mut GCWorker
. We can't useworker
directly, and we have to use it throughObjectsClosure
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry. I didn't notice that.