RFR: Parallelize safepoint cleanup

[Robbin Ehn]

Hi,

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.

Thanks, Robbin

> 
> 
> Roman
> 
> 
>>
>> If the sweeper thread is running "outside safepoint", then how is
>> the sweeper thread "holding still" while the VMThread is doing the
>> nmethod marking? Those two points are contradictory.
>>
>> If the sweeper thread is indeed executing outside a safepoint, then
>> a storestore() is needed for its memory changes to be seen by the
>> VMThread which is doing things in parallel. That means that the
>> comment that sweeper.cpp doesn't need the storestore() is also
>> contradictory.
>>
>> So what do you mean by this comment:
>>
>>> - sweeper thread runs outside safepoint
>>
>> and once we know that we can figure out the rest...
>>
>> Dan
>>
>>
>>>
>>> Roman
>>>
>>>>
>>>> igor
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>
>>>
>>
>