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. Next set of comments. src/hotspot/share/gc/z/zStackWatermark.cpp line 78: > 76: ZThreadLocalData::do_invisible_root(_jt, > ZBarrier::load_barrier_on_invisible_root_oop_field); > 77: > 78: ZVerify::verify_thread_frames_bad(_jt); Every time I read the name verify_thread_no_frames_bad I read it as "verify that no frames are bad", but that's not at all what that function does. Is there still a reason why verify_thread_no_frames_bad and verify_thread_frames_bad are split up into two functions? Or could we fuse them into a ZVerify::verify_thread_bad? src/hotspot/share/gc/z/zBarrier.cpp line 130: > 128: uintptr_t > ZBarrier::load_barrier_on_invisible_root_oop_slow_path(uintptr_t addr) { > 129: return during_relocate() ? relocate(addr) : mark<DontFollow, Strong, > Publish>(addr); > 130: } There's a style skew between load_barrier_on_oop_slow_path and load_barrier_on_invisible_root_oop_slow_path. The former calls wrapper function relocate_or_mark, which does the during_relocate() ... check. Maybe do the same for load_barrier_on_invisible_root_oop_slow_path, and introduce a relocate_or_mark_no_follow? src/hotspot/share/gc/z/zBarrierSet.cpp line 85: > 83: ZThreadLocalData::set_address_bad_mask(thread, ZAddressBadMask); > 84: if (thread->is_Java_thread()) { > 85: JavaThread* const jt = static_cast<JavaThread*>(thread); Use as_Java_thread() here? src/hotspot/share/gc/z/zCollectedHeap.cpp line 235: > 233: return true; > 234: } > 235: Weird placement between store barrier functions. But even weirder that we still have those functions. I'll remove them with JDK-8253516. src/hotspot/share/gc/z/zDriver.cpp line 108: > 106: return false; > 107: } > 108: Group needs_inactive_gc_locker, skip_thread_oop_barriers, and allow_coalesced_vm_operations together? Add a comment about why we chose to skip coalescing here. src/hotspot/share/gc/z/zHeapIterator.cpp line 90: > 88: virtual void do_thread(Thread* thread) { > 89: if (thread->is_Java_thread()) { > 90: > StackWatermarkSet::finish_processing(static_cast<JavaThread*>(thread), NULL > /* context */, > StackWatermarkKind::gc); as_Java_thread() ? src/hotspot/share/gc/z/zNMethod.cpp line 244: > 242: ZNMethodToOopsDoClosure(OopClosure* cl, bool should_disarm_nmethods) : > 243: _cl(cl), > 244: _should_disarm_nmethods(should_disarm_nmethods) {} Indentation is off. src/hotspot/share/gc/z/zObjectAllocator.cpp line 2: > 1: /* > 2: * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights > reserved. Revert. src/hotspot/share/gc/z/zRootsIterator.cpp line 200: > 198: ZRootsIteratorClosure* const _cl; > 199: // The resource mark is needed because interpreter oop maps are not > reused in concurrent mode. > 200: // Instead, they are temporary, and resource allocated. temporary, and => temporary and ? src/hotspot/share/gc/z/zStackWatermark.cpp line 96: > 94: void ZStackWatermark::process(const frame& fr, RegisterMap& register_map, > void* context) { > 95: ZVerify::verify_frame_bad(fr, register_map); > 96: frame(fr).oops_do(closure_from_context(context), &_cb_cl, > ®ister_map); With recent frame::oops_do cleanups, frame(fr) can be changed to simply fr. src/hotspot/share/gc/z/zVerify.cpp line 377: > 375: void ZVerify::verify_frame_bad(const frame& fr, RegisterMap& > register_map) { > 376: ZVerifyBadOopClosure verify_cl; > 377: frame(fr).oops_do(&verify_cl, NULL, ®ister_map); With recent frame::oops_do cleanups, frame(fr) can be changed to simply fr. src/hotspot/share/gc/z/zStackWatermark.hpp line 46: > 44: ZOnStackCodeBlobClosure(); > 45: > 46: virtual void do_code_blob(CodeBlob* cb); Could be private. src/hotspot/share/gc/z/zStackWatermark.hpp line 57: > 55: OopClosure* closure_from_context(void* context); > 56: > 57: protected: No need to be protected, so can be private. We don't have sub-classes that overrides ZStackWatermark. ------------- PR: https://git.openjdk.java.net/jdk/pull/296
