RFR: Parallelize safepoint cleanup

Wed Jul 5 21:17:51 UTC 2017

Am 05.07.2017 um 20:30 schrieb Daniel D. Daugherty:
> On 6/27/17 1:47 PM, Roman Kennke wrote:
>> Hi Robbin,
>>
>> Ugh. Thanks for catching this.
>> Problem was that I was accounting the thread-local deflations twice:
>> once in thread-local processing (basically a leftover from my earlier
>> attempt to implement this accounting) and then again in
>> finish_deflate_idle_monitors(). Should be fixed here:
>>
>> http://cr.openjdk.java.net/~rkennke/8180932/webrev.09/
>> <http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.09/>
>
> Are you thinking that this fix resolves all three bugs:
>
>     8132849 Increased stop time in cleanup phase because of
> single-threaded
>             walk of thread stacks in
> NMethodSweeper::mark_active_nmethods()
Yes. It requires additional support code by a GC though to become
actually multithreaded.
>     8153224 Monitor deflation prolong safepoints
Yes. But there's more that we want to do:
- deflate monitors during GC thread scanning (this is a huge winner)
- ultimately, deflate monitors concurrently (a JEP is on the way to
address this)

>     8180932 Parallelize safepoint cleanup
Yes :-)

> JDK-8132849 is assigned to Tobias; it would be good to get Tobias'
> review of this fix also.
Ok, I will reach out to him.

> General comments:
>   - Please don't forget to update Copyright years as needed before
> pushing
Fixed.
>
> src/share/vm/runtime/safepoint.hpp
>     L78:   enum SafepointCleanupTasks {
>         You might want to add a comment here:
>              // The enums are listed in the order of the tasks when
> done serially.
Good idea. Done.
> src/share/vm/runtime/safepoint.cpp
>     L556:         ! thread->is_Code_cache_sweeper_thread()) {
>     L581:     if (!
> _subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_DEFLATE_MONITORS))
> {
>     L589:     if (!
> _subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_UPDATE_INLINE_CACHES))
> {
>     L597:     if (!
> _subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_COMPILATION_POLICY))
> {
>     L605:     if (!
> _subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_SYMBOL_TABLE_REHASH))
> {
>     L615:     if (!
> _subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_STRING_TABLE_REHASH))
> {
>     L625:     if (!
> _subtasks.is_task_claimed(SafepointSynchronize::SAFEPOINT_CLEANUP_CLD_PURGE))
> {
>         nit: HotSpot style doesn't usually have a space after unary '!'.
Ok, thanks! I didn't know that. Is there a document that describes the
Hotspot style? Because, from the top of my head, I can name 3 source
files all in entirely different styles ;-)
>
>     L638: // Various cleaning tasks that should be done periodically
> at safepoints
>     L641:   // Prepare for monitor deflation
>         nit: Please add a period to the end of these sentences.
>
Done.
> src/share/vm/runtime/sweeper.cpp
>     L205:     // TODO: Is this really needed?
>     L206:     OrderAccess::storestore();
>         That's a good question. Looks like that storestore() was
>         added by this changeset:
>
>         $ hg log -r 5357 src/share/vm/runtime/sweeper.cpp
>         changeset:   5357:510fbd28919c
>         user:        anoll
>         date:        Fri Sep 27 10:50:55 2013 +0200
>         summary:     8020151: PSR:PERF Large performance regressions
> when code cache is filled
>
>         The changeset is not small and it looks like two
>         OrderAccess::storestore() calls were added (and one
>         load_ptr_acquire() was deleted):
>
>         $ hg diff -r 5356 -r 5357 | grep OrderAccess
>         +      OrderAccess::storestore();
>         -  nmethod *code = (nmethod
> *)OrderAccess::load_ptr_acquire(&_code);
>         +  OrderAccess::storestore();
>
>         It could be that the storestore() is matching an existing
>         OrderAccess operation or it could have been added in an
>         abundance of caution. We definitely need a Compiler team
>         person to take a look here.
I looked around a little bit. As far as I can tell, all compiler threads
are stopped at a safepoint there. And I don't see anything else that
uses the affected fields during the safepoint. There's a fence() before
resuming safepointed threads. I think it should be safe without
storestore(), but would like to get confirmation from compiler team too.
> src/share/vm/runtime/synchronizer.hpp
>     L36:   int nInuse;         // currently associated with objects
>     L37:   int nInCirculation; // extant
>     L38:   int nScavenged;      // reclaimed
>         nit: Please add one more space before '//' on L36,L37.
Oops. Done.
> src/share/vm/runtime/synchronizer.cpp
>     L1663: // Walk a given monitor list, and deflate idle monitors
>     L1664: // The given list could be a per-thread list or a global list
>     L1665: // Caller acquires gListLock
>     L1666: int
> ObjectSynchronizer::deflate_monitor_list(ObjectMonitor** listHeadp,
>     <snip>
>     L1802:   int deflated_count =
> deflate_monitor_list(thread->omInUseList_addr(), &freeHeadp, &freeTailp);
>     L1804:   Thread::muxAcquire(&gListLock, "scavenge - return");
>         The above deflate_monitor_list() now occurs outside of the
>         gListLock where the old code held the gListLock for this call.
>
>         Yes, it is operating on the thread local list, but what keeps
>         two different worker threads from trying to
> deflate_monitor_list()
>         on the same JavaThread at the same time?
The mechanics in Threads::parallel_java_threads_do() (which I adapted
from Threads::possibly_parallel_oops_do()) ensure that each worker
thread claims a Java thread before processing it. This ensures that each
Java thread is processed by exactly one worker thread.
>         Without the gListLock, I don't see how the worker threads
>         avoid conflicting over the same thread local list. Minimally,
>         the comment on L1665 needs updating.
Okidoki, I added those blocks there:

// In the case of parallel processing of thread local monitor lists,
// work is done by Threads::parallel_threads_do() which ensures that
// each Java thread is processed by exactly one worker thread, and
// thus avoid conflicts that would arise when worker threads would
// process the same monitor lists concurrently.
//
// See also ParallelSPCleanupTask and
// SafepointSynchronizer::do_cleanup_tasks() in safepoint.cpp and
// Threads::parallel_java_threads_do() in thread.cpp.

>
>     L1697:   counters->nInuse = 0;         // currently associated
> with objects
>     L1698:   counters->nInCirculation = 0; // extant
>     L1699:   counters->nScavenged = 0;      // reclaimed
>         nit: Please add one more space before '//' on L1697, L1698.
Done.
>     old L1698:   int nInuse = 0;
>     old L1713:     int inUse = 0;
>         Nice catch here. I've read this code countless times and missed
>         this bug until now. It explains why some of my Java monitor
> testing
>         had odd "in use" counts.
Hmm. I am not aware of a bug there. the inUse declaration was unused,
that is all (I think..)
>     L1797:   if (! MonitorInUseLists) return;
>         nit: HotSpot style doesn't usually have a space after unary '!'.
Done.
>     L1808:   thread->omInUseCount-= deflated_count;
>         nit: Please add a space before '-='.
Done. Also some lines up:

 gOmInUseCount-= deflated_count;

> The only comment I need resolved is about the locking for the thread
> local deflate_monitor_list() call. Everything else is minor.

Thanks so much for the thorough review!

So here's revision #11:

http://cr.openjdk.java.net/~rkennke/8180932/webrev.10/
<http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.10/>

Roman
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170705/0c130331/attachment.htm>