RFR: Make weakref processing always use Executor

[Roman Kennke]

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.

> *) The pt.print_all_references() is moved closer to process_discovered_references in traversal GC,
> but not in concurrent GC.

Hmm, this should be consistent at the least.

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

Roman