RFR: Make partial GC concurrent

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

Roman