RFR: Parallelize safepoint cleanup

Robbin Ehn robbin.ehn at oracle.com
Fri Jul 7 11:23:30 UTC 2017


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

Yes, exactly, I like this!
Dan? Igor ? Tobias?

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-runtime-dev mailing list