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

[Y. Srinivas Ramakrishna]

On Mon, 28 Aug 2023 21:16:00 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

> 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.

Did a spin through test pipeline and will run another following the latest changes. All looks good from what I can tell from tests, including jtreg's & SPECjbb as well.

Opened official PR from draft and will link JBS ticket momentarily.

> 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.

Done.

> 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.

Done

> 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...

Done

> 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.

Done.

> 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()?

Today we call arraycopy_epilogue() unconditionally from common code (such as stub generators) irrespective of collector. We'll probably want a better barrier set abstraction across all collectors to do what you suggest in the future. For now, I'll just massage this to conform to the other platforms where we test and branch on the flag here (rather than test and return at the top, or do the job in the body that follows).

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

PR Comment: https://git.openjdk.org/shenandoah/pull/313#issuecomment-1697923735
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1308013930
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1308030882
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1308014029
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1308031191
PR Review Comment: https://git.openjdk.org/shenandoah/pull/313#discussion_r1308024348