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

[Roman Kennke]

Hi David,

thanks for looking at this!

> Should compiler folk be looking at this as well?

Maybe. I added them.

> 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.

There are two scenarios now:
- TLHS enabled: NMethodSweeper::prepare_mark_active_nmethods() only gets
called from the sweeper thread.
- TLHS disabled: NMethodSweeper::prepare_mark_active_nmethods() only
gets called from VMThread/at-safepoint.

The structures used in NMethodSweeper::prepare_mark_active_nmethods()
are only ever called from sweeper thread, or at safepoint, and those are
exclusive, that means it should be safe. And instead of removing the
assert, we can extend it to accept the sweeper thread. I also noticed
that we need to grab the CodeCache_lock before calling into
prepare_mark_active_nmethods() so I added that and put that into the
assertion.

Incremental webrev:
http://cr.openjdk.java.net/~rkennke/JDK-8132849/webrev.02.diff/
Full webrev:
http://cr.openjdk.java.net/~rkennke/JDK-8132849/webrev.02/

Better now?

Thanks,
Roman


> 
> 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?
>>
>>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20180923/12215fd9/signature.asc>