On Wed, 2 Jul 2025 01:47:59 GMT, Alex Menkov <amen...@openjdk.org> wrote:
> Currently jvmtiAgentList keeps agents in reversed order (new agents are added
> to the head of the list).
> To restore original order JvmtiAgentList::Iterator uses GrowableArray
> allocated in heap.
> Iterators for different agent types are returned by value, and the iterator
> class nas no custom copy ctor, so if the constructor not elides,
> GrowableArray is deallocated twice.
>
> The fix updates jvmtiAgentList to keep agents in the original order, agents
> are added to the tail.
> Iterator now needs only single pointer to next agent.
> Additionally removed `JvmtiAgentList::Iterator::next() const` method (it
> looks very strange as `next()` is expected to change state of the iterator).
>
> Testing: tier1..4,hs-tier5-svc
I'm not very familiar with these aspects of C++ but this seems to be a very
complex solution to what sounds in JBS to be a pretty straight-forward problem.
src/hotspot/share/prims/jvmtiAgentList.cpp line 35:
> 33:
> 34: JvmtiAgent* JvmtiAgentList::_head = nullptr;
> 35: JvmtiAgent** JvmtiAgentList::_tail = nullptr;
Why is the tail a pointer to a pointer? Sorry I'm not understanding how your
list is being managed here.
I'm trying to work out where you need acquire/release because I'm pretty sure
it is missing where needed, but again I can't understand how you are actually
constructing and using the list.
src/hotspot/share/prims/jvmtiAgentList.cpp line 105:
> 103: while (true) {
> 104: // set _tail to address of agent->_next
> 105: JvmtiAgent** tail = Atomic::load_acquire(&_tail);
You don't need acquire semantics here as you are not doing anything with the
`_tail` pointer value other than use it in relation to cmpxchg which provides
fully memory barriers.
-------------
PR Review: https://git.openjdk.org/jdk/pull/26083#pullrequestreview-2977985314
PR Review Comment: https://git.openjdk.org/jdk/pull/26083#discussion_r2179305538
PR Review Comment: https://git.openjdk.org/jdk/pull/26083#discussion_r2179284415
