RFR: Parallelize safepoint cleanup

Roman Kennke rkennke at redhat.com
Fri Jul 7 10:51:38 UTC 2017

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?



More information about the hotspot-runtime-dev mailing list