On Wed, 2 Jul 2025 07:16:57 GMT, David Holmes <dhol...@openjdk.org> wrote:
> 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.
Straight-forward solution would be to add copy ctor for Iterator class which
clones GrowableArray (allocates new instance and copies all elements).
But I'm not quite happy with the current implementation of AgentList and its
iterator, so prefer to update it to keep entries in the original order (and
iterator becomes much simpler).
Changes in the callers are required as they used `const
JvmtiAgentList::Iterator` and const `next` method is removed.
> 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.
pointer to pointer because it contains not the pointer to the last agent in the
list, but address of the `JvmtiAgent::_next` field.
`_tail` is for faster adding at the end of the list. Performance is not
important here and number of agents is always low, so I think `_tail` can be
removed to make the logic simpler. Will update the fix after testing completes
> 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.
ok, thanks
-------------
PR Comment: https://git.openjdk.org/jdk/pull/26083#issuecomment-3029654109
PR Review Comment: https://git.openjdk.org/jdk/pull/26083#discussion_r2181149878
PR Review Comment: https://git.openjdk.org/jdk/pull/26083#discussion_r2181149860
