RFR (11u, XXL): Upstream/backport Shenandoah to JDK11u - the review thread

[Roman Kennke]

On Fri, 2020-07-24 at 22:23 +0000, Aditya Mandaleeka wrote:
> Hey Roman,
> 
> Thanks for addressing the comments! Just following up on one of them:
> 
> > >     The outermost block has been relaxed to be under `#if
> > > INCLUDE_G1GC || INCLUDE_SHENANDOAHGC`, but I think
> > >     that last line above (the call to
> > > G1ThreadLocalData::dirty_card_queue_buffer_offset()) won't
> > > compile if
> > > G1
> > >     is disabled, will it? I haven't verified it myself, but you
> > > may
> > > want to check that. I assume including
> > >     Shenandoah and not G1 is a valid configuration based on the
> > > other
> > > checks you added.
> > > 
> > 
> > I think you're missing the final #endif after the block that you
> > showed
> > here. The one that closes the #if INCLUDE_G1GC ||
> > INCLUDE_SHENANDOAHGC.
> > I think it's ok. Also notice that
> > G1ThreadLocalData::dirty_card_queue_buffer_offset() will never
> > happen
> > with Shenandoah, we don't use the dirty_card_queue.
> 
> I think I should have explained the issue I see better.
> 
> Here is the relevant code from escape.cpp again, after the patch:
> 
>         #if INCLUDE_G1GC || INCLUDE_SHENANDOAHGC
>                   if ((UseG1GC SHENANDOAHGC_ONLY(|| UseShenandoahGC))
> && adr->is_AddP()) {
>                     Node* base = get_addp_base(adr);
>                     if (base->Opcode() == Op_LoadP &&
>                         base->in(MemNode::Address)->is_AddP()) {
>                       adr = base->in(MemNode::Address);
>                       Node* tls = get_addp_base(adr);
>                       if (tls->Opcode() == Op_ThreadLocal) {
>                         int offs = (int)igvn->find_intptr_t_con(adr-
> >in(AddPNode::Offset), Type::OffsetBot);
>         #if INCLUDE_G1GC && INCLUDE_SHENANDOAHGC
>                         const int buf_offset = in_bytes(UseG1GC ?
> G1ThreadLocalData::satb_mark_queue_buffer_offset()
>                                                                 :
> ShenandoahThreadLocalData::satb_mark_queue_buffer_offset());
>         #elif INCLUDE_G1GC
>                         const int buf_offset =
> in_bytes(G1ThreadLocalData::satb_mark_queue_buffer_offset());
>         #else
>                         const int buf_offset =
> in_bytes(ShenandoahThreadLocalData::satb_mark_queue_buffer_offset());
>         #endif
>                         if (offs == buf_offset) {
>                           break; // G1 pre barrier previous oop value
> store.
>                         }
>                         if (offs ==
> in_bytes(G1ThreadLocalData::dirty_card_queue_buffer_offset())) {
>                           break; // G1 post barrier card address
> store.
>                         }
>                       }
>                     }
>                   }
>         #endif
> 
> That last call to G1ThreadLocalData::dirty_card_queue_buffer_offset()
> is guarded only by
> the `#if INCLUDE_G1GC || INCLUDE_SHENANDOAHGC` so it can be included
> in the compilation
> unit even when INCLUDE_G1GC is off (as long as INCLUDE_SHENANDOAHGC
> is on), and thus cause
> a build break. I think that part needs to also be guarded under #if
> INCLUDE_G1GC.

Indeed! That mess of ifdefs is indeed a little complicated, it is so
much nicer in JDK12+ where we have proper GC interfaces for that stuff.
But that would be too much to backport now. (And who disables G1 and
enables Shenandoah from their builds anyway? :-P )

Here comes webrev12, I only added the #if-block around G1-code:

Shared-only:
http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.12-shared/
Full:
http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.12-all/

Thanks,
Roman