On Wed, 23 Sep 2020 08:51:08 GMT, Stefan Karlsson <stef...@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. > > 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. Will do. > 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? Correct - I'm not updating the poll values, because it's not necessarily the current thread we are processing. But the verify_poll_context() code does correctly always refer to the current frame. It verifies we are in the right context to perform this kind of processing, yet the processing could be for a different thread. > src/hotspot/share/runtime/stackWatermarkSet.cpp line 125: > >> 123: watermark->start_processing(); >> 124: } >> 125: } > > No update poll values here? Like the previous scenario, start_processing may be called on a different thread than Thread::current. The thread the stack belongs to will always update the poll values when it wakes up from the safepoint. Other threads have no business updating it. > 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 Done. ------------- PR: https://git.openjdk.java.net/jdk/pull/296
