RFR: Parallelize safepoint cleanup

Robbin Ehn robbin.ehn at oracle.com
Wed Jul 12 20:39:59 UTC 2017 

On 2017-07-12 15:32, Roman Kennke wrote:
> Hi Robbin and all,
>
> I fixed the 32bit failures by using jlong in all relevant places:
>
> http://cr.openjdk.java.net/~rkennke/8180932/webrev.14.diff/
> <http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.14.diff/>

Looks good!

>
> then Robbin found another problem. SafepointCleanupTest started to fail,
> because "mark nmethods" is no longer printed. This made me think that
> we're not measuring the conflated (and possibly parallelized)
> deflate-idle-monitors+mark-nmethods pass. I added a TraceTime with
> "safepoint cleanup tasks" which measures the total duration of safepoint
> cleanup. We can't reasonably measure a possibly parallel and conflated
> pass standalone, but we can measure all and by subtrating all the other
> subphases, get an idea how long deflation and nmethod marking take up.
>
> http://cr.openjdk.java.net/~rkennke/8180932/webrev.15.diff/
> <http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.15.diff/>

Looks good and thanks for fixing

It's time to ship this, can we have a second review please!

/Robbin

>
> The full webrev is now:
>
> http://cr.openjdk.java.net/~rkennke/8180932/webrev.15/
> <http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.15/>
>
> Hope that's all ;-)
>
> Roman
>
> Am 10.07.2017 um 21:22 schrieb Robbin Ehn:
>> Hi, unfortunately the push failed on 32-bit.
>>
>> (looks like _stack_traversal_mark should be jlong, I feel a bit guilty)
>>
>> I do not have anytime to look at this, so here is the error.
>>
>> /Robbin
>>
>> make[3]: Leaving directory '/opt/jprt/T/P1/185117.rehn/s/hotspot/make'
>> make/Main.gmk:263: recipe for target 'hotspot-client-libs' failed
>> In file included from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/safepoint.hpp:29:0,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/shared/collectedHeap.hpp:33,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/cms/adaptiveFreeList.cpp:28:
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp: In
>> member function 'long int nmethod::stack_traversal_mark()':
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp:399:108:
>> error: call of overloaded 'load_acquire(volatile long int*)' is ambiguous
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp:399:108:
>> note: candidates are:
>> In file included from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/typeArrayOop.hpp:30:0,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/constantPool.hpp:32,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/method.hpp:34,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/frame.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/codeBlob.hpp:31,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/compiledMethod.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/safepoint.hpp:29,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/shared/collectedHeap.hpp:33,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/cms/adaptiveFreeList.cpp:28:
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:57:17:
>> note: static jint OrderAccess::load_acquire(const volatile jint*)
>> <near match>
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:57:17:
>> note:   no known conversion for argument 1 from 'volatile long int*'
>> to 'const volatile jint* {aka const volatile int*}'
>> In file included from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/typeArrayOop.hpp:30:0,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/constantPool.hpp:32,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/method.hpp:34,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/frame.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/codeBlob.hpp:31,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/compiledMethod.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/safepoint.hpp:29,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/shared/collectedHeap.hpp:33,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/cms/adaptiveFreeList.cpp:28:
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:63:17:
>> note: static juint OrderAccess::load_acquire(const volatile juint*)
>> <near match>
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:63:17:
>> note:   no known conversion for argument 1 from 'volatile long int*'
>> to 'const volatile juint* {aka const volatile unsigned int*}'
>> In file included from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/safepoint.hpp:29:0,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/shared/collectedHeap.hpp:33,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/cms/adaptiveFreeList.cpp:28:
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp: In
>> member function 'void nmethod::set_stack_traversal_mark(long int)':
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp:400:105:
>> error: call of overloaded 'release_store(volatile long int*, long
>> int&)' is ambiguous
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp:400:105:
>> note: candidates are:
>> In file included from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/typeArrayOop.hpp:30:0,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/constantPool.hpp:32,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/oops/method.hpp:34,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/frame.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/codeBlob.hpp:31,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/compiledMethod.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/code/nmethod.hpp:28,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/safepoint.hpp:29,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/shared/collectedHeap.hpp:33,
>>                  from
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/gc/cms/adaptiveFreeList.cpp:28:
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:71:17:
>> note: static void OrderAccess::release_store(volatile jint*, jint)
>> <near match>
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:71:17:
>> note:   no known conversion for argument 1 from 'volatile long int*'
>> to 'volatile jint* {aka volatile int*}'
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:77:17:
>> note: static void OrderAccess::release_store(volatile juint*, juint)
>> <near match>
>> /opt/jprt/T/P1/185117.rehn/s/hotspot/src/share/vm/runtime/orderAccess.inline.hpp:77:17:
>> note:   no known conversion for argument 1 from 'volatile long int*'
>> to 'volatile juint* {aka volatile unsigned int*}'
>>
>> On 2017-07-10 20:50, Robbin Ehn wrote:
>>> I'll start a push now.
>>>
>>> /Robbin
>>>
>>> On 2017-07-10 12:38, Roman Kennke wrote:
>>>> Ok, so I guess I need a sponsor for this now:
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/8180932/webrev.12/
>>>> <http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.12/>
>>>>
>>>> Roman
>>>>
>>>> Am 07.07.2017 um 20:09 schrieb Igor Veresov:
>>>>>
>>>>>> On Jul 7, 2017, at 4:23 AM, Robbin Ehn <robbin.ehn at oracle.com
>>>>>> <mailto:robbin.ehn at oracle.com>> wrote:
>>>>>>
>>>>>> Hi Roman,
>>>>>>
>>>>>> On 07/07/2017 12:51 PM, Roman Kennke wrote:
>>>>>>> Hi Robbin,
>>>>>>>>
>>>>>>>> Far down ->
>>>>>>>>
>>>>>>>> On 07/06/2017 08:05 PM, Roman Kennke wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I'm not happy about this change:
>>>>>>>>>>
>>>>>>>>>> +  ~ParallelSPCleanupThreadClosure() {
>>>>>>>>>> +    // This is here to be consistent with sweeper.cpp
>>>>>>>>>> NMethodSweeper::mark_active_nmethods().
>>>>>>>>>> +    // TODO: Is this really needed?
>>>>>>>>>> +    OrderAccess::storestore();
>>>>>>>>>> +  }
>>>>>>>>>>
>>>>>>>>>> because we're adding an OrderAccess::storestore() to be
>>>>>>>>>> consistent
>>>>>>>>>> with an OrderAccess::storestore() that's not properly documented
>>>>>>>>>> which is only increasing the technical debt.
>>>>>>>>>>
>>>>>>>>>> So a couple of things above don't make sense to me:
>>>>>>>>>>
>>>>>>>>>>> - sweeper thread runs outside safepoint
>>>>>>>>>>> - VMThread (which is doing the nmethod marking in the case that
>>>>>>>>>>>    I'm looking at) runs while all other threads (incl. the
>>>>>>>>>>> sweeper)
>>>>>>>>>>>    is holding still.
>>>>>>>>>>
>>>>>>>>>> and:
>>>>>>>>>>
>>>>>>>>>>> There should be no need for a storestore() (at least in
>>>>>>>>>>> sweeper.cpp...
>>>>>>>>>
>>>>>>>>> Either one or the other are running. Either the VMThread is
>>>>>>>>> marking
>>>>>>>>> nmethods (during safepoint) or the sweeper threads are running
>>>>>>>>> (outside
>>>>>>>>> safepoint). Between the two phases, there is a guaranteed
>>>>>>>>> OrderAccess::fence() (see safepoint.cpp). Therefore, no
>>>>>>>>> storestore()
>>>>>>>>> should be necessary.
>>>>>>>>>
>>>>>>>>>  From Igor's comment I can see how it happened though: Apparently
>>>>>>>>> there
>>>>>>>>> *is* a race in sweeper's own concurrent processing (concurrent
>>>>>>>>> with
>>>>>>>>> compiler threads, as far as I understand). And there's a call to
>>>>>>>>> nmethod::mark_as_seen_on_stack() after which a storestore() is
>>>>>>>>> required
>>>>>>>>> (as per Igor's explanation). So the logic probably was: we have
>>>>>>>>> mark_as_seen_on_stack() followed by storestore() here, so let's
>>>>>>>>> also put
>>>>>>>>> a storestore() in the other places that call
>>>>>>>>> mark_as_seen_on_stack(),
>>>>>>>>> one of which happens to be the safepoint cleanup code that we're
>>>>>>>>> discussing. (why the storestore() hasn't been put right into
>>>>>>>>> mark_as_seen_on_stack() I don't understand). In short, one
>>>>>>>>> storestore()
>>>>>>>>> really was necessary, the other looks like it has been put there
>>>>>>>>> 'for
>>>>>>>>> consistency' or just conservatively. But it shouldn't be
>>>>>>>>> necessary in
>>>>>>>>> the safepoint cleanup code that we're discussing.
>>>>>>>>>
>>>>>>>>> So what should we do? Remove the storestore() for good?
>>>>>>>>> Refactor the
>>>>>>>>> code so that both paths at least call the storestore() in the same
>>>>>>>>> place? (E.g. make mark_active_nmethods() use the closure and call
>>>>>>>>> storestore() in the dtor as proposed?)
>>>>>>>>
>>>>>>>> I took a quick look, maybe I'm missing some stuff but:
>>>>>>>>
>>>>>>>> So there is a slight optimization when not running sweeper to skip
>>>>>>>> compiler barrier/fence in stw.
>>>>>>>>
>>>>>>>> Don't think that matter, so I propose something like:
>>>>>>>> -  long  stack_traversal_mark()                    { return
>>>>>>>> _stack_traversal_mark; }
>>>>>>>> -  void  set_stack_traversal_mark(long l)          {
>>>>>>>> _stack_traversal_mark = l; }
>>>>>>>> +  long  stack_traversal_mark()                    { return
>>>>>>>> OrderAccess::load_acquire(&_stack_traversal_mark); }
>>>>>>>> +  void  set_stack_traversal_mark(long l)          {
>>>>>>>> OrderAccess::release_store(&_stack_traversal_mark, l); }
>>>>>>>>
>>>>>>>> Maybe make _stack_traversal_mark volatile also, just as a marking
>>>>>>>> that
>>>>>>>> it is concurrent accessed.
>>>>>>>> And remove both storestore.
>>>>>>>>
>>>>>>>> "Also neither of these state variables are volatile in nmethod, so
>>>>>>>> even the compiler may reorder the stores"
>>>>>>>> Fortunately at least _state is volatile now.
>>>>>>>>
>>>>>>>> I think _state also should use la/rs semantics instead, but that's
>>>>>>>> another story.
>>>>>>> Like this?
>>>>>>> http://cr.openjdk.java.net/~rkennke/8180932/webrev.12/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.12/>
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.12/>
>>>>>>
>>>>>> Yes, exactly, I like this!
>>>>>> Dan? Igor ? Tobias?
>>>>>>
>>>>>
>>>>> That seems correct.
>>>>>
>>>>> igor
>>>>>
>>>>>> Thanks Roman!
>>>>>>
>>>>>> BTW I'm going on vacation (5w) in a few hours, but I will follow this
>>>>>> thread/changeset to the end!
>>>>>>
>>>>>> /Robbin
>>>>>>
>>>>>>> Roman
>>>>>
>>>>
>

More information about the hotspot-gc-dev mailing list