RFR: Make weakref processing always use Executor

[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();


>>> 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.

-Aleksey