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

[Roman Kennke]

Am 12.01.2018 um 16:57 schrieb Aleksey Shipilev:
> On 01/12/2018 12:11 PM, Aleksey Shipilev wrote:
>> http://cr.openjdk.java.net/~shade/shenandoah/single-flag/webrev.01/
>>
>> Please review this carefully. I did AArch64 change blindly, symmetric with x86. Taking care of SATB
>> check requires some specialization in g1_wb_pre, and the relevant compiler matching changes. This
>> would get convenient as we common the gc-state load between the safepoints, and that would not touch
>> G1 SATB barriers then.
>>
>> Example disassembly, (0x3d8(%r15) is our flag):
>>    http://cr.openjdk.java.net/~shade/shenandoah/single-flag/single-flag.perfasm
>>
>> Testing: hotspot_gc_shenandoah, eyeballing generated code
> 
> Renamed need_update_refs to is_unstable -- this captures the intent better, and updated comments a
> little:
>    http://cr.openjdk.java.net/~shade/shenandoah/single-flag/webrev.02/
> 
> Thanks,
> -Aleksey
> 
> 

This is great stuff!

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 :-)

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.

src/hotspot/share/c1/c1_LIRGenerator.cpp:

same as above?

src/hotspot/share/opto/graphKit.cpp

same as above?

This stuff probably qualifies for a separate patch, as it diverges from 
previous code.

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

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.

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

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

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

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.

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

AArch64 looks reasonable, but should probably be built and tested once? 
I don't have resources right now to do that though.


Everything else looks great!