RFR: Refactor evac-in-progress flag to more general gc-phase flag

[Roman Kennke]

Am 14.09.2017 um 20:04 schrieb Aleksey Shipilev:
> On 09/14/2017 07:09 PM, Roman Kennke wrote:
>> Here's another one in preparation of concurrent partial GC:
>>
>> This patch refactors the Thread::evacuation_in_progress flag to a more general gc_phase_in_progress
>> flag. Instead of checking for 0 or != 0, it must now be masked with an appropriate gc-phase-mask
>> before checking. Each bit represents a GC phase. For now, I've only done EVACUATION on bit 0.
>>
>> The idea here is that when storing an oop, we usually do:
>>
>> - check satb-in-progress (and do satb-barrier if so)
>> - check evacuation-in-progress (and do write-barrier if so)
>>
>> and with conc partial, we want to check for conc-partial-in-progress and do a special barrier too.
>>
>> Putting all in one flag allows us to load the flag once, and test it repeatedly. The compiler may
>> even common this flag-loading over multiple stores.
>>
>> Test: hotspot_gc_shenandoah on x86 and aarch64
>>
>> http://cr.openjdk.java.net/~rkennke/gc-phase-flag/webrev.00/
> *) There is no commonning for SATB and evac flags in this patch, right?
Not yet. This is the next step.
> *) aarch64 tests for EVACUATION_BITPOS:
>
>    63   __ tbz(rscratch1, ShenandoahHeap::EVACUATION_BITPOS, done);
>
>   x86 tests for EVACUATION:
>
>   5623   testb(gc_phase_in_progress, ShenandoahHeap::EVACUATION);
>
>   Why this discrepancy?

Because the aarch64 tbz instruction takes the bit position as argument, 
not a mask.
> *) Indent is wrong:
>
> 1610   static ByteSize gc_phase_in_progress_offset() { return byte_offset_of(JavaThread,
> _gc_phase_in_progress); }
Fixed in webrev below.

> *) I wonder if state getters should really be left as "bool"-s:
>
>   166 char JavaThread::gc_phase_in_progress() const {
>   167   return _gc_phase_in_progress;
>   168 }
>
>   ...e.g.:
>
>       bool JavaThread::evacuation_in_progress() const ...
>
>   Can't say for sure without seeing where is going.

My reasoning is that the actual meaning of gc_phase in thread.hpp is 
GC-specific and thus opaque. Maybe in the future, another GC will use 
the same field(s), but with other set of GC phases. I'd put/leave the 
bool state getters in ShenandoahHeap, and get rid of the corresponding 
fields there.

(In fact, we may want to put some sort of opaque void* there to allow 
GCs to manage thread-local data structures... but that is clearly 
another story)

Only intendation is changed as you requested above:
http://cr.openjdk.java.net/~rkennke/gc-phase-flag/webrev.01/ 
<http://cr.openjdk.java.net/%7Erkennke/gc-phase-flag/webrev.01/>

Ok to push?

Roman