RFR: Parallelize safepoint cleanup

[Roman Kennke]

Am 06.07.2017 um 12:14 schrieb Tobias Hartmann:
> Hi,
>
> On 05.07.2017 20:30, Daniel D. Daugherty wrote:
>> JDK-8132849 is assigned to Tobias; it would be good to get Tobias'
>> review of this fix also.
> Thanks for the notification. The sweeper/safepoint changes look good to me!
Thanks!

I guess I'm going to need a sponsor when the orderAccess::storestore()
issue is resolved.

I'd say *if* we decide to keep the storestore() as conservative measure,
it makes sense to also add it to the parallel processing routines like this:

diff --git a/src/share/vm/runtime/safepoint.cpp
b/src/share/vm/runtime/safepoint.cpp
--- a/src/share/vm/runtime/safepoint.cpp
+++ b/src/share/vm/runtime/safepoint.cpp
@@ -550,6 +550,12 @@
     _counters(counters),
     _nmethod_cl(NMethodSweeper::prepare_mark_active_nmethods()) {}
 
+  ~ParallelSPCleanupThreadClosure() {
+    // This is here to be consistent with sweeper.cpp
NMethodSweeper::mark_active_nmethods().
+    // TODO: Is this really needed?
+    OrderAccess::storestore();
+  }
+
   void do_thread(Thread* thread) {
     ObjectSynchronizer::deflate_thread_local_monitors(thread, _counters);
     if (_nmethod_cl != NULL && thread->is_Java_thread() &&


I've included this in the following (final?) webrev:

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

(I've also added Tobias to Reviewed-by: list... if anybody wants to
sponsor it as-is, simply grab the changeset from here:

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

)

Cheers, Roman


>> 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.
> Unfortunately, I'm also not sure if that barrier is required. Looking at the old RFR thread:
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2013-September/011588.html
>
> It seems that Igor V. suggested this:
> "You definitely need a store-store barrier for non-TSO architectures after the mark_as_seen_on_stack() call on line 1360. Otherwise it still can be reordered by the CPU with respect to the following state assignment. Also neither of these state variables are volatile in nmethod, so even the compiler may reorder the stores."
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2013-September/011729.html
>
> The requested OrderAccess::storestore() was added to nmethod::make_not_entrant_or_zombie() but seems like Albert also added one to NMethodSweeper::mark_active_nmethods().
>
> I'll ping Igor, maybe he knows more.
>
> Thanks,
> Tobias