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

[Stefan Karlsson]

On Tue, 22 Sep 2020 11:43:41 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.

I've gone over the entire patch, but I'm leaving the compiler parts to others to review.

src/hotspot/share/jfr/leakprofiler/checkpoint/rootResolver.cpp line 275:

> 273:
> 274:     // Traverse the execution stack
> 275:     for (StackFrameStream fst(jt, true /* update */, true /* process_frames */); !fst.is_done(); fst.next()) {

Noticed that lines 261-262 refers to the array that was removed with:
JDK-8252656: Replace RegisterArrayForGC mechanism with plain Handles

And the comment in block 299-304 might need some love.

It's not directly related to this patch, but something that has been moved around in preparation for this patch. I
wouldn't be opposed to cleaning that up with this patch.

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 39:

> 37: #include "runtime/os.hpp"
> 38: #include "runtime/semaphore.hpp"
> 39: #include "runtime/stackWatermarkSet.hpp"

Revert?

src/hotspot/share/runtime/safepoint.cpp line 507:

> 505:       return;
> 506:     }
> 507:     StackWatermarkSet::start_processing(static_cast<JavaThread*>(thread), StackWatermarkKind::gc);

Maybe simply:
if (thread->is_Java_thread()) {
  StackWatermarkSet::start_processing(static_cast<JavaThread*>(thread), StackWatermarkKind::gc);
}

src/hotspot/share/runtime/stackWatermarkSet.cpp line 70:

> 68:   Thread* thread = Thread::current();
> 69:   if (thread->is_Java_thread()) {
> 70:     JavaThread* jt = static_cast<JavaThread*>(thread);

as_Java_thread()

src/hotspot/share/runtime/stackWatermarkSet.cpp line 112:

> 110:     return;
> 111:   }
> 112:   verify_poll_context();

There's a verfy_poll_context here, but no update_poll_values call. I guess this is because we could be iterating over a
thread that is not Thread::current()? But in that case, should we really be "verifying the poll context" of the current
thread?

src/hotspot/share/runtime/stackWatermarkSet.cpp line 125:

> 123:     watermark->start_processing();
> 124:   }
> 125: }

No update poll values here?

src/hotspot/share/utilities/vmError.cpp line 754:

> 752:        Thread* thread = ((NamedThread *)_thread)->processed_thread();
> 753:        if (thread != NULL && thread->is_Java_thread()) {
> 754:          JavaThread* jt = static_cast<JavaThread*>(thread);

as_Java_thread() - maybe just search for this in the patch

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

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