RFR (S): 8185580: Refactor Threads::possibly_parallel_oops_do() to use Threads::parallel_java_threads_do()

[David Holmes]

Hi Roman,

On 17/10/2017 3:27 PM, Roman Kennke wrote:
> Am 17.10.2017 um 07:11 schrieb David Holmes:
>> On 17/10/2017 2:55 PM, Roman Kennke wrote:
>>> Am 17.10.2017 um 05:33 schrieb David Holmes:
>>>> Hi Roman,
>>>>
>>>> On 12/10/2017 5:30 AM, Roman Kennke wrote:
>>>>> This is a follow-up to my earlier safepoint parallel cleanup work.
>>>>>
>>>>> Currently we have 2 places where we apply the parallel claiming 
>>>>> protocol when iterating threads, that is 
>>>>> Threads::parallel_java_threads_do() and 
>>>>> Threads::possibly_parallel_oops_do(). In order to avoid code 
>>>>> duplication, the latter should call the former, using a private 
>>>>> ThreadClosure. We already had one bug (JDK-8185273) that was caused 
>>>>> by an inconsistency between the two.
>>>>>
>>>>> The only other user of parallel_java_threads_do() already filters 
>>>>> out any non-Java-threads, which means that doing the extra 
>>>>> processing of the VMThread should be ok. 
>>>>
>>>> "should" be okay? It's not at all clear to me what processing the 
>>>> VMThread where previously it was not processed, will actually do ??
>>> It ensures that both parallel thread iteration routines are doing the 
>>> same, and thus claim the same threads, which is later 
>>> assert_all_threads_are_claimed(). If we don't do this consistently, 
>>> we throw off the thread claiming mechanics.
>>
>> Sorry that's not answering my question. Before we did not call
>>
>> tc->do_thread(vmt);
>>
>> for the VMThread, but now we do. So what is the affect of this change 
>> in behaviour?
> 
> The effect is that for the safepointing code we now also call 
> do_thread(vmt), but since we're filtering out non-Java threads in the 
> ThreadClosure, the net effect is zero. There is no actual bug here to 
> fix. The only point of this refactoring is to make both code path use 
> the same iterator and thus ensure consistency and *not change* any 
> effects. (Note: there will be future uses of 
> possibly_parallel_threads_do() that need to see the VMThread, e.g. we 
> have one such case in Shenandoah.)
> 
> Does that answer your question?

Yes - thank you. I find it odd that a closure that was passed to 
parallel_java_threads_do additionally filtered on is_Java_thread, but 
with the new entry point it's perfectly fine.

Thanks,
David

> Roman
>