RFR: Make weakref processing always use Executor

[Zhengyu Gu]

Please hold this, I got assertion failure with this patch ... I might 
not have time to dig into if assertion is related to this patch until 
Monday.


I did quick scanning of the patch, it does not look right to me, at 
least I did not see the removal of OOM scope from VMThread 
(ShenandoahTraversalGC::weak_refs_work_doit()).

Thanks,

-Zhengyu


On 08/10/2018 12:47 PM, Roman Kennke wrote:
> Am 10.08.2018 um 16:05 schrieb Aleksey Shipilev:
>> On 08/10/2018 03:40 PM, Roman Kennke wrote:
>>>>> 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?
>>
>> Wait a minute. The code relies on RP always entering the MT path when Shenandoah is enabled, even
>> though task proxy would fake it before actually setting up the threads, right? Which means
>> processing_is_mt() should be true on all Shenandoah paths? My intent with that assert was to assert
>> we always enter the task proxy path. ShouldNotReachable-closures would be more straightforward way
>> to assert this.
> 
> I've added ShouldNotReachHere*Closures in the relevant places and fixed
> the other stuff you've mentioned.
> I've also run tier3_gc_shenandoah and good that I did: there's a special
> case when -XX:-ParallelRefProcEnabled where we used to turn off
> mt_processing wholesale. I've fixed that to always do mt_processing but
> with only 1 thread (which is 100% equivalent with this patch).
> 
> This passes tier3_gc_shenandoah now. specjvm is currently running, and
> looks good so far.
> 
> Incremental:
> http://cr.openjdk.java.net/~rkennke/weakrefs-1thread/webrev.01.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/weakrefs-1thread/webrev.01/
> 
> Good now?
> Cheers,
> Roman
>