RFR: Parallelize safepoint cleanup

[Roman Kennke]

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

Roman