RFR: Make weakref processing always use Executor

[Zhengyu Gu]

On 08/12/2018 09:19 AM, Zhengyu Gu wrote:
> 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.
> 
Okay, there is a bad assertion, not related to this patch:

shenandoahTraversalGC.cpp L1187:

     assert(!_heap->cancelled_gc() || task_queues()->is_empty(), "Should 
be empty");

==>

assert(_heap->cancelled_gc() || task_queues()->is_empty(), "Should be 
empty");


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

I mis-read this part ...

Looks good to me.

-Zhengyu

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