RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing [v5]

[Coleen Phillimore]

On Thu, 24 Sep 2020 12:27:42 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> This PR the implementation of "JEP 376: ZGC: Concurrent Thread-Stack Processing" (cf.
>> https://openjdk.java.net/jeps/376).
>> Basically, this patch modifies the epilog safepoint when returning from a frame (supporting interpreter frames, c1, c2,
>> and native wrapper frames), to compare the stack pointer against a thread-local value. This turns return polls into
>> more of a swiss army knife that can be used to poll for safepoints, handshakes, but also returns into not yet safe to
>> expose frames, denoted by a "stack watermark".  ZGC will leave frames (and other thread oops) in a state of a mess in
>> the GC checkpoint safepoints, rather than processing all threads and their stacks. Processing is initialized
>> automagically when threads wake up for a safepoint, or get poked by a handshake or safepoint. Said initialization
>> processes a few (3) frames and other thread oops. The rest - the bulk of the frame processing, is deferred until it is
>> actually needed. It is needed when a frame is exposed to either 1) execution (returns or unwinding due to exception
>> handling), or 2) stack walker APIs. A hook is then run to go and finish the lazy processing of frames.  Mutator and GC
>> threads can compete for processing. The processing is therefore performed under a per-thread lock. Note that disarming
>> of the poll word (that the returns are comparing against) is only performed by the thread itself. So sliding the
>> watermark up will require one runtime call for a thread to note that nothing needs to be done, and then update the poll
>> word accordingly. Downgrading the poll word concurrently by other threads was simply not worth the complexity it
>> brought (and is only possible on TSO machines). So left that one out.
>
> Erik Österlund has updated the pull request with a new target base due to a merge or a rebase. The pull request now
> contains seven commits:
>  - Review: Per CR 1
>  - Merge branch 'master' into 8253180_conc_stack_scanning
>  - Review: Albert CR 1
>  - Review: SteafanK CR 2
>  - Merge branch 'master' into 8253180_conc_stack_scanning
>  - Review: SteafanK CR 1
>  - 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing

I looked at the runtime code and it is nicely non-invasive, and makes sense where there are changes.

src/hotspot/share/runtime/vframe.hpp line 340:

> 338:  public:
> 339:   // Constructors
> 340:   vframeStream(JavaThread* thread, bool stop_at_java_call_stub = false, bool process_frames = true);

I was wondering if you supply arguments to all callers of vframeStream now?  It seems like having default parameters is
an invitation to errors. Same with RegMap.

src/hotspot/share/classfile/javaClasses.cpp line 2440:

> 2438:   // trace as utilizing vframe.
> 2439: #ifdef ASSERT
> 2440:   vframeStream st(thread, false /* stop_at_java_call_stub */, false /* process_frames */);

This is a nit, but could you put the /* stop_at_java_call_stub*/ and /* process_frames */ before the values? Having a
bunch of bool parameters seems like it could also become buggy.  In linkResolver.hpp we have enums so that the values
can be checked and are more explicit at the call sites.  Not this change, but can we fix this later to do the same
thing?

-------------

PR: https://git.openjdk.java.net/jdk/pull/296