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

[Roman Kennke]

Am 15.01.2018 um 12:05 schrieb 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.

Yes. I believe traversal is still different: with partial we can fall 
back to regular Shenandoah (intermediate GC), and thus need to check 
different states, and thus need the masking check. With traversal we'd 
never do that, and thus can use a simple zero/not-zero check. I suspect 
this would be a tiny little bit faster. ? But I'll figure this out once 
your patch is in.

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

Thanks!

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

Ok.

>> src/hotspot/share/opto/graphKit.cpp
>>
>> same as above?
> 
> Tried to, but the change gets uncomfortably large.

Ok, that is fine. Should do that later. (But I suspect it only affects 
partial GC stuff, and is thus not high priority.)

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

Very good!

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

Yes. Let's change this once the upstream stuff arrives, and make it 
consistent then. I actually see a chance to upstream our stuff, and make 
G1 use the generic flag, or maybe even generify it even more and make 
room for generic thread local GC data structures.

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

Very good. Patch looks ok for me now.

Roman