On Thu, 24 Sep 2020 12:27:42 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. > > 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
