From ca0db02bd08c398395ad7517545016dc07a677a3 Mon Sep 17 00:00:00 2001 From: Patricio Chilano Mateo Date: Tue, 2 Jul 2024 23:56:44 -0400 Subject: [PATCH 1/5] v1 --- src/hotspot/share/interpreter/oopMapCache.cpp | 37 ++++++++----------- src/hotspot/share/interpreter/oopMapCache.hpp | 12 +++--- src/hotspot/share/runtime/frame.cpp | 1 - 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/hotspot/share/interpreter/oopMapCache.cpp b/src/hotspot/share/interpreter/oopMapCache.cpp index a10f8c439eaa4..1694596961c35 100644 --- a/src/hotspot/share/interpreter/oopMapCache.cpp +++ b/src/hotspot/share/interpreter/oopMapCache.cpp @@ -66,9 +66,6 @@ class OopMapCacheEntry: private InterpreterOopMap { public: OopMapCacheEntry() : InterpreterOopMap() { _next = nullptr; -#ifdef ASSERT - _resource_allocate_bit_mask = false; -#endif } }; @@ -178,10 +175,18 @@ class VerifyClosure : public OffsetClosure { InterpreterOopMap::InterpreterOopMap() { initialize(); #ifdef ASSERT - _resource_allocate_bit_mask = true; + _used = false; #endif } +InterpreterOopMap::~InterpreterOopMap() { + if (mask_size() > small_mask_limit) { + assert(!Thread::current()->resource_area()->contains((void*)_bit_mask[0]), + "The bit mask should be allocated from the C heap"); + FREE_C_HEAP_ARRAY(uintptr_t, _bit_mask[0]); + } +} + bool InterpreterOopMap::is_empty() const { bool result = _method == nullptr; assert(_method != nullptr || (_bci == 0 && @@ -400,10 +405,10 @@ void OopMapCacheEntry::deallocate(OopMapCacheEntry* const entry) { // Implementation of OopMapCache void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) { - assert(_resource_allocate_bit_mask, - "Should not resource allocate the _bit_mask"); - assert(from->has_valid_mask(), - "Cannot copy entry with an invalid mask"); + // The expectation is that this InterpreterOopMap is a recently created + // and empty. It is used to get a copy of a cached entry. + assert(!_used, "InterpreterOopMap object can only be filled once"); + assert(from->has_valid_mask(), "Cannot copy entry with an invalid mask"); set_method(from->method()); set_bci(from->bci()); @@ -416,21 +421,11 @@ void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) { memcpy((void *)_bit_mask, (void *)from->_bit_mask, mask_word_size() * BytesPerWord); } else { - // The expectation is that this InterpreterOopMap is a recently created - // and empty. It is used to get a copy of a cached entry. - // If the bit mask has a value, it should be in the - // resource area. - assert(_bit_mask[0] == 0 || - Thread::current()->resource_area()->contains((void*)_bit_mask[0]), - "The bit mask should have been allocated from a resource area"); - // Allocate the bit_mask from a Resource area for performance. Allocating - // from the C heap as is done for OopMapCache has a significant - // performance impact. - _bit_mask[0] = (uintptr_t) NEW_RESOURCE_ARRAY(uintptr_t, mask_word_size()); + _bit_mask[0] = (uintptr_t) NEW_C_HEAP_ARRAY(uintptr_t, mask_word_size(), mtClass); assert(_bit_mask[0] != 0, "bit mask was not allocated"); - memcpy((void*) _bit_mask[0], (void*) from->_bit_mask[0], - mask_word_size() * BytesPerWord); + memcpy((void*) _bit_mask[0], (void*) from->_bit_mask[0], mask_word_size() * BytesPerWord); } + DEBUG_ONLY(_used = true); } inline unsigned int OopMapCache::hash_value_for(const methodHandle& method, int bci) const { diff --git a/src/hotspot/share/interpreter/oopMapCache.hpp b/src/hotspot/share/interpreter/oopMapCache.hpp index 7037b2c7d1fcf..4bdef4d36febf 100644 --- a/src/hotspot/share/interpreter/oopMapCache.hpp +++ b/src/hotspot/share/interpreter/oopMapCache.hpp @@ -36,12 +36,13 @@ // OopMapCache's are allocated lazily per InstanceKlass. // The oopMap (InterpreterOopMap) is stored as a bit mask. If the -// bit_mask can fit into two words it is stored in +// bit_mask can fit into four words it is stored in // the _bit_mask array, otherwise it is allocated on the heap. // For OopMapCacheEntry the bit_mask is allocated in the C heap // because these entries persist between garbage collections. -// For InterpreterOopMap the bit_mask is allocated in -// a resource area for better performance. InterpreterOopMap +// For InterpreterOopMap the bit_mask is allocated in the C heap +// to avoid issues with allocations from the resource area that have +// to live accross the oop closure (see 8335409). InterpreterOopMap // should only be created and deleted during same garbage collection. // // If ENABBLE_ZAP_DEAD_LOCALS is defined, two bits are used @@ -89,7 +90,7 @@ class InterpreterOopMap: ResourceObj { protected: #ifdef ASSERT - bool _resource_allocate_bit_mask; + bool _used; #endif int _num_oops; intptr_t _bit_mask[N]; // the bit mask if @@ -128,11 +129,12 @@ class InterpreterOopMap: ResourceObj { public: InterpreterOopMap(); + ~InterpreterOopMap(); // Copy the OopMapCacheEntry in parameter "from" into this // InterpreterOopMap. If the _bit_mask[0] in "from" points to // allocated space (i.e., the bit mask was to large to hold - // in-line), allocate the space from a Resource area. + // in-line), allocate the space from the C heap. void resource_copy(OopMapCacheEntry* from); void iterate_oop(OffsetClosure* oop_closure) const; diff --git a/src/hotspot/share/runtime/frame.cpp b/src/hotspot/share/runtime/frame.cpp index 8f5d2ad4acbf1..1aed46d58804f 100644 --- a/src/hotspot/share/runtime/frame.cpp +++ b/src/hotspot/share/runtime/frame.cpp @@ -947,7 +947,6 @@ void frame::oops_interpreted_do(OopClosure* f, const RegisterMap* map, bool quer InterpreterFrameClosure blk(this, max_locals, m->max_stack(), f); // process locals & expression stack - ResourceMark rm(thread); InterpreterOopMap mask; if (query_oop_map_cache) { m->mask_for(m, bci, &mask); From 127ae6e68621dd8167ccb6104f369231ade0a9ce Mon Sep 17 00:00:00 2001 From: Patricio Chilano Mateo Date: Fri, 5 Jul 2024 09:57:52 -0400 Subject: [PATCH 2/5] David's comments --- src/hotspot/share/interpreter/oopMapCache.cpp | 6 ++---- src/hotspot/share/interpreter/oopMapCache.hpp | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/interpreter/oopMapCache.cpp b/src/hotspot/share/interpreter/oopMapCache.cpp index 1694596961c35..48d27f37e9e7e 100644 --- a/src/hotspot/share/interpreter/oopMapCache.cpp +++ b/src/hotspot/share/interpreter/oopMapCache.cpp @@ -174,9 +174,7 @@ class VerifyClosure : public OffsetClosure { InterpreterOopMap::InterpreterOopMap() { initialize(); -#ifdef ASSERT - _used = false; -#endif + DEBUG_ONLY(_used = false;) } InterpreterOopMap::~InterpreterOopMap() { @@ -405,7 +403,7 @@ void OopMapCacheEntry::deallocate(OopMapCacheEntry* const entry) { // Implementation of OopMapCache void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) { - // The expectation is that this InterpreterOopMap is a recently created + // The expectation is that this InterpreterOopMap is recently created // and empty. It is used to get a copy of a cached entry. assert(!_used, "InterpreterOopMap object can only be filled once"); assert(from->has_valid_mask(), "Cannot copy entry with an invalid mask"); diff --git a/src/hotspot/share/interpreter/oopMapCache.hpp b/src/hotspot/share/interpreter/oopMapCache.hpp index 4bdef4d36febf..a1e28826e50cb 100644 --- a/src/hotspot/share/interpreter/oopMapCache.hpp +++ b/src/hotspot/share/interpreter/oopMapCache.hpp @@ -133,7 +133,7 @@ class InterpreterOopMap: ResourceObj { // Copy the OopMapCacheEntry in parameter "from" into this // InterpreterOopMap. If the _bit_mask[0] in "from" points to - // allocated space (i.e., the bit mask was to large to hold + // allocated space (i.e., the bit mask was too large to hold // in-line), allocate the space from the C heap. void resource_copy(OopMapCacheEntry* from); From 805358e7463d831c76010bfefdc8da1fe3733423 Mon Sep 17 00:00:00 2001 From: Patricio Chilano Mateo Date: Fri, 5 Jul 2024 10:03:56 -0400 Subject: [PATCH 3/5] Coleen's comments --- src/hotspot/share/interpreter/oopMapCache.cpp | 26 +++++++++---------- src/hotspot/share/interpreter/oopMapCache.hpp | 10 +++---- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/hotspot/share/interpreter/oopMapCache.cpp b/src/hotspot/share/interpreter/oopMapCache.cpp index 48d27f37e9e7e..86364f1975741 100644 --- a/src/hotspot/share/interpreter/oopMapCache.cpp +++ b/src/hotspot/share/interpreter/oopMapCache.cpp @@ -402,26 +402,26 @@ void OopMapCacheEntry::deallocate(OopMapCacheEntry* const entry) { // Implementation of OopMapCache -void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) { +void InterpreterOopMap::copy_from(OopMapCacheEntry* src) { // The expectation is that this InterpreterOopMap is recently created // and empty. It is used to get a copy of a cached entry. assert(!_used, "InterpreterOopMap object can only be filled once"); - assert(from->has_valid_mask(), "Cannot copy entry with an invalid mask"); + assert(src->has_valid_mask(), "Cannot copy entry with an invalid mask"); - set_method(from->method()); - set_bci(from->bci()); - set_mask_size(from->mask_size()); - set_expression_stack_size(from->expression_stack_size()); - _num_oops = from->num_oops(); + set_method(src->method()); + set_bci(src->bci()); + set_mask_size(src->mask_size()); + set_expression_stack_size(src->expression_stack_size()); + _num_oops = src->num_oops(); // Is the bit mask contained in the entry? - if (from->mask_size() <= small_mask_limit) { - memcpy((void *)_bit_mask, (void *)from->_bit_mask, + if (src->mask_size() <= small_mask_limit) { + memcpy((void *)_bit_mask, (void *)src->_bit_mask, mask_word_size() * BytesPerWord); } else { _bit_mask[0] = (uintptr_t) NEW_C_HEAP_ARRAY(uintptr_t, mask_word_size(), mtClass); assert(_bit_mask[0] != 0, "bit mask was not allocated"); - memcpy((void*) _bit_mask[0], (void*) from->_bit_mask[0], mask_word_size() * BytesPerWord); + memcpy((void*) _bit_mask[0], (void*) src->_bit_mask[0], mask_word_size() * BytesPerWord); } DEBUG_ONLY(_used = true); } @@ -505,7 +505,7 @@ void OopMapCache::lookup(const methodHandle& method, for (int i = 0; i < probe_depth; i++) { OopMapCacheEntry *entry = entry_at(probe + i); if (entry != nullptr && !entry->is_empty() && entry->match(method, bci)) { - entry_for->resource_copy(entry); + entry_for->copy_from(entry); assert(!entry_for->is_empty(), "A non-empty oop map should be returned"); log_debug(interpreter, oopmap)("- found at hash %d", probe + i); return; @@ -519,7 +519,7 @@ void OopMapCache::lookup(const methodHandle& method, OopMapCacheEntry* tmp = NEW_C_HEAP_OBJ(OopMapCacheEntry, mtClass); tmp->initialize(); tmp->fill(method, bci); - entry_for->resource_copy(tmp); + entry_for->copy_from(tmp); if (method->should_not_be_cached()) { // It is either not safe or not a good idea to cache this Method* @@ -620,7 +620,7 @@ void OopMapCache::compute_one_oop_map(const methodHandle& method, int bci, Inter tmp->initialize(); tmp->fill(method, bci); if (tmp->has_valid_mask()) { - entry->resource_copy(tmp); + entry->copy_from(tmp); } OopMapCacheEntry::deallocate(tmp); } diff --git a/src/hotspot/share/interpreter/oopMapCache.hpp b/src/hotspot/share/interpreter/oopMapCache.hpp index a1e28826e50cb..aef58e65e6409 100644 --- a/src/hotspot/share/interpreter/oopMapCache.hpp +++ b/src/hotspot/share/interpreter/oopMapCache.hpp @@ -42,8 +42,8 @@ // because these entries persist between garbage collections. // For InterpreterOopMap the bit_mask is allocated in the C heap // to avoid issues with allocations from the resource area that have -// to live accross the oop closure (see 8335409). InterpreterOopMap -// should only be created and deleted during same garbage collection. +// to live accross the oop closure. InterpreterOopMap should only be +// created and deleted during the same garbage collection. // // If ENABBLE_ZAP_DEAD_LOCALS is defined, two bits are used // per entry instead of one. In all cases, @@ -131,11 +131,11 @@ class InterpreterOopMap: ResourceObj { InterpreterOopMap(); ~InterpreterOopMap(); - // Copy the OopMapCacheEntry in parameter "from" into this - // InterpreterOopMap. If the _bit_mask[0] in "from" points to + // Copy the OopMapCacheEntry in parameter "src" into this + // InterpreterOopMap. If the _bit_mask[0] in "src" points to // allocated space (i.e., the bit mask was too large to hold // in-line), allocate the space from the C heap. - void resource_copy(OopMapCacheEntry* from); + void copy_from(OopMapCacheEntry* src); void iterate_oop(OffsetClosure* oop_closure) const; void print() const; From 7ce559cb317499782089c9dde1559a58160ebf22 Mon Sep 17 00:00:00 2001 From: Patricio Chilano Mateo Date: Fri, 5 Jul 2024 10:53:15 -0400 Subject: [PATCH 4/5] use DEBUG_ONLY on _used declaration --- src/hotspot/share/interpreter/oopMapCache.hpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/interpreter/oopMapCache.hpp b/src/hotspot/share/interpreter/oopMapCache.hpp index aef58e65e6409..13910e4194a55 100644 --- a/src/hotspot/share/interpreter/oopMapCache.hpp +++ b/src/hotspot/share/interpreter/oopMapCache.hpp @@ -89,11 +89,9 @@ class InterpreterOopMap: ResourceObj { unsigned short _bci; // the bci for which the mask is valid protected: -#ifdef ASSERT - bool _used; -#endif - int _num_oops; - intptr_t _bit_mask[N]; // the bit mask if + DEBUG_ONLY(bool _used;) + int _num_oops; + intptr_t _bit_mask[N]; // the bit mask if // mask_size <= small_mask_limit, // ptr to bit mask otherwise // "protected" so that sub classes can From 88d866ba798bef2b8a76262c105ae4e05278bb0f Mon Sep 17 00:00:00 2001 From: Patricio Chilano Mateo Date: Mon, 8 Jul 2024 14:04:20 -0400 Subject: [PATCH 5/5] address Thomas' comments --- src/hotspot/share/interpreter/oopMapCache.cpp | 15 +++++---------- src/hotspot/share/interpreter/oopMapCache.hpp | 7 +++---- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/hotspot/share/interpreter/oopMapCache.cpp b/src/hotspot/share/interpreter/oopMapCache.cpp index 86364f1975741..87b124e9d7968 100644 --- a/src/hotspot/share/interpreter/oopMapCache.cpp +++ b/src/hotspot/share/interpreter/oopMapCache.cpp @@ -174,13 +174,11 @@ class VerifyClosure : public OffsetClosure { InterpreterOopMap::InterpreterOopMap() { initialize(); - DEBUG_ONLY(_used = false;) } InterpreterOopMap::~InterpreterOopMap() { - if (mask_size() > small_mask_limit) { - assert(!Thread::current()->resource_area()->contains((void*)_bit_mask[0]), - "The bit mask should be allocated from the C heap"); + if (has_valid_mask() && mask_size() > small_mask_limit) { + assert(_bit_mask[0] != 0, "should have pointer to C heap"); FREE_C_HEAP_ARRAY(uintptr_t, _bit_mask[0]); } } @@ -402,10 +400,10 @@ void OopMapCacheEntry::deallocate(OopMapCacheEntry* const entry) { // Implementation of OopMapCache -void InterpreterOopMap::copy_from(OopMapCacheEntry* src) { +void InterpreterOopMap::copy_from(const OopMapCacheEntry* src) { // The expectation is that this InterpreterOopMap is recently created // and empty. It is used to get a copy of a cached entry. - assert(!_used, "InterpreterOopMap object can only be filled once"); + assert(!has_valid_mask(), "InterpreterOopMap object can only be filled once"); assert(src->has_valid_mask(), "Cannot copy entry with an invalid mask"); set_method(src->method()); @@ -416,14 +414,11 @@ void InterpreterOopMap::copy_from(OopMapCacheEntry* src) { // Is the bit mask contained in the entry? if (src->mask_size() <= small_mask_limit) { - memcpy((void *)_bit_mask, (void *)src->_bit_mask, - mask_word_size() * BytesPerWord); + memcpy(_bit_mask, src->_bit_mask, mask_word_size() * BytesPerWord); } else { _bit_mask[0] = (uintptr_t) NEW_C_HEAP_ARRAY(uintptr_t, mask_word_size(), mtClass); - assert(_bit_mask[0] != 0, "bit mask was not allocated"); memcpy((void*) _bit_mask[0], (void*) src->_bit_mask[0], mask_word_size() * BytesPerWord); } - DEBUG_ONLY(_used = true); } inline unsigned int OopMapCache::hash_value_for(const methodHandle& method, int bci) const { diff --git a/src/hotspot/share/interpreter/oopMapCache.hpp b/src/hotspot/share/interpreter/oopMapCache.hpp index 13910e4194a55..062b4178ce06e 100644 --- a/src/hotspot/share/interpreter/oopMapCache.hpp +++ b/src/hotspot/share/interpreter/oopMapCache.hpp @@ -89,9 +89,8 @@ class InterpreterOopMap: ResourceObj { unsigned short _bci; // the bci for which the mask is valid protected: - DEBUG_ONLY(bool _used;) - int _num_oops; - intptr_t _bit_mask[N]; // the bit mask if + int _num_oops; + intptr_t _bit_mask[N]; // the bit mask if // mask_size <= small_mask_limit, // ptr to bit mask otherwise // "protected" so that sub classes can @@ -133,7 +132,7 @@ class InterpreterOopMap: ResourceObj { // InterpreterOopMap. If the _bit_mask[0] in "src" points to // allocated space (i.e., the bit mask was too large to hold // in-line), allocate the space from the C heap. - void copy_from(OopMapCacheEntry* src); + void copy_from(const OopMapCacheEntry* src); void iterate_oop(OffsetClosure* oop_closure) const; void print() const;