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

[David Holmes]

Hi Roman,

Should compiler folk be looking at this as well?

I'm not familiar with the details of the NMethodSweeper but it seems to 
me that this change potentially allows multiple concurrent executions of 
NMethodSweeper::prepare_mark_active_nmethods() and that code does not 
appear to be thread-safe.

Thanks,
David

On 21/09/2018 9:57 AM, Roman Kennke wrote:
> Please review the following change to improve and/or eliminate stop to
> to mark stacks for NMethodSweeper.
> 
> The proposed change is two-fold:
> - If ThreadLocalHandshake is enabled, do the stack-nmethod-marking using
> TLHS. This completely eliminates the full safepoint. In this scenario,
> nmethod-marking will also be skipped during safepoint-cleanup. IOW, it
> only happens when the sweeper loop asks for it. It is also most
> efficient because each thread scans its own stack, without requiring to
> synchronize with other threads. Everything remains free-running.
> - Otherwise, try to use GC-safepoint-workers to do the marking at SP.
> The infrastructure for this is already there since some time, and both
> G1 and ZGC (and Shenandoah, when it arrives) support it. The
> safepoint-cleanup-phase already uses it, so let's just do the same in
> sweeper-loop-induced safepoints.
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8132849
> Webrev:
> http://cr.openjdk.java.net/~rkennke/JDK-8132849/webrev.01/
> 
> Testing: hotspot/jtreg:tier1 using +ThreadLocalHandshakes and
> -ThreadLocalHandshakes
> 
> One issue that I am not sure of is the:
> 
> assert(SafepointSynchronize::is_at_safepoint(), "must be executed at a
> safepoint");
> 
> at the start of NMethodSweeper::prepare_mark_active_nmethods().
> 
> I couldn't see any particular reason for it. The
> wait_for_stack_scanning() stuff is called outside safepoinst anyway, and
> the other stuff doesn't seem critical. And besides, in the scenario
> where we'd call this outside safepoint (+ThreadLocalHandshakes) we'd
> only ever call it from the sweeper thread anyway.
> 
> What do you think?
> 
>