On Tue, 22 Sep 2020 11:43:41 GMT, Erik Österlund <eosterl...@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
