RFR: Make weakref processing always use Executor

[Roman Kennke]

Am 10.08.2018 um 15:31 schrieb Aleksey Shipilev:
> On 08/10/2018 02:57 PM, Roman Kennke wrote:
>> Am 10.08.2018 um 14:12 schrieb Aleksey Shipilev:
>>> On 08/09/2018 04:38 PM, Roman Kennke wrote:
>>>> This is a solution to the problem that Zhengyu reported recently, where
>>>> the evac-oom-scope for VMThread would block evac-oom-scope for worker
>>>> threads in weak-ref-processing.
>>>>
>>>> The idea is to always go through the executor, no matter if it's 1- or
>>>> N-threaded. The executor in turn calls the proxy, which then sets up the
>>>> correct scopes. In the 1-threaded case, the executor doesn't need to
>>>> fire up worker threads, but can call straight to the proxy in its
>>>> current thread (the VMThread). In other words, it preserves the current
>>>> behaviour, but moves setting up the scopes into the right places.
>>>>
>>>> As a nice side-effect, this unclutters the code a little bit, and will
>>>> unclutter the code *significantly* as soon as WeakProcessor becomes
>>>> MT-capable (it's currently , in which case we can move it to the proxy
>>>> too and get rid of the mess that we currently need to drive it. I
>>>> suspect this might be welcome upstream.
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/weakrefs-1thread/webrev.00/
>>>
>>> I do not understand all the changes in the patch. It seems to mess up our refproc handling, and it
>>> increases our upstream exposure.
>>>
>>> *) It claims to only move the scopes up into the right places, but what it does *on top* of that is
>>> moving the calls to process_discovered_references, and changing what closures it is using. It is
>>> entirely not clear why that change is correct:
>>>
>>>  728     rp->process_discovered_references(NULL, NULL, NULL,
>>>  729                                       &executor, &pt);
>>
>> I am passing NULL for all the single-threaded-closures because they are
>> no longer needed. Because of this, all the
>> process_discovered_references() call are the same and can be moved out
>> of the if-else-tree.
> 
> Well, *that* should be expressed in the comment near the appropriate block, because you remember it
> today, but would completely forget in half a year. Also need assert to nail down the precondition
> for this to work. Something like this:
> 
>    // Process references runs in executor workers only. This means we do not
>    // need to pass any closures to it, because task proxy would set up its own.
>    assert(rp->processing_is_mt(), "sanity");
>    rp->process_discovered_references(NULL, NULL, NULL,
>                                      &executor, &pt);
>    pt.print_all_references();

Well, the thing is that RP decides on phase-by-phase-basis to change
mt-ness. The assert would not catch that. Maybe pass
'should-not-reach-here-closures'  instead of NULL?

>>>> Testing: tier1_gc_shenandoah
>>>
>>> This is not enough. Run at least tier3, and specjbb for completeness?
>>
>> Yes, planning to do that. I did it in the train and couldn't not run
>> anything long. However: do you prefer a 'minimal' change, that would
>> only be a few lines of code, but carry all sorts of now-unnecessary
>> cruft with it? E.g. the single-threaded-closures and all that?
> 
> The patch you have right now is okay, but it leaves cruft around, which muddles the intent. Clean
> that up, and the patch is good. For example, I think complete_gc are not needed anymore, and
> terminator is not needed anymore (CLion should highlight this!):
> 
>  738     ParallelTaskTerminator terminator(1, task_queues());
>     ...
>  740     ShenandoahCMDrainMarkingStackClosure complete_gc(serial_worker_id, &terminator, /*
> reset_terminator = */ true);
> 
> ...plus the comment needs to be updated:
> 
>    // Closures instantiated here are only needed for the single-threaded path in RP.
>    // They share the queue 0 for tracking work, which simplifies implementation.
>    // TODO: As soon as WeakProcessor becomes MT-capable, these closures would become
>    // unnecessary, and could be removed.

Ok. I will clean that up, but only get to it later today.

Roman