RFR: Parallelize safepoint cleanup

[Roman Kennke]

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


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