RFR: 8315247: GenShen: Condition calls to post-write barrier code generation by a flag

[Kelvin Nilsen]

On Mon, 28 Aug 2023 19:48:58 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

> Protect the card barrier code generation by a new global `ShenandoahCardBarrier`, which is disabled by default and enabled for GenShen (generational mode Shenandoah) only. If the user forces the barrier flag to a value inconsistent with mode, we exit with an appropriate error message.
> 
> The changes have been made for x86, aarch, and ppc. I have tested on x86 on a cloud host running jtreg1-4 and SPECjbb. A round of aarch64 and x86 pipeline tests have run (but ran into existing assertion that William is fixing). Will request PPC testing from partners able to test. RISC-V & S390 don't currently support generational mode Shenandoah.

This looks pretty good.  Have you sent it through the test pipeline?

If we have this ShenandoahCardBarrier flag, I'd be inclined to use it everywhere we currently have the query heap->mode()->is_generational().  It ought to be more efficient.  Before you make that global change, maybe we should confirm that William (and other interested reviewers) like that approach.

src/hotspot/cpu/aarch64/gc/shenandoah/shenandoahBarrierSetAssembler_aarch64.cpp line 390:

> 388:   // This will need to be changed if we ever use a card-marking post-barrier w/a non-generational
> 389:   // Shenandoah in the future.
> 390:   assert(ShenandoahHeap::heap()->mode()->is_generational(), "Only use case for Shenandoah today");

I'd be ok to assert ShenandoahCardBarrier since it ought to be a bit faster than code as written.  Though I understand this is perhaps a bit more rigorous.

src/hotspot/cpu/aarch64/gc/shenandoah/shenandoahBarrierSetAssembler_aarch64.cpp line 636:

> 634:   // This will need to be changed if we ever use a card-marking post-barrier w/a non-generational
> 635:   // Shenandoah in the future.
> 636:   assert(ShenandoahHeap::heap()->mode()->is_generational(), "Only use case for Shenandoah today");

same here: ok to use ShenandoahCardBarrier.

src/hotspot/cpu/ppc/gc/shenandoah/shenandoahBarrierSetAssembler_ppc.cpp line 600:

> 598: 
> 599: void ShenandoahBarrierSetAssembler::store_check(MacroAssembler* masm, Register base, RegisterOrConstant ind_or_offs, Register tmp) {
> 600:   assert(ShenandoahCardBarrier && ShenandoahHeap::heap()->mode()->is_generational(), "Error");)

And here...

src/hotspot/cpu/ppc/gc/shenandoah/shenandoahBarrierSetAssembler_ppc.cpp line 796:

> 794: void ShenandoahBarrierSetAssembler::gen_write_ref_array_post_barrier(MacroAssembler* masm, DecoratorSet decorators,
> 795:                                                                      Register addr, Register count, Register preserve) {
> 796:   assert(ShenandoahCardBarrier && ShenandoahHeap::heap()->mode()->is_generational(), "Error");

Here too.

src/hotspot/cpu/x86/gc/shenandoah/shenandoahBarrierSetAssembler_x86.cpp line 213:

> 211:                                                        Register src, Register dst, Register count) {
> 212: 
> 213:   if (!ShenandoahCardBarrier) {

Why wouldn't we handle this the same as other barriers.  assert(ShenandoahCardBarrier) here and check for ShenandoahCardBarrier before calling arraycopy_epilogue()?

src/hotspot/share/gc/shenandoah/shenandoah_globals.hpp line 523:

> 521:   product(bool, ShenandoahCardBarrier, false, DIAGNOSTIC,                   \
> 522:           "Turn on/off card-marking post-write barrier in Shenandoah")      \
> 523:                                                                             \

I was thinking this would not be visible on the command line.  If we make it visible, then we would need to assure consistency with mode == generational.

My thought was we would just set this global variable in, for example, ShenandoahArguments::initialize() if !strcmp(ShenandoahGCMode, "generational")

-------------

PR Review: https://git.openjdk.org/shenandoah/pull/313#pullrequestreview-1599099341
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1307943153
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1307943562
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1307944197
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1307944658
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1307946501
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1307956536