RFR: Parallelize safepoint cleanup

Wed Jul 5 23:30:42 UTC 2017

On 7/5/17 3:17 PM, Roman Kennke wrote:
> 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?

There is such a document:

https://wiki.openjdk.java.net/display/HotSpot/StyleGuide

I believe John Rose is the usual maintainer of the doc...

> Because, from the top of my head, I can name 3 source files all in 
> entirely different styles ;-)

True, very true... unfortunately. I don't know if John's doc mentions
it, but a general rule is to follow the prevailing style in the file.
Sometime this is impossible because sometimes we see multiple styles
in the same file (and we pull our hair out)...

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

Good idea! :-)

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

Cool. No race there.

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

I like the comment. (Others may find it wordy, but my comments are
often thought to be wordy...)

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

You would have that that when I pasted the two lines into the comment,
I would have noticed the difference in the names... sigh...

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

src/share/vm/runtime/synchronizer.cpp
     L1664: // Caller acquires gListLock.
         The new stuff you added below the existing comment is fine.
         However, that existing comment is still wrong because the caller
         doesn't always acquire gListLock. Perhaps:

         // Caller acquires gListLock when operating on a global list.

Thanks for making the changes.

Thumbs up!

Dan

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