RFR: Make weakref processing always use Executor

[Roman Kennke]

Ah! Then please fix it. ;-)

Am 12. August 2018 21:57:03 MESZ schrieb Zhengyu Gu <zgu at redhat.com>:
>
>
>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
>>>>>
>> 

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