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

[Roman Kennke]

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?

Roman