Sanity GC is broken

[angussidney]

In the latest version of MMTk (mmtk-core commit 42754a5, mmtk-openjdk commit 54f6688fe4), the Sanity GC is broken.

MTK_PLAN=Immix ~/honours/openjdk-builds/20240130-mmtk-slowdebug-42754a5
f2-upstream/bin/java -XX:+UseThirdPartyHeap -Xms512M -Xmx512M -jar /usr/share/benchmarks/dacap
o/dacapo-23.11-chopin.jar lusearch                                                            
[2024-01-30T00:31:21Z INFO  mmtk::memory_manager] Initialized MMTk with Immix (FixedHeapSize(5
36870912))                                                                                    
Using scaled threading model. 24 processors detected, 24 threads used to drive the workload, i
n a possible range of [1,2048]                                                                
Version: lucene 9.7.0 (use -p to print nominal benchmark stats)                               
[2024-01-30T00:31:25Z INFO  mmtk::mmtk] User triggering collection                            
[2024-01-30T00:31:25Z INFO  mmtk::util::sanity::sanity_checker] Sanity GC prepare             
thread 'thread 'thread 'thread 'thread '<unnamed>thread 'thread 'thread '<unnamed><unnamed><un
named><unnamed>' panicked at 'thread '<unnamed><unnamed><unnamed><unnamed>thread '' panicked a
t 'thread '' panicked at 'thread '' panicked at 'thread 'internal error: entered unreachable c
ode: `MutatorConfig::prepare_func` must not be called for the current plan.' panicked at 'thre
ad 'internal error: entered unreachable code: `MutatorConfig::prepare_func` must not be called
 for the current plan.'  

[...]

fatal runtime error: failed to initiate panic, error 
5

Aborted (core dumped)

This is occurring because the sanity GC issues a PrepareMutator work packet for each mutator, which calls the prepare method on the mutator, which in turn calls the prepare_func for that plan. For most plans (with the exception of MarkSweep), this is set to the unreachable_prepare_func stub, which triggers a panic.

[qinsoon]

I think sanity GC should do something like the code below in SanityPrepare

mmtk-core/src/scheduler/gc_work.rs

Lines 62 to 67 in 42754a5

	if plan_mut.constraints().needs_prepare_mutator {
	for mutator in <C::VM as VMBinding>::VMActivePlan::mutators() {
	mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
	.add(PrepareMutator::<C::VM>::new(mutator));
	}
	}

mmtk-core/src/util/sanity/sanity_checker.rs

Lines 135 to 138 in 42754a5

	for mutator in <P::VM as VMBinding>::VMActivePlan::mutators() {
	mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
	.add(PrepareMutator::<P::VM>::new(mutator));
	}

[angussidney]

I guess a broader question is whether it is necessary to schedule the PrepareMutator packet at all during the Sanity GC? prepare_func is only defined for the MarkSweep plan, which uses it to schedule the allocators for lazy sweeping (sweeping is deferred till the release phase if eager sweeping is enabled). If sanity GC is enabled, is it intended behaviour to do this twice?

Similarly, Sanity GC schedules PrepareCollector packets for each thread. This seems to only be used to get the copy contexts, which is only useful if the worker will be copying objects. However, the sanity GC never moves anything, so this is seems to be unnecessary?

I may be misunderstanding the purpose of the PrepareMutator/PrepareCollector packets, but I'm unsure whether it is correct semantics to be scheduling them at the start of the Sanity GC at all.

[angussidney]

As a quick test, I tried removing the following sections of code, and the Sanity GC worked fine on the Immix, SemiSpace, MarkSweep and MarkCompact plans.

https://github.com/mmtk/mmtk-core/blob/master/src/util/sanity/sanity_checker.rs#L135-L142

https://github.com/mmtk/mmtk-core/blob/master/src/util/sanity/sanity_checker.rs#L160-L167

[qinsoon]

As a quick test, I tried removing the following sections of code, and the Sanity GC worked fine on the Immix, SemiSpace, MarkSweep and MarkCompact plans.

https://github.com/mmtk/mmtk-core/blob/master/src/util/sanity/sanity_checker.rs#L135-L142

https://github.com/mmtk/mmtk-core/blob/master/src/util/sanity/sanity_checker.rs#L160-L167

In Java MMTk there isn't mutator/collector prepare/release for sanity GC, and there was no mutator/collector prepare/release when it was ported to Rust: https://github.com/mmtk/mmtk-core/blob/96855d287fe5ea789a532f347d6ee37e6679c71f/src/util/sanity/sanity_checker.rs. @wenyuzhao added those when work packet was introduced: https://github.com/mmtk/mmtk-core/blame/66dbdce4e3b392c260b9f4601764b53379877eed/src/util/sanity/sanity_checker.rs.

@wenyuzhao Do you remember why we would need those code?

[wenyuzhao]

As a quick test, I tried removing the following sections of code, and the Sanity GC worked fine on the Immix, SemiSpace, MarkSweep and MarkCompact plans.
https://github.com/mmtk/mmtk-core/blob/master/src/util/sanity/sanity_checker.rs#L135-L142
https://github.com/mmtk/mmtk-core/blob/master/src/util/sanity/sanity_checker.rs#L160-L167

In Java MMTk there isn't mutator/collector prepare/release for sanity GC, and there was no mutator/collector prepare/release when it was ported to Rust: https://github.com/mmtk/mmtk-core/blob/96855d287fe5ea789a532f347d6ee37e6679c71f/src/util/sanity/sanity_checker.rs. @wenyuzhao added those when work packet was introduced: https://github.com/mmtk/mmtk-core/blame/66dbdce4e3b392c260b9f4601764b53379877eed/src/util/sanity/sanity_checker.rs.

@wenyuzhao Do you remember why we would need those code?

No I think it's not necessary. I probably directly copied this from the normal GC tracing work packets.