RFR: Make partial GC concurrent

[Roman Kennke]

Am 06.10.2017 um 21:11 schrieb Roman Kennke:
> Am 06.10.2017 um 19:46 schrieb Aleksey Shipilev:
>> On 10/06/2017 11:20 AM, Roman Kennke wrote:
>>> http://cr.openjdk.java.net/~rkennke/concpartial/webrev.01/
>> It looks like a great chunk of work!
>>
>> First batch of comments:
>>
>>   *) It feels weird to have the accessor like SH::is_conc_partial_gc. 
>> For one, I thought it is a
>> "progress" accessor, that tripped me into believing this was the 
>> coding error. Then it became
>> obvious it selects the flavors of barriers. It seems to me that it's 
>> cleaner to continue our style
>> of global barriers flags. This means:
>>
>>    - Things like these would not be needed:
>>
>>     580     if (UseTLAB && ShenandoahAsmWB &&
>>     581         ! ShenandoahHeap::heap()->is_conc_partial_gc()) {
>>
>>     ...if we just disable ShenandoahAsmWB when is_conc_partial_gc() 
>> is off.
>>
>>   - Conditions like these would not be needed:
>>
>>     storeval_barrier:
>>       if (_heap->is_conc_partial_gc()) {
>>         ...
>>       } else {
>>          ...
>>       }
>>
>>     ...if we have separate ShenandoahStoreValReadBarrier and 
>> ShenandoahStoreValWriteBarrier. This
>> would also let us measure their respective overheads better
>
> Ok, I see your point. I am not really a fan of controlling behaviour 
> via global flags, but I see the benefit of it for diagnostics and 
> performance measurements, etc. I will change it accordingly.
>>   *) There is some weird interaction between 
>> ShenandoahKeepAliveBarrier and new concurrent partial
>> code. MacroAssembler::keep_alive_barrier would call 
>> shenandoah_write_barrier_pre, and that only get
>> executed when concurrent mark is active. Shouldn't it execute when 
>> concurrent partial is active too?
>> This gets weirder when ShenandoahBarrierSet::keep_alive_barrier 
>> executes WB when conc-partial is in
>> progress, but MacroAssembler::keep_alive_barrier does not.
> Yes, it looks weird but is actually correct.
> In order to keep an object alive during conc partial, we *would* need 
> a write barrier, I have done it this way in the runtime barriers. We 
> must not enqueue the object to keep alive in the SATB queues, we use 
> this for different purpose. I haven't added those write-barriers to 
> the interpreter, c1 and c2 paths though, for 2 reasons:
>  1. they are only used to support Reference.get() (as opposed to 
> runtime, where it's also used for stuff like string table), and we 
> don't do special reference processing (yet) in conc partial.
>  2. I got some weird errors when adding write-barriers in interpreter 
> (iirc), because it's used in a strange place and messes up the stack 
> frame. This would require work if we ever want to support ref 
> processing during conc partial, but I decided against it for now.
>
>>   *) It seems that !UseShenandoahMatrix paths in 
>> ShenandoahBarrierSet::write_ref_array,
>> ShenandoahBarrierSet::write_region_work are not needed, because no 
>> matrix -> no partial? Would need
>> to assert that though. But that keeps compiled code denser.
> Right.
>>   *) What's the purpose of ShenandoahConcMarkGC flag?
> I found that useful during implementation to turn off concurrent 
> marking altogether. I.e. it would only do conc-partial or full-gc. If 
> it bothers you, I can throw it out again.
>>   *) Yes, marking loops in shenandoahPartialGC.cpp should definitely 
>> be deduplicated.
>
> Eh hum. Forgot about that. Will do in next iteration.
>>   *) ShenandoahPartialGC::concurrent_partial_collection() and 
>> final_partial_collection should
>> probably have a marker object with ShenandoahWorkerScope to set up 
>> appropriate number of concurrent
>> and parallel workers. This would need a new method in 
>> ShenandoahWorkerPolicy.
> Ah ok. Haven't thought about that, but you're right.
>>   *) Checking for cancellation is not needed in 
>> ShenandoahPartialGC::final_partial_collection(),
>> because it is checked before?
>>
>>   521     if (ShenandoahVerify && ! _heap->cancelled_concgc()) {
> It can still run OOM during final-evac and cancel GC and leave us in 
> an incomplete state. Or am I missing something?
>
> Thanks for reviewing. I'll post another webrev with your suggested 
> changes ... I guess next week :-)

And here it comes:

http://cr.openjdk.java.net/~rkennke/concpartial/webrev.02/ 
<http://cr.openjdk.java.net/%7Erkennke/concpartial/webrev.02/>

I think I addressed all the points you mentioned (except those that are 
not applicable). hotspot_gc_shenandoah still passes.


Roman