RFR: Parallelize safepoint cleanup

[Roman Kennke]

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
>

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