RFR: JDK-8132849: Increased stop time in cleanup phase because of single-threaded walk of thread stacks in NMethodSweeper::mark_active_nmethods()

[Roman Kennke]

Hi Erik,

> Thank you for sorting this out. It is very helpful.
> 
> Could you change the name of ThreadToCodeBlobClosure to
> NMethodMarkingThreadClosure. (Motivation: the closure filters
> JavaThreads that are not the sweeper, and actually only looks at
> nmethods, and not other types of CodeBlobs, e.g. AoT methods, so it does
> less than I expect).

Yes will do, but let's first agree on the other issues:

> Also, the NMethodSweeper::prepare_mark_active_nmethods() was built for
> safepoint cleaning and returns either hotness counting or nmethod
> marking closures. However, when moving nmethod marking out to be done
> concurrently with TLH, it is slightly confusing to have the same member
> function called from the concurrent context despite never ever wanting a
> hotness counter closure from there.

The hotness-counting-only-closure will never be used when called from
the sweeper thread because this only happens between sweeping-cycles.
E.g. when safepointing while sweeper is active, it would do only hotness
counting, when sweeper is idle, it would do nmethod marking, which
*also* does the hotness counting. With TLHS, we'd always do the full
thing, because it's only ever queried between sweeper cycles.

> I'm thinking the prepare_mark_active_nmethods() member function could be
> split into two:
> 
> One member function that returns either nmethod marking closure or NULL
> (depending on whether it's needed or not).
> Another member function that calls the first one, and if NULL slaps on a
> hotness counter closure.

We could do that, yes.

> Then from concurrent contexts we would call the first method (nmethod
> marking or NULL), and from STW contexts we would call the second member
> function (nmethod marking or hotness counter).

Right.

> Another thing worth noticing is that the VM_MarkActiveNMethods VM
> operation marks the nmethods on the stack twice. First in safepoint
> cleanup, and subsequently in the operation itself
> (VM_MarkActiveNMethods::doit). I would argue that only one pass is
> enough.

Right...

 Therefore, I would propose to completely remove the nmethod
> marking from the safepoint cleanup, and have safepoint cleanup *only*
> fiddle around with hotness counters. If we do that, then nmethod marking
> is done in VM_MarkActiveNMethods::doit if TLH is off, and in your new
> handshake operation when TLH is on.

Yeah, except that hotness counting is also done in nmethod marking pass.
Would it be enough if we just kept it there? Or do we want hotness
counting stuff to be done always in SP cleanup phase, and not piggy-back
it on nmethod marking?

> Then we can have zero nmethod marking in safepoint cleanup, and
> subsequently figure out how to get rid of the hotness counters.

Is there a use of doing nmethod marking more frequently than what is
forced in do_stack_scanning() ? Is there a use of doing frequent hotness
counters?

Roman