Skip to content

Commit 9c513d0

Browse files
committed
Fix issues in sanity GC (mmtk#1079)
This PR fixes two issues in sanity GC. * It fixes mmtk#1078 by removing the code to prepare/release mutator/collector, which seems unnecessary. * It replaces the use of `ScanObjects` for cached root nodes with `ProcessRootNode`. Otherwise, the assertion `assert!(!self.roots())` will fail in `ScanObjectsWork::do_work_common`.
1 parent f304aa8 commit 9c513d0

File tree

5 files changed

+33
-82
lines changed

5 files changed

+33
-82
lines changed

.github/workflows/merge-check.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,3 +25,9 @@ jobs:
2525
checkInterval: 600
2626
env:
2727
GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
28+
- name: Check result
29+
if: ${{ steps.waitforstatuschecks.outputs.status != 'success' }}
30+
uses: actions/github-script@v3
31+
with:
32+
script: |
33+
core.setFailed('Status checks failed')

docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,8 @@ impl<VM: VMBinding> ProcessEdgesWork for MyGCProcessEdges<VM> {
7878
}
7979
}
8080

81-
fn create_scan_work(&self, nodes: Vec<ObjectReference>, roots: bool) -> ScanObjects<Self> {
82-
ScanObjects::<Self>::new(nodes, false, roots, self.bucket)
81+
fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> ScanObjects<Self> {
82+
ScanObjects::<Self>::new(nodes, false, self.bucket)
8383
}
8484
}
8585
// ANCHOR_END: mygc_process_edges_impl

src/plan/generational/gc_work.rs

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,12 +57,8 @@ impl<VM: VMBinding, P: GenerationalPlanExt<VM> + PlanTraceObject<VM>> ProcessEdg
5757
}
5858
}
5959

60-
fn create_scan_work(
61-
&self,
62-
nodes: Vec<ObjectReference>,
63-
roots: bool,
64-
) -> Self::ScanObjectsWorkType {
65-
PlanScanObjects::new(self.plan, nodes, false, roots, self.bucket)
60+
fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
61+
PlanScanObjects::new(self.plan, nodes, false, self.bucket)
6662
}
6763
}
6864

@@ -122,7 +118,7 @@ impl<E: ProcessEdgesWork> GCWork<E::VM> for ProcessModBuf<E> {
122118
// Scan objects in the modbuf and forward pointers
123119
let modbuf = std::mem::take(&mut self.modbuf);
124120
GCWork::do_work(
125-
&mut ScanObjects::<E>::new(modbuf, false, false, WorkBucketStage::Closure),
121+
&mut ScanObjects::<E>::new(modbuf, false, WorkBucketStage::Closure),
126122
worker,
127123
mmtk,
128124
)

src/scheduler/gc_work.rs

Lines changed: 17 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ impl<E: ProcessEdgesWork> ProcessEdgesWorkTracer<E> {
302302
fn flush(&mut self) {
303303
let next_nodes = self.process_edges_work.pop_nodes();
304304
assert!(!next_nodes.is_empty());
305-
let work_packet = self.process_edges_work.create_scan_work(next_nodes, false);
305+
let work_packet = self.process_edges_work.create_scan_work(next_nodes);
306306
let worker = self.process_edges_work.worker();
307307
worker.scheduler().work_buckets[self.stage].add(work_packet);
308308
}
@@ -647,18 +647,14 @@ pub trait ProcessEdgesWork:
647647
///
648648
/// `roots` indicates if we are creating a packet for root scanning. It is only true when this
649649
/// method is called to handle `RootsWorkFactory::create_process_pinning_roots_work`.
650-
fn create_scan_work(
651-
&self,
652-
nodes: Vec<ObjectReference>,
653-
roots: bool,
654-
) -> Self::ScanObjectsWorkType;
650+
fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType;
655651

656652
/// Flush the nodes in ProcessEdgesBase, and create a ScanObjects work packet for it. If the node set is empty,
657653
/// this method will simply return with no work packet created.
658654
fn flush(&mut self) {
659655
let nodes = self.pop_nodes();
660656
if !nodes.is_empty() {
661-
self.start_or_dispatch_scan_work(self.create_scan_work(nodes, false));
657+
self.start_or_dispatch_scan_work(self.create_scan_work(nodes));
662658
}
663659
}
664660

@@ -738,8 +734,8 @@ impl<VM: VMBinding> ProcessEdgesWork for SFTProcessEdges<VM> {
738734
sft.sft_trace_object(&mut self.base.nodes, object, worker)
739735
}
740736

741-
fn create_scan_work(&self, nodes: Vec<ObjectReference>, roots: bool) -> ScanObjects<Self> {
742-
ScanObjects::<Self>::new(nodes, false, roots, self.bucket)
737+
fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> ScanObjects<Self> {
738+
ScanObjects::<Self>::new(nodes, false, self.bucket)
743739
}
744740
}
745741
pub(crate) struct ProcessEdgesWorkRootsWorkFactory<
@@ -821,10 +817,6 @@ pub trait ScanObjectsWork<VM: VMBinding>: GCWork<VM> + Sized {
821817
/// The associated ProcessEdgesWork for processing the edges of the objects in this packet.
822818
type E: ProcessEdgesWork<VM = VM>;
823819

824-
/// Return true if the objects in this packet are pointed by roots, in which case we need to
825-
/// call trace_object on them.
826-
fn roots(&self) -> bool;
827-
828820
/// Called after each object is scanned.
829821
fn post_scan_object(&self, object: ObjectReference);
830822

@@ -842,7 +834,6 @@ pub trait ScanObjectsWork<VM: VMBinding>: GCWork<VM> + Sized {
842834
_mmtk: &'static MMTK<<Self::E as ProcessEdgesWork>::VM>,
843835
) {
844836
let tls = worker.tls;
845-
debug_assert!(!self.roots());
846837

847838
// Scan the nodes in the buffer.
848839
let objects_to_scan = buffer;
@@ -914,22 +905,15 @@ pub struct ScanObjects<Edges: ProcessEdgesWork> {
914905
buffer: Vec<ObjectReference>,
915906
#[allow(unused)]
916907
concurrent: bool,
917-
roots: bool,
918908
phantom: PhantomData<Edges>,
919909
bucket: WorkBucketStage,
920910
}
921911

922912
impl<Edges: ProcessEdgesWork> ScanObjects<Edges> {
923-
pub fn new(
924-
buffer: Vec<ObjectReference>,
925-
concurrent: bool,
926-
roots: bool,
927-
bucket: WorkBucketStage,
928-
) -> Self {
913+
pub fn new(buffer: Vec<ObjectReference>, concurrent: bool, bucket: WorkBucketStage) -> Self {
929914
Self {
930915
buffer,
931916
concurrent,
932-
roots,
933917
phantom: PhantomData,
934918
bucket,
935919
}
@@ -939,10 +923,6 @@ impl<Edges: ProcessEdgesWork> ScanObjects<Edges> {
939923
impl<VM: VMBinding, E: ProcessEdgesWork<VM = VM>> ScanObjectsWork<VM> for ScanObjects<E> {
940924
type E = E;
941925

942-
fn roots(&self) -> bool {
943-
self.roots
944-
}
945-
946926
fn get_bucket(&self) -> WorkBucketStage {
947927
self.bucket
948928
}
@@ -952,7 +932,7 @@ impl<VM: VMBinding, E: ProcessEdgesWork<VM = VM>> ScanObjectsWork<VM> for ScanOb
952932
}
953933

954934
fn make_another(&self, buffer: Vec<ObjectReference>) -> Self {
955-
Self::new(buffer, self.concurrent, false, self.bucket)
935+
Self::new(buffer, self.concurrent, self.bucket)
956936
}
957937
}
958938

@@ -997,12 +977,8 @@ impl<VM: VMBinding, P: PlanTraceObject<VM> + Plan<VM = VM>, const KIND: TraceKin
997977
Self { plan, base }
998978
}
999979

1000-
fn create_scan_work(
1001-
&self,
1002-
nodes: Vec<ObjectReference>,
1003-
roots: bool,
1004-
) -> Self::ScanObjectsWorkType {
1005-
PlanScanObjects::<Self, P>::new(self.plan, nodes, false, roots, self.bucket)
980+
fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
981+
PlanScanObjects::<Self, P>::new(self.plan, nodes, false, self.bucket)
1006982
}
1007983

1008984
fn trace_object(&mut self, object: ObjectReference) -> ObjectReference {
@@ -1051,7 +1027,6 @@ pub struct PlanScanObjects<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceO
10511027
buffer: Vec<ObjectReference>,
10521028
#[allow(dead_code)]
10531029
concurrent: bool,
1054-
roots: bool,
10551030
phantom: PhantomData<E>,
10561031
bucket: WorkBucketStage,
10571032
}
@@ -1061,14 +1036,12 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> PlanScan
10611036
plan: &'static P,
10621037
buffer: Vec<ObjectReference>,
10631038
concurrent: bool,
1064-
roots: bool,
10651039
bucket: WorkBucketStage,
10661040
) -> Self {
10671041
Self {
10681042
plan,
10691043
buffer,
10701044
concurrent,
1071-
roots,
10721045
phantom: PhantomData,
10731046
bucket,
10741047
}
@@ -1080,10 +1053,6 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> ScanObje
10801053
{
10811054
type E = E;
10821055

1083-
fn roots(&self) -> bool {
1084-
self.roots
1085-
}
1086-
10871056
fn get_bucket(&self) -> WorkBucketStage {
10881057
self.bucket
10891058
}
@@ -1093,7 +1062,7 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> ScanObje
10931062
}
10941063

10951064
fn make_another(&self, buffer: Vec<ObjectReference>) -> Self {
1096-
Self::new(self.plan, buffer, self.concurrent, false, self.bucket)
1065+
Self::new(self.plan, buffer, self.concurrent, self.bucket)
10971066
}
10981067
}
10991068

@@ -1111,7 +1080,11 @@ impl<E: ProcessEdgesWork, P: Plan<VM = E::VM> + PlanTraceObject<E::VM>> GCWork<E
11111080
/// but creates the work to scan these objects using E. This is necessary to guarantee that these objects do not move
11121081
/// (`I` should trace them without moving) as we do not have the information about the edges pointing to them.
11131082
1114-
struct ProcessRootNode<VM: VMBinding, I: ProcessEdgesWork<VM = VM>, E: ProcessEdgesWork<VM = VM>> {
1083+
pub(crate) struct ProcessRootNode<
1084+
VM: VMBinding,
1085+
I: ProcessEdgesWork<VM = VM>,
1086+
E: ProcessEdgesWork<VM = VM>,
1087+
> {
11151088
phantom: PhantomData<(VM, I, E)>,
11161089
roots: Vec<ObjectReference>,
11171090
bucket: WorkBucketStage,
@@ -1175,7 +1148,7 @@ impl<VM: VMBinding, I: ProcessEdgesWork<VM = VM>, E: ProcessEdgesWork<VM = VM>>
11751148
};
11761149

11771150
let process_edges_work = E::new(vec![], false, mmtk, self.bucket);
1178-
let work = process_edges_work.create_scan_work(scanned_root_objects, false);
1151+
let work = process_edges_work.create_scan_work(scanned_root_objects);
11791152
crate::memory_manager::add_work_packet(mmtk, self.bucket, work);
11801153

11811154
trace!("ProcessRootNode End");
@@ -1220,11 +1193,7 @@ impl<VM: VMBinding> ProcessEdgesWork for UnsupportedProcessEdges<VM> {
12201193
panic!("unsupported!")
12211194
}
12221195

1223-
fn create_scan_work(
1224-
&self,
1225-
_nodes: Vec<ObjectReference>,
1226-
_roots: bool,
1227-
) -> Self::ScanObjectsWorkType {
1196+
fn create_scan_work(&self, _nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
12281197
panic!("unsupported!")
12291198
}
12301199
}

src/util/sanity/sanity_checker.rs

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,12 @@ impl<P: Plan> GCWork<P::VM> for ScheduleSanityGC<P> {
9696
);
9797
}
9898
for roots in &sanity_checker.root_nodes {
99-
scheduler.work_buckets[WorkBucketStage::Closure].add(ScanObjects::<
99+
scheduler.work_buckets[WorkBucketStage::Closure].add(ProcessRootNode::<
100+
P::VM,
101+
SanityGCProcessEdges<P::VM>,
100102
SanityGCProcessEdges<P::VM>,
101103
>::new(
102104
roots.clone(),
103-
false,
104-
true,
105105
WorkBucketStage::Closure,
106106
));
107107
}
@@ -132,14 +132,6 @@ impl<P: Plan> GCWork<P::VM> for SanityPrepare<P> {
132132
let mut sanity_checker = mmtk.sanity_checker.lock().unwrap();
133133
sanity_checker.refs.clear();
134134
}
135-
for mutator in <P::VM as VMBinding>::VMActivePlan::mutators() {
136-
mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
137-
.add(PrepareMutator::<P::VM>::new(mutator));
138-
}
139-
for w in &mmtk.scheduler.worker_group.workers_shared {
140-
let result = w.designated_work.push(Box::new(PrepareCollector));
141-
debug_assert!(result.is_ok());
142-
}
143135
}
144136
}
145137

@@ -157,14 +149,6 @@ impl<P: Plan> GCWork<P::VM> for SanityRelease<P> {
157149
fn do_work(&mut self, _worker: &mut GCWorker<P::VM>, mmtk: &'static MMTK<P::VM>) {
158150
info!("Sanity GC release");
159151
mmtk.sanity_checker.lock().unwrap().clear_roots_cache();
160-
for mutator in <P::VM as VMBinding>::VMActivePlan::mutators() {
161-
mmtk.scheduler.work_buckets[WorkBucketStage::Release]
162-
.add(ReleaseMutator::<P::VM>::new(mutator));
163-
}
164-
for w in &mmtk.scheduler.worker_group.workers_shared {
165-
let result = w.designated_work.push(Box::new(ReleaseCollector));
166-
debug_assert!(result.is_ok());
167-
}
168152
mmtk.sanity_end();
169153
}
170154
}
@@ -243,11 +227,7 @@ impl<VM: VMBinding> ProcessEdgesWork for SanityGCProcessEdges<VM> {
243227
object
244228
}
245229

246-
fn create_scan_work(
247-
&self,
248-
nodes: Vec<ObjectReference>,
249-
roots: bool,
250-
) -> Self::ScanObjectsWorkType {
251-
ScanObjects::<Self>::new(nodes, false, roots, WorkBucketStage::Closure)
230+
fn create_scan_work(&self, nodes: Vec<ObjectReference>) -> Self::ScanObjectsWorkType {
231+
ScanObjects::<Self>::new(nodes, false, WorkBucketStage::Closure)
252232
}
253233
}

0 commit comments

Comments
 (0)