RFR: Make weakref processing always use Executor

[Roman Kennke]

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

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

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.