URGENT RFR (S): fix for Test8004741.java crashes with SIGSEGV in JDK10-hs nightly (8185273)

[Daniel D. Daugherty]

Thanks Roman!

Dan


On 7/31/17 11:10 AM, Roman Kennke wrote:
> Looks good!
>
> Roman (not an official reviewer)
>
> PS: I've filed JDK-8185580: Refactor 
> Threads::possibly_parallel_oops_do() to use 
> Threads::parallel_java_threads_do() 
> <https://bugs.openjdk.java.net/browse/JDK-8185580> to take care of the 
> rest for when I get back from vacation.
>
>
>> Latest webrev: http://cr.openjdk.java.net/~dcubed/8185273-webrev/1/
>>
>> Only src/share/vm/runtime/thread.cpp is changed relative to round 0:
>>
>> - Revised the comment in Threads::parallel_java_threads_do.
>> - Added the assert to Threads::assert_all_threads_claimed().
>>
>> Comments, questions and feedback are welcome.
>>
>> Dan
>>
>>
>> On 7/31/17 10:47 AM, Daniel D. Daugherty wrote:
>>> On 7/31/17 9:43 AM, Aleksey Shipilev wrote:
>>>> On 07/31/2017 05:09 PM, Daniel D. Daugherty wrote:
>>>>> On 7/31/17 8:35 AM, Aleksey Shipilev wrote:
>>>>>>> Webrev URL: http://cr.openjdk.java.net/~dcubed/8185273-webrev/0/
>>>>>> Those changes make sense, thanks.
>>>>> Thanks for the fast review!
>>>>>
>>>>>
>>>>>> It is probably worth mentioning that 
>>>>>> Threads::parallel_java_threads_do should be in sync with
>>>>>> Threads::possibly_parallel_oops_do? It gets easier to point out 
>>>>>> the symmetry: possibly_parallel_...
>>>>>> claims all Java threads and the VMThread, so this should also 
>>>>>> claim the VMThread.
>>>>> We would have to be careful about how we phrase that.
>>>>> Threads::possibly_parallel_oops_do() claims and applies
>>>>> the closure to all the threads it claims.
>>>>>
>>>>> Threads::parallel_java_threads_do() is missing the claim
>>>>> for the VMThread (this bug), but does not apply the
>>>>> closure to the VMThread.
>>>> Yeah. It's just I had to work upwards from the gory details 
>>>> explained in the comment to the actual
>>>> setup for the bug to appear. I think details about 
>>>> ParallelSPCleanupTask, safepoint.cpp, parity,
>>>> etc. are too low-level here, and capture only the current state of 
>>>> affairs. E.g. what if there are
>>>> more callers to parallel_java_threads_do in future? What if 
>>>> Parallel SP cleanup ceases to call it?
>>>> Would the comment get outdated? Does 
>>>> Threads::parallel_java_threads_do make sense without Parallel
>>>> SP cleanup? Yes, it does. Would it make sense to cherry-pick it 
>>>> somewhere else with that comment as
>>>> stated? Not really.
>>>>
>>>> AFAIU, the high-level bug is because we have to claim the same 
>>>> subset of threads on all paths. From
>>>> that, it becomes obvious that if possibly_parallel_java_threads_do 
>>>> claims VMThread, all other paths
>>>> should claim it too.
>>>>
>>>> Something like this:
>>>>
>>>> Threads::parallel_java_threads_do(ThreadClosure* tc) {
>>>>     ...
>>>>
>>>>     // Thread claiming protocol requires us to claim the same 
>>>> interesting threads
>>>>     // on all paths. Notably, Threads::possibly_parallel_threads_do 
>>>> claims all
>>>>     // Java threads *and* the VMThread. To avoid breaking the 
>>>> claiming protocol,
>>>>     // we have to appear to claim VMThread on this path too, even 
>>>> if we would not
>>>>     // process the VMThread oops.
>>>>     VMThread* vmt = VMThread::vm_thread();
>>>>     (void)vmt->claim_oops_do(true, cp);
>>>
>>> I like your comment better than mine, with a slight tweak:
>>>
>>>    // Thread claiming protocol requires us to claim the same 
>>> interesting threads
>>>    // on all paths. Notably, Threads::possibly_parallel_threads_do 
>>> claims all
>>>    // Java threads *and* the VMThread. To avoid breaking the 
>>> claiming protocol,
>>>    // we have to claim VMThread on this path too, even if we do not 
>>> apply the
>>>    // closure to the VMThread.
>>>
>>>>
>>>> ...and then the assert fix would seal the deal.
>>>
>>> The assert diffs are applied and tested via JPRT. Please see my
>>> other e-mail on this thread...
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Thanks,
>>>> -Aleksey
>>>>
>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170731/be9c31c1/attachment.htm>