RFR: Implement protocol for safe OOM during evacuation handling

[Roman Kennke]

Am 07.03.2018 um 17:48 schrieb Aleksey Shipilev:
> On 03/05/2018 11:30 PM, Roman Kennke wrote:
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/safe-oom-during-evac/webrev.09.diff
>> Full:
>> http://cr.openjdk.java.net/~rkennke/safe-oom-during-evac/webrev.09
> 
> I see no significant problems with this patch. Looks exciting!
> 
>  *) Naming: I think it is too verbose. Suggestion:
>    shenandoahEvacOOMHandler.hpp
>    shenandoahEvacOOMHandler.cpp
>    class ShenandoahEvacOOMHandler
>    class ShenandoahEvacOOMHandlerScope
>    class ShenandoahEvacOOMHandlerScopeLeaver

Good suggestions. I renamed them.

>  *) Does it make sense to enter the evac scope once for entire array in SBS::arraycopy, not in
> SBS::arraycopy_element? E.g. enter the scope when StoreValMode is {WRITE_BARRIER_MAYBE_ENQUEUE,
> WRITE_BARRIER_ALWAYS_ENQUEUE}?

Absolutely! I actually planned to do that, but then forgot. There's one
small caveat: It enters + leaves the OOM-scope even for
non-partial/non-traversal heuristics, which don't actually evac
anything. It could be improved by passing an arg to
ShenandoahEvacOOMScope to activate it only when true *but* I don't
really know how to deal with leaving the scope then. I guess it doesn't
really hurt to do the OOM-scope even if it does not actually evac anything.

>  *) SpinPause() in wait loop is too bad: it is the active busy-wait. It is better to actively yield
> to allow other threads to make progress and make what you are waiting for a reality sooner.
> 
>  *) _threads_in_evac should be handled consistently with atomics.
> 
> This, and other changes here:
>   http://cr.openjdk.java.net/~shade/shenandoah/oomevac-handler-shade.patch

Thanks for doing those!


>> I would also like to draw the discussion on thread.hpp/thread.cpp
>> changes: it seems like we may be able to forgo those and fold the
>> oom_during_evac flag into the existing gc_state. I don't care very much
>> about evac_allowed flags, we could drop that. WDYT?
> 
> The problem, I think, is that gc_state is really the cache of global ShHeap state, and it is treated
> as such. E.g. any setter fro ShHeap would overwrite all thread-local gc_states wholesale. This is
> not what the patch is apparently expects: it has to track individual per-thread state?

Right. I left the extra state in Thread in place. In the long run, we
should probably advocate for some GC-specific thread-local stuff in
thread.hpp that any GC can use as it wishes.


Differential webrev (against your changes):
http://cr.openjdk.java.net/~rkennke/safe-oom-during-evac/webrev.10.diff/
Full (incl. your changes):
http://cr.openjdk.java.net/~rkennke/safe-oom-during-evac/webrev.10/

Ok to go?

Roman