RFR: Single thread-local GC state flag for all barriers

[Aleksey Shipilev]

Updated patch:
  http://cr.openjdk.java.net/~shade/shenandoah/single-flag/webrev.03/


On 01/12/2018 11:06 PM, Roman Kennke wrote:
> One little note: traversal GC will only have one state, and thus doesn't need the masking. I need to
> see how that fits with this new code :-)

So does partial now. You can add another constant to ShenandoahHeap::GCState, and act accordingly.

> I only have fairly minor comments:
> 
> src/hotspot/cpu/x86/macroAssembler_x86.cpp:
> 
>    if (ShenandoahConditionalSATBBarrier) {
>      Label done;
> -    movptr(tmp, (intptr_t) ShenandoahHeap::concurrent_mark_in_progress_addr());
> -    testb(Address(tmp, 0), 1);
> +    movptr(tmp, (intptr_t) ShenandoahHeap::gc_state_addr());
> +    testb(Address(tmp, 0), ShenandoahHeap::MARKING);
> 
> Can't we use the thread-local flag here? There are several occurances of the pattern in that file.

We can use it here, fixed.

> src/hotspot/share/c1/c1_LIRGenerator.cpp:
> 
> same as above?

Not really: the TLS access is complicated there. WB does it differently: it is accessing TLS flags
after the actual lowering. Kept intact.

> src/hotspot/share/opto/graphKit.cpp
> 
> same as above?

Tried to, but the change gets uncomfortably large.


> need_update_refs() -> is_unstable():
> 
> I don't think this captures the intent better. The former, I know right away what it means, the
> latter means I need to look up what 'unstable' means in our context.

I still think need_update_refs is a bad name: it describes "what corrective action we should do",
not "what the heap state is". This gets awkward when we choose not to do RB when need_update_refs is
false. But I guess "is_unstable" is too generic, how about "has_forwarded_objects"? This clearly
states what the heap state is, and makes it trivial to comprehend.

> ---------------
> 
> C2 changes look ok from afar, but Roland should look at them too.
> 
> -------------

Roland, can you take a look?

> src/hotspot/share/runtime/thread.hpp:
> 
> +  // Support for Shenandoah barriers
> +  static char _gc_state_global;
> +  char _gc_state;
> 
> Little side-note: upstream got rid of the static stuff for SATB state and moved it into the
> collector. It's not merged into Shenandoah yet (we really need to update our codebase!!) Maybe we
> should do the same? Keep global state in ShenandoahHeap and ThreadLocal state in Thread, and nowhere
> else.

This patch follows what we have with evac_in_progress -- we have static field in Thread, let's keep
it that way for a time being.

> AArch64 looks reasonable, but should probably be built and tested once? I don't have resources
> right now to do that though.
I have cross-compiled it to AArch64 and ran basic tests on RPi 3. It failed, and I discovered a few
AArch64-specific bugs in the patch. Fixed them, and the basic tests run fine now.


Thanks,
-Aleksey