RFR: JDK-8132849: Increased stop time in cleanup phase because of single-threaded walk of thread stacks in NMethodSweeper::mark_active_nmethods()

[Zhengyu Gu]

Hi Roman,

On 09/24/2018 12:04 PM, Roman Kennke wrote:
> Zhengyu noted off-list that the !ThreadLocalHandshakes version requires
> to call Threads::change_thread_parity() before using
> Threads::possibly_parallel_threads_do(), and that we can assert
> is_Java_thread() instead of explicit filtering.

My bad suggestion on assertion for Java thread, 
Threads::possibly_parallel_threads_do also walks VMThread, sorry!

Otherwise, looks good to me.

-Zhengyu

> 
> This change does that:
> Incremental:
> http://cr.openjdk.java.net/~rkennke/JDK-8132849/webrev.04.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/JDK-8132849/webrev.04/
> 
> Let me know what you think!
> 
> Thanks,
> Roman
> 
>>> So my suggestion for now is:
>>> Do nmethod marking in VM operation/handshake operation.
>>> Do hotness counter updating when UseCodeAging in safepoint cleanup.
>>
>> Ok, this change should do that:
>> Incremental:
>> http://cr.openjdk.java.net/~rkennke/JDK-8132849/webrev.03.diff/
>> Full:
>> http://cr.openjdk.java.net/~rkennke/JDK-8132849/webrev.03/
>>
>> Note that nmethod-marking still resets hotness counters.
>>
>> Good now for pushing?
>>
>> Roman
>>
>>
>>> And now you might be wondering if it really makes sense to walk all
>>> stacks in the system every safepoint, to provide some heuristic about
>>> whether nmethods are inactive or not. Arguably not. I have an idea about
>>> a much better way of doing this. I will get back to you in a few days
>>> about that.
>>>
>>> Hope this helps.
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2018-09-24 11:36, Roman Kennke wrote:
>>>>> Is there a use of doing nmethod marking more frequently than what is
>>>>> forced in do_stack_scanning() ?
>>>> As far as I can tell, it is sufficient to mark nmethods right before
>>>> sweeping. It might even be counter-productive to do more marking passes:
>>>> it would result in more non-entrant nmethods marked as 'seen on stack'
>>>> even if they are no longer on stack.
>>>>
>>>> I am not 100% sure about the hotness counter though. From what I see,
>>>> it's only used for sweeper too, and it really looks like resetting the
>>>> counter on nmethod-walk is enough. But I'd like confirmation from
>>>> somebody who knows better than I do. If it's really good enough, we may
>>>> remove the nmethod stuff completely from SP cleanup, and also remove the
>>>> hotness-counter-closure, and always piggy-back the stuff on nmethod
>>>> walking, either in its own VM_Op, or in its handshake.
>>>>
>>>> On the other hand, why is hotness counting and nmethod marking split out
>>>> in sp-cleanup in the first place then?
>>>>
>>>> Roman
>>>>
>>>
>>
>>
> 
>