RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing
Stefan Karlsson
stefank at openjdk.java.net
Wed Sep 23 08:14:10 UTC 2020
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.
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
More information about the serviceability-dev
mailing list