RFR: Parallelize safepoint cleanup

[Robbin Ehn]

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