RFR: Parallelize safepoint cleanup

[Igor Veresov]

> On Jul 7, 2017, at 4:23 AM, Robbin Ehn <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/>
> 
> 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

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