From 3353f8e0875165adbc8ee764a4c8d8817a87cd88 Mon Sep 17 00:00:00 2001 From: Stefan Karlsson Date: Tue, 18 Feb 2025 07:51:45 +0000 Subject: [PATCH] 8349652: Rewire nmethod oop load barriers Reviewed-by: kvn, aboldtch --- src/hotspot/share/code/nmethod.cpp | 8 ++++-- .../share/gc/shared/barrierSetNMethod.cpp | 8 ++++++ .../share/gc/shared/barrierSetNMethod.hpp | 4 +++ src/hotspot/share/gc/z/zBarrierSet.inline.hpp | 6 +---- src/hotspot/share/gc/z/zBarrierSetNMethod.cpp | 8 ++++++ src/hotspot/share/gc/z/zBarrierSetNMethod.hpp | 3 +++ src/hotspot/share/gc/z/zNMethod.cpp | 24 ++++++++++------- src/hotspot/share/gc/z/zNMethod.hpp | 6 ++++- src/hotspot/share/oops/access.hpp | 6 ----- src/hotspot/share/oops/accessDecorators.hpp | 26 +++++++++---------- 10 files changed, 62 insertions(+), 37 deletions(-) diff --git a/src/hotspot/share/code/nmethod.cpp b/src/hotspot/share/code/nmethod.cpp index 283970afa69..e454d8f3f68 100644 --- a/src/hotspot/share/code/nmethod.cpp +++ b/src/hotspot/share/code/nmethod.cpp @@ -2143,14 +2143,18 @@ oop nmethod::oop_at(int index) const { if (index == 0) { return nullptr; } - return NMethodAccess::oop_load(oop_addr_at(index)); + + BarrierSetNMethod* bs_nm = BarrierSet::barrier_set()->barrier_set_nmethod(); + return bs_nm->oop_load_no_keepalive(this, index); } oop nmethod::oop_at_phantom(int index) const { if (index == 0) { return nullptr; } - return NMethodAccess::oop_load(oop_addr_at(index)); + + BarrierSetNMethod* bs_nm = BarrierSet::barrier_set()->barrier_set_nmethod(); + return bs_nm->oop_load_phantom(this, index); } // diff --git a/src/hotspot/share/gc/shared/barrierSetNMethod.cpp b/src/hotspot/share/gc/shared/barrierSetNMethod.cpp index 4b81f321be3..c5d8a7e5401 100644 --- a/src/hotspot/share/gc/shared/barrierSetNMethod.cpp +++ b/src/hotspot/share/gc/shared/barrierSetNMethod.cpp @@ -212,3 +212,11 @@ bool BarrierSetNMethod::nmethod_osr_entry_barrier(nmethod* nm) { OrderAccess::cross_modify_fence(); return result; } + +oop BarrierSetNMethod::oop_load_no_keepalive(const nmethod* nm, int index) { + return NativeAccess::oop_load(nm->oop_addr_at(index)); +} + +oop BarrierSetNMethod::oop_load_phantom(const nmethod* nm, int index) { + return NativeAccess::oop_load(nm->oop_addr_at(index)); +} diff --git a/src/hotspot/share/gc/shared/barrierSetNMethod.hpp b/src/hotspot/share/gc/shared/barrierSetNMethod.hpp index d003abe9bbe..756dc43b3f1 100644 --- a/src/hotspot/share/gc/shared/barrierSetNMethod.hpp +++ b/src/hotspot/share/gc/shared/barrierSetNMethod.hpp @@ -26,6 +26,7 @@ #define SHARE_GC_SHARED_BARRIERSETNMETHOD_HPP #include "memory/allocation.hpp" +#include "oops/oopsHierarchy.hpp" #include "utilities/formatBuffer.hpp" #include "utilities/globalDefinitions.hpp" #include "utilities/sizes.hpp" @@ -57,6 +58,9 @@ public: void arm_all_nmethods(); + virtual oop oop_load_no_keepalive(const nmethod* nm, int index); + virtual oop oop_load_phantom(const nmethod* nm, int index); + #if INCLUDE_JVMCI bool verify_barrier(nmethod* nm, FormatBuffer<>& msg); #endif diff --git a/src/hotspot/share/gc/z/zBarrierSet.inline.hpp b/src/hotspot/share/gc/z/zBarrierSet.inline.hpp index 174cdfd9e90..f7baf85efbf 100644 --- a/src/hotspot/share/gc/z/zBarrierSet.inline.hpp +++ b/src/hotspot/share/gc/z/zBarrierSet.inline.hpp @@ -473,11 +473,7 @@ template inline oop ZBarrierSet::AccessBarrier::oop_load_not_in_heap(oop* p) { verify_decorators_absent(); - if (HasDecorator::value) { - return ZNMethod::load_oop(p, decorators); - } else { - return oop_load_not_in_heap((zpointer*)p); - } + return oop_load_not_in_heap((zpointer*)p); } template diff --git a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp index cdb17018756..0d6be2b789f 100644 --- a/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp +++ b/src/hotspot/share/gc/z/zBarrierSetNMethod.cpp @@ -97,3 +97,11 @@ int* ZBarrierSetNMethod::disarmed_guard_value_address() const { ByteSize ZBarrierSetNMethod::thread_disarmed_guard_value_offset() const { return ZThreadLocalData::nmethod_disarmed_offset(); } + +oop ZBarrierSetNMethod::oop_load_no_keepalive(const nmethod* nm, int index) { + return ZNMethod::oop_load_no_keepalive(nm, index); +} + +oop ZBarrierSetNMethod::oop_load_phantom(const nmethod* nm, int index) { + return ZNMethod::oop_load_phantom(nm, index); +} diff --git a/src/hotspot/share/gc/z/zBarrierSetNMethod.hpp b/src/hotspot/share/gc/z/zBarrierSetNMethod.hpp index ed9f06672e7..780d3772123 100644 --- a/src/hotspot/share/gc/z/zBarrierSetNMethod.hpp +++ b/src/hotspot/share/gc/z/zBarrierSetNMethod.hpp @@ -36,6 +36,9 @@ protected: public: virtual ByteSize thread_disarmed_guard_value_offset() const; virtual int* disarmed_guard_value_address() const; + + virtual oop oop_load_no_keepalive(const nmethod* nm, int index); + virtual oop oop_load_phantom(const nmethod* nm, int index); }; #endif // SHARE_GC_Z_ZBARRIERSETNMETHOD_HPP diff --git a/src/hotspot/share/gc/z/zNMethod.cpp b/src/hotspot/share/gc/z/zNMethod.cpp index 26f2ab96946..4ae440ea231 100644 --- a/src/hotspot/share/gc/z/zNMethod.cpp +++ b/src/hotspot/share/gc/z/zNMethod.cpp @@ -305,26 +305,32 @@ uintptr_t ZNMethod::color(nmethod* nm) { return (uintptr_t)bs_nm->guard_value(nm); } -oop ZNMethod::load_oop(oop* p, DecoratorSet decorators) { - assert((decorators & ON_WEAK_OOP_REF) == 0, - "nmethod oops have phantom strength, not weak"); - nmethod* const nm = CodeCache::find_nmethod((void*)p); - assert(nm != nullptr, "did not find nmethod"); +oop ZNMethod::oop_load_no_keepalive(const nmethod* nm, int index) { + return oop_load(nm, index, false /* keep_alive */); +} + +oop ZNMethod::oop_load_phantom(const nmethod* nm, int index) { + return oop_load(nm, index, true /* keep_alive */); +} + +oop ZNMethod::oop_load(const nmethod* const_nm, int index, bool keep_alive) { + // The rest of the code is not ready to handle const nmethod, so cast it away + // until we are more consistent with our const corectness. + nmethod* nm = const_cast(const_nm); + if (!is_armed(nm)) { // If the nmethod entry barrier isn't armed, then it has been applied // already. The implication is that the contents of the memory location // is already a valid oop, and the barrier would have kept it alive if // necessary. Therefore, no action is required, and we are allowed to // simply read the oop. - return *p; + return *nm->oop_addr_at(index); } - const bool keep_alive = (decorators & ON_PHANTOM_OOP_REF) != 0 && - (decorators & AS_NO_KEEPALIVE) == 0; ZLocker locker(ZNMethod::lock_for_nmethod(nm)); // Make a local root - zaddress_unsafe obj = *ZUncoloredRoot::cast(p); + zaddress_unsafe obj = *ZUncoloredRoot::cast(nm->oop_addr_at(index)); if (keep_alive) { ZUncoloredRoot::process(&obj, ZNMethod::color(nm)); diff --git a/src/hotspot/share/gc/z/zNMethod.hpp b/src/hotspot/share/gc/z/zNMethod.hpp index 2c92e1f5efb..5d797f3fd55 100644 --- a/src/hotspot/share/gc/z/zNMethod.hpp +++ b/src/hotspot/share/gc/z/zNMethod.hpp @@ -42,6 +42,8 @@ private: static void log_unregister(const nmethod* nm); static void log_purge(const nmethod* nm); + static oop oop_load(const nmethod* nm, int index, bool keep_alive); + public: static void register_nmethod(nmethod* nm); static void unregister_nmethod(nmethod* nm); @@ -69,7 +71,9 @@ public: static void purge(); static uintptr_t color(nmethod* nm); - static oop load_oop(oop* p, DecoratorSet decorators); + + static oop oop_load_no_keepalive(const nmethod* nm, int index); + static oop oop_load_phantom(const nmethod* nm, int index); }; #endif // SHARE_GC_Z_ZNMETHOD_HPP diff --git a/src/hotspot/share/oops/access.hpp b/src/hotspot/share/oops/access.hpp index 34393462d03..917627f445e 100644 --- a/src/hotspot/share/oops/access.hpp +++ b/src/hotspot/share/oops/access.hpp @@ -284,11 +284,6 @@ class HeapAccess: public Access {}; template class NativeAccess: public Access {}; -// Helper for performing accesses in nmethods. These accesses -// may resolve an accessor on a GC barrier set. -template -class NMethodAccess: public Access {}; - // Helper for array access. template class ArrayAccess: public HeapAccess { @@ -366,7 +361,6 @@ void Access::verify_decorators() { const DecoratorSet location_decorators = decorators & IN_DECORATOR_MASK; STATIC_ASSERT(location_decorators == 0 || ( // make sure location decorators are disjoint if set (location_decorators ^ IN_NATIVE) == 0 || - (location_decorators ^ IN_NMETHOD) == 0 || (location_decorators ^ IN_HEAP) == 0 )); } diff --git a/src/hotspot/share/oops/accessDecorators.hpp b/src/hotspot/share/oops/accessDecorators.hpp index fc5860d6487..3d89b5c4334 100644 --- a/src/hotspot/share/oops/accessDecorators.hpp +++ b/src/hotspot/share/oops/accessDecorators.hpp @@ -173,11 +173,9 @@ const DecoratorSet ON_DECORATOR_MASK = ON_STRONG_OOP_REF | ON_WEAK_OOP_REF | // * IN_HEAP: The access is performed in the heap. Many barriers such as card marking will // be omitted if this decorator is not set. // * IN_NATIVE: The access is performed in an off-heap data structure. -// * IN_NMETHOD: The access is performed inside of an nmethod. const DecoratorSet IN_HEAP = UCONST64(1) << 18; const DecoratorSet IN_NATIVE = UCONST64(1) << 19; -const DecoratorSet IN_NMETHOD = UCONST64(1) << 20; -const DecoratorSet IN_DECORATOR_MASK = IN_HEAP | IN_NATIVE | IN_NMETHOD; +const DecoratorSet IN_DECORATOR_MASK = IN_HEAP | IN_NATIVE; // == Boolean Flag Decorators == // * IS_ARRAY: The access is performed on a heap allocated array. This is sometimes a special case @@ -185,9 +183,9 @@ const DecoratorSet IN_DECORATOR_MASK = IN_HEAP | IN_NATIVE | IN_NMETHOD; // * IS_DEST_UNINITIALIZED: This property can be important to e.g. SATB barriers by // marking that the previous value is uninitialized nonsense rather than a real value. // * IS_NOT_NULL: This property can make certain barriers faster such as compressing oops. -const DecoratorSet IS_ARRAY = UCONST64(1) << 21; -const DecoratorSet IS_DEST_UNINITIALIZED = UCONST64(1) << 22; -const DecoratorSet IS_NOT_NULL = UCONST64(1) << 23; +const DecoratorSet IS_ARRAY = UCONST64(1) << 20; +const DecoratorSet IS_DEST_UNINITIALIZED = UCONST64(1) << 21; +const DecoratorSet IS_NOT_NULL = UCONST64(1) << 22; // == Arraycopy Decorators == // * ARRAYCOPY_CHECKCAST: This property means that the class of the objects in source @@ -199,11 +197,11 @@ const DecoratorSet IS_NOT_NULL = UCONST64(1) << 23; // * ARRAYCOPY_ARRAYOF: The copy is in the arrayof form. // * ARRAYCOPY_ATOMIC: The accesses have to be atomic over the size of its elements. // * ARRAYCOPY_ALIGNED: The accesses have to be aligned on a HeapWord. -const DecoratorSet ARRAYCOPY_CHECKCAST = UCONST64(1) << 24; -const DecoratorSet ARRAYCOPY_DISJOINT = UCONST64(1) << 25; -const DecoratorSet ARRAYCOPY_ARRAYOF = UCONST64(1) << 26; -const DecoratorSet ARRAYCOPY_ATOMIC = UCONST64(1) << 27; -const DecoratorSet ARRAYCOPY_ALIGNED = UCONST64(1) << 28; +const DecoratorSet ARRAYCOPY_CHECKCAST = UCONST64(1) << 23; +const DecoratorSet ARRAYCOPY_DISJOINT = UCONST64(1) << 24; +const DecoratorSet ARRAYCOPY_ARRAYOF = UCONST64(1) << 25; +const DecoratorSet ARRAYCOPY_ATOMIC = UCONST64(1) << 26; +const DecoratorSet ARRAYCOPY_ALIGNED = UCONST64(1) << 27; const DecoratorSet ARRAYCOPY_DECORATOR_MASK = ARRAYCOPY_CHECKCAST | ARRAYCOPY_DISJOINT | ARRAYCOPY_DISJOINT | ARRAYCOPY_ARRAYOF | ARRAYCOPY_ATOMIC | ARRAYCOPY_ALIGNED; @@ -212,11 +210,11 @@ const DecoratorSet ARRAYCOPY_DECORATOR_MASK = ARRAYCOPY_CHECKCAST | ARRAYC // * ACCESS_READ: Indicate that the resolved object is accessed read-only. This allows the GC // backend to use weaker and more efficient barriers. // * ACCESS_WRITE: Indicate that the resolved object is used for write access. -const DecoratorSet ACCESS_READ = UCONST64(1) << 29; -const DecoratorSet ACCESS_WRITE = UCONST64(1) << 30; +const DecoratorSet ACCESS_READ = UCONST64(1) << 28; +const DecoratorSet ACCESS_WRITE = UCONST64(1) << 29; // Keep track of the last decorator. -const DecoratorSet DECORATOR_LAST = UCONST64(1) << 30; +const DecoratorSet DECORATOR_LAST = UCONST64(1) << 29; namespace AccessInternal { // This class adds implied decorators that follow according to decorator rules.