From 14557e72ef55c6161a3fa0c1960f7be618a34bf1 Mon Sep 17 00:00:00 2001 From: Afshin Zafari Date: Thu, 23 Nov 2023 22:16:57 +0000 Subject: [PATCH] 8314502: Change the comparator taking version of GrowableArray::find to be a template method Reviewed-by: jsjolen, sspitsyn, stefank --- .../share/gc/parallel/mutableNUMASpace.cpp | 13 ++-- .../share/gc/parallel/mutableNUMASpace.hpp | 6 +- src/hotspot/share/prims/jvmtiImpl.cpp | 14 +---- src/hotspot/share/prims/jvmtiImpl.hpp | 14 ++--- src/hotspot/share/runtime/perfData.cpp | 14 ++--- src/hotspot/share/runtime/perfData.hpp | 5 +- src/hotspot/share/runtime/unhandledOops.cpp | 13 ++-- src/hotspot/share/runtime/unhandledOops.hpp | 7 ++- .../share/services/diagnosticFramework.cpp | 5 +- .../share/services/diagnosticFramework.hpp | 3 +- src/hotspot/share/services/management.cpp | 4 +- src/hotspot/share/utilities/growableArray.hpp | 20 ++++-- .../gtest/utilities/test_growableArray.cpp | 62 +++++++++++++++++++ 13 files changed, 123 insertions(+), 57 deletions(-) diff --git a/src/hotspot/share/gc/parallel/mutableNUMASpace.cpp b/src/hotspot/share/gc/parallel/mutableNUMASpace.cpp index 93fa7c55190..2a39ce7b470 100644 --- a/src/hotspot/share/gc/parallel/mutableNUMASpace.cpp +++ b/src/hotspot/share/gc/parallel/mutableNUMASpace.cpp @@ -142,6 +142,11 @@ size_t MutableNUMASpace::free_in_words() const { return s; } +int MutableNUMASpace::lgrp_space_index(int lgrp_id) const { + return lgrp_spaces()->find_if([&](LGRPSpace* space) { + return space->lgrp_id() == checked_cast(lgrp_id); + }); +} size_t MutableNUMASpace::tlab_capacity(Thread *thr) const { guarantee(thr != nullptr, "No thread"); @@ -160,7 +165,7 @@ size_t MutableNUMASpace::tlab_capacity(Thread *thr) const { } } // That's the normal case, where we know the locality group of the thread. - int i = lgrp_spaces()->find(&lgrp_id, LGRPSpace::equals); + int i = lgrp_space_index(lgrp_id); if (i == -1) { return 0; } @@ -179,7 +184,7 @@ size_t MutableNUMASpace::tlab_used(Thread *thr) const { return 0; } } - int i = lgrp_spaces()->find(&lgrp_id, LGRPSpace::equals); + int i = lgrp_space_index(lgrp_id); if (i == -1) { return 0; } @@ -199,7 +204,7 @@ size_t MutableNUMASpace::unsafe_max_tlab_alloc(Thread *thr) const { return 0; } } - int i = lgrp_spaces()->find(&lgrp_id, LGRPSpace::equals); + int i = lgrp_space_index(lgrp_id); if (i == -1) { return 0; } @@ -569,7 +574,7 @@ HeapWord* MutableNUMASpace::cas_allocate(size_t size) { thr->set_lgrp_id(lgrp_id); } - int i = lgrp_spaces()->find(&lgrp_id, LGRPSpace::equals); + int i = lgrp_space_index(lgrp_id); // It is possible that a new CPU has been hotplugged and // we haven't reshaped the space accordingly. if (i == -1) { diff --git a/src/hotspot/share/gc/parallel/mutableNUMASpace.hpp b/src/hotspot/share/gc/parallel/mutableNUMASpace.hpp index 8ed25006ccf..77ecb4da466 100644 --- a/src/hotspot/share/gc/parallel/mutableNUMASpace.hpp +++ b/src/hotspot/share/gc/parallel/mutableNUMASpace.hpp @@ -93,10 +93,6 @@ class MutableNUMASpace : public MutableSpace { delete _alloc_rate; } - static bool equals(void* lgrp_id_value, LGRPSpace* p) { - return *(uint*)lgrp_id_value == p->lgrp_id(); - } - // Report a failed allocation. void set_allocation_failed() { _allocation_failed = true; } @@ -158,6 +154,8 @@ class MutableNUMASpace : public MutableSpace { void select_tails(MemRegion new_region, MemRegion intersection, MemRegion* bottom_region, MemRegion *top_region); + int lgrp_space_index(int lgrp_id) const; + public: GrowableArray* lgrp_spaces() const { return _lgrp_spaces; } MutableNUMASpace(size_t alignment); diff --git a/src/hotspot/share/prims/jvmtiImpl.cpp b/src/hotspot/share/prims/jvmtiImpl.cpp index c9c3974b591..21122539af8 100644 --- a/src/hotspot/share/prims/jvmtiImpl.cpp +++ b/src/hotspot/share/prims/jvmtiImpl.cpp @@ -119,14 +119,6 @@ void GrowableCache::recache() { _listener_fun(_this_obj,_cache); } -bool GrowableCache::equals(void* v, GrowableElement *e2) { - GrowableElement *e1 = (GrowableElement *) v; - assert(e1 != nullptr, "e1 != nullptr"); - assert(e2 != nullptr, "e2 != nullptr"); - - return e1->equals(e2); -} - // // class GrowableCache - public methods // @@ -163,8 +155,8 @@ GrowableElement* GrowableCache::at(int index) { return e; } -int GrowableCache::find(GrowableElement* e) { - return _elements->find(e, GrowableCache::equals); +int GrowableCache::find(const GrowableElement* e) const { + return _elements->find_if([&](const GrowableElement* other_e) { return e->equals(other_e); }); } // append a copy of the element to the end of the collection @@ -216,7 +208,7 @@ void JvmtiBreakpoint::copy(JvmtiBreakpoint& bp) { _class_holder = OopHandle(JvmtiExport::jvmti_oop_storage(), bp._class_holder.resolve()); } -bool JvmtiBreakpoint::equals(JvmtiBreakpoint& bp) { +bool JvmtiBreakpoint::equals(const JvmtiBreakpoint& bp) const { return _method == bp._method && _bci == bp._bci; } diff --git a/src/hotspot/share/prims/jvmtiImpl.hpp b/src/hotspot/share/prims/jvmtiImpl.hpp index 851dec72ee7..c93abe5eedf 100644 --- a/src/hotspot/share/prims/jvmtiImpl.hpp +++ b/src/hotspot/share/prims/jvmtiImpl.hpp @@ -66,9 +66,9 @@ class JvmtiBreakpoints; class GrowableElement : public CHeapObj { public: virtual ~GrowableElement() {} - virtual address getCacheValue() =0; - virtual bool equals(GrowableElement* e) =0; - virtual GrowableElement *clone() =0; + virtual address getCacheValue() =0; + virtual bool equals(const GrowableElement* e) const =0; + virtual GrowableElement* clone() =0; }; class GrowableCache { @@ -88,8 +88,6 @@ private: // (but NOT when cached elements are recomputed). void (*_listener_fun)(void *, address*); - static bool equals(void *, GrowableElement *); - // recache all elements after size change, notify listener void recache(); @@ -104,7 +102,7 @@ public: // get the value of the index element in the collection GrowableElement* at(int index); // find the index of the element, -1 if it doesn't exist - int find(GrowableElement* e); + int find(const GrowableElement* e) const; // append a copy of the element to the end of the collection, notify listener void append(GrowableElement* e); // remove the element at index, notify listener @@ -165,7 +163,7 @@ public: JvmtiBreakpoint() : _method(nullptr), _bci(0) {} JvmtiBreakpoint(Method* m_method, jlocation location); virtual ~JvmtiBreakpoint(); - bool equals(JvmtiBreakpoint& bp); + bool equals(const JvmtiBreakpoint& bp) const; void copy(JvmtiBreakpoint& bp); address getBcp() const; void each_method_version_do(method_action meth_act); @@ -177,7 +175,7 @@ public: // GrowableElement implementation address getCacheValue() { return getBcp(); } - bool equals(GrowableElement* e) { return equals((JvmtiBreakpoint&) *e); } + bool equals(const GrowableElement* e) const { return equals((const JvmtiBreakpoint&) *e); } GrowableElement *clone() { JvmtiBreakpoint *bp = new JvmtiBreakpoint(); diff --git a/src/hotspot/share/runtime/perfData.cpp b/src/hotspot/share/runtime/perfData.cpp index d1caa2edeab..e2549a11f10 100644 --- a/src/hotspot/share/runtime/perfData.cpp +++ b/src/hotspot/share/runtime/perfData.cpp @@ -185,6 +185,10 @@ void PerfData::create_entry(BasicType dtype, size_t dsize, size_t vlen) { PerfMemory::mark_updated(); } +bool PerfData::name_equals(const char* name) const { + return strcmp(name, this->name()) == 0; +} + PerfLong::PerfLong(CounterNS ns, const char* namep, Units u, Variability v) : PerfData(ns, namep, u, v) { @@ -501,17 +505,9 @@ PerfDataList::~PerfDataList() { } -bool PerfDataList::by_name(void* name, PerfData* pd) { - - if (pd == nullptr) - return false; - - return strcmp((const char*)name, pd->name()) == 0; -} - PerfData* PerfDataList::find_by_name(const char* name) { - int i = _set->find((void*)name, PerfDataList::by_name); + int i = _set->find_if([&](PerfData* pd) { return pd->name_equals(name); }); if (i >= 0 && i <= _set->length()) return _set->at(i); diff --git a/src/hotspot/share/runtime/perfData.hpp b/src/hotspot/share/runtime/perfData.hpp index 5c7b3d104e9..968e0813bdb 100644 --- a/src/hotspot/share/runtime/perfData.hpp +++ b/src/hotspot/share/runtime/perfData.hpp @@ -319,7 +319,8 @@ class PerfData : public CHeapObj { // PerfData memory region. This redundancy is maintained for // security reasons as the PerfMemory region may be in shared // memory. - const char* name() { return _name; } + const char* name() const { return _name; } + bool name_equals(const char* name) const; // returns the variability classification associated with this item Variability variability() { return _v; } @@ -576,7 +577,7 @@ class PerfDataList : public CHeapObj { PerfDataArray* _set; // method to search for a instrumentation object by name - static bool by_name(void* name, PerfData* pd); + static bool by_name(const char* name, PerfData* pd); protected: // we expose the implementation here to facilitate the clone diff --git a/src/hotspot/share/runtime/unhandledOops.cpp b/src/hotspot/share/runtime/unhandledOops.cpp index 11c8160fe03..cd1bde76c77 100644 --- a/src/hotspot/share/runtime/unhandledOops.cpp +++ b/src/hotspot/share/runtime/unhandledOops.cpp @@ -71,11 +71,6 @@ void UnhandledOops::register_unhandled_oop(oop* op) { _oop_list->push(entry); } - -bool match_oop_entry(void *op, UnhandledOopEntry e) { - return (e.oop_ptr() == op); -} - // Mark unhandled oop as okay for GC - the containing struct has an oops_do and // for some reason the oop has to be on the stack. // May not be called for the current thread, as in the case of @@ -83,7 +78,9 @@ bool match_oop_entry(void *op, UnhandledOopEntry e) { void UnhandledOops::allow_unhandled_oop(oop* op) { assert (CheckUnhandledOops, "should only be called with checking option"); - int i = _oop_list->find_from_end(op, match_oop_entry); + int i = _oop_list->find_from_end_if([&](const UnhandledOopEntry& e) { + return e.match_oop_entry(op); + }); assert(i!=-1, "safe for gc oop not in unhandled_oop_list"); UnhandledOopEntry entry = _oop_list->at(i); @@ -105,7 +102,9 @@ void UnhandledOops::unregister_unhandled_oop(oop* op) { } _level--; - int i = _oop_list->find_from_end(op, match_oop_entry); + int i = _oop_list->find_from_end_if([&](const UnhandledOopEntry& e) { + return e.match_oop_entry(op); + }); assert(i!=-1, "oop not in unhandled_oop_list"); _oop_list->remove_at(i); } diff --git a/src/hotspot/share/runtime/unhandledOops.hpp b/src/hotspot/share/runtime/unhandledOops.hpp index 400e6dd5543..09ebbe68be3 100644 --- a/src/hotspot/share/runtime/unhandledOops.hpp +++ b/src/hotspot/share/runtime/unhandledOops.hpp @@ -53,14 +53,17 @@ class UnhandledOopEntry : public CHeapObj { private: oop* _oop_ptr; bool _ok_for_gc; + + bool match_oop_entry(oop* op) const { + return _oop_ptr == op; + } + public: - oop* oop_ptr() { return _oop_ptr; } UnhandledOopEntry() : _oop_ptr(nullptr), _ok_for_gc(false) {} UnhandledOopEntry(oop* op) : _oop_ptr(op), _ok_for_gc(false) {} }; - class UnhandledOops : public CHeapObj { friend class Thread; private: diff --git a/src/hotspot/share/services/diagnosticFramework.cpp b/src/hotspot/share/services/diagnosticFramework.cpp index d17e426ee8a..006c08cb63f 100644 --- a/src/hotspot/share/services/diagnosticFramework.cpp +++ b/src/hotspot/share/services/diagnosticFramework.cpp @@ -144,9 +144,8 @@ bool DCmdArgIter::next(TRAPS) { return _key_len != 0; } -bool DCmdInfo::by_name(void* cmd_name, DCmdInfo* info) { - if (info == nullptr) return false; - return strcmp((const char*)cmd_name, info->name()) == 0; +bool DCmdInfo::name_equals(const char* name) const { + return strcmp(name, this->name()) == 0; } void DCmdParser::add_dcmd_option(GenDCmdArgument* arg) { diff --git a/src/hotspot/share/services/diagnosticFramework.hpp b/src/hotspot/share/services/diagnosticFramework.hpp index 8313954aaec..898f29274ea 100644 --- a/src/hotspot/share/services/diagnosticFramework.hpp +++ b/src/hotspot/share/services/diagnosticFramework.hpp @@ -140,13 +140,12 @@ public: : _name(name), _description(description), _impact(impact), _permission(permission), _num_arguments(num_arguments), _is_enabled(enabled) {} const char* name() const { return _name; } + bool name_equals(const char* cmd_name) const; const char* description() const { return _description; } const char* impact() const { return _impact; } const JavaPermission& permission() const { return _permission; } int num_arguments() const { return _num_arguments; } bool is_enabled() const { return _is_enabled; } - - static bool by_name(void* name, DCmdInfo* info); }; // A DCmdArgumentInfo instance provides a description of a diagnostic command diff --git a/src/hotspot/share/services/management.cpp b/src/hotspot/share/services/management.cpp index a9c50a8bf06..8e9249f5a61 100644 --- a/src/hotspot/share/services/management.cpp +++ b/src/hotspot/share/services/management.cpp @@ -2004,7 +2004,9 @@ JVM_ENTRY(void, jmm_GetDiagnosticCommandInfo(JNIEnv *env, jobjectArray cmds, THROW_MSG(vmSymbols::java_lang_NullPointerException(), "Command name cannot be null."); } - int pos = info_list->find((void*)cmd_name,DCmdInfo::by_name); + int pos = info_list->find_if([&](DCmdInfo* info) { + return info->name_equals(cmd_name); + }); if (pos == -1) { THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "Unknown diagnostic command"); diff --git a/src/hotspot/share/utilities/growableArray.hpp b/src/hotspot/share/utilities/growableArray.hpp index d9c47e8360f..e9abd9fae9a 100644 --- a/src/hotspot/share/utilities/growableArray.hpp +++ b/src/hotspot/share/utilities/growableArray.hpp @@ -209,17 +209,29 @@ public: return -1; } - int find(void* token, bool f(void*, E)) const { + // Find first element that matches the given predicate. + // + // Predicate: bool predicate(const E& elem) + // + // Returns the index of the element or -1 if no element matches the predicate + template + int find_if(Predicate predicate) const { for (int i = 0; i < _len; i++) { - if (f(token, _data[i])) return i; + if (predicate(_data[i])) return i; } return -1; } - int find_from_end(void* token, bool f(void*, E)) const { + // Find last element that matches the given predicate. + // + // Predicate: bool predicate(const E& elem) + // + // Returns the index of the element or -1 if no element matches the predicate + template + int find_from_end_if(Predicate predicate) const { // start at the end of the array for (int i = _len-1; i >= 0; i--) { - if (f(token, _data[i])) return i; + if (predicate(_data[i])) return i; } return -1; } diff --git a/test/hotspot/gtest/utilities/test_growableArray.cpp b/test/hotspot/gtest/utilities/test_growableArray.cpp index cb70dfb57b3..cd269e09212 100644 --- a/test/hotspot/gtest/utilities/test_growableArray.cpp +++ b/test/hotspot/gtest/utilities/test_growableArray.cpp @@ -601,3 +601,65 @@ TEST(GrowableArrayCHeap, sanity) { delete a; } } + +TEST(GrowableArrayCHeap, find_if) { + struct Element { + int value; + }; + GrowableArrayCHeap array; + array.push({1}); + array.push({2}); + array.push({3}); + + { + int index = array.find_if([&](const Element& elem) { + return elem.value == 1; + }); + ASSERT_EQ(index, 0); + } + + { + int index = array.find_if([&](const Element& elem) { + return elem.value > 1; + }); + ASSERT_EQ(index, 1); + } + + { + int index = array.find_if([&](const Element& elem) { + return elem.value == 4; + }); + ASSERT_EQ(index, -1); + } +} + +TEST(GrowableArrayCHeap, find_from_end_if) { + struct Element { + int value; + }; + GrowableArrayCHeap array; + array.push({1}); + array.push({2}); + array.push({3}); + + { + int index = array.find_from_end_if([&](const Element& elem) { + return elem.value == 1; + }); + ASSERT_EQ(index, 0); + } + + { + int index = array.find_from_end_if([&](const Element& elem) { + return elem.value > 1; + }); + ASSERT_EQ(index, 2); + } + + { + int index = array.find_from_end_if([&](const Element& elem) { + return elem.value == 4; + }); + ASSERT_EQ(index, -1); + } +}