RFR: Make weakref processing always use Executor

[Zhengyu Gu]

On 08/12/2018 03:31 PM, Roman Kennke wrote:
> Thanks for reviewing and checking the test!
> 
> Which test is throwing the assertion? Is it reproducible? Any idea what could cause it? Do we have unbalancing or early termination bug in traversal?
> 
> Roman
It is just a bad assertion, following change should fix it.

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

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

-Zhengyu

> 
> Am 12. August 2018 16:14:46 MESZ schrieb Zhengyu Gu <zgu at redhat.com>:
>>
>>
>> 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
>>>>
>