On Mon, 18 Sep 2023 12:11:24 GMT, Axel Boldt-Christmas <abold...@openjdk.org>
wrote:
>> ObjectMonitorIterator fails to return the most resent monitor added. It
>> start with returning the `nextOM()` ObjectMonitor from the `_head`
>> ObjectMonitor but fails to ever return the `_head` ObjectMonitor.
>> The current implementation can also not handle that the `_head` is nullptr
>> (no monitors in the system) and returns a null ObjectMonitorIterator. Which
>> is interpreted as `monitor list not supported, too old hotspot VM`.
>>
>> Changed the iterator to keep return the current monitor (starts with
>> `_head`) and decoupled `_head == nullptr` from the question if
>> ObjectMonitorIterator is supported.
>>
>> Testing:
>> * Passes all `serviceability/sa` tests
>> * Currently running tier 1-3
>> * Currently running GHA
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Copyright changes too
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java
line 51:
> 49: Address monitorListAddr =
> objectSynchronizerType.getField("_in_use_list").getStaticFieldAddress();
> 50: inUseListHead =
> monitorListType.getAddressField("_head").getAddress(monitorListAddr);
> 51: isSupported = true;
I understand the changes below, but I'm a bit less clear why the above changes
were needed. Is this to help simplyfy the iterator logic below and make it
easier to check for a null list?
I'm not so sure I like the `isSupported` logic. It seems we should throw an
exception at some point if initialize() fails. I think in other classes when
some part of initialize() fails, we just allow the NPE to happen when later
logic tries to access the value. The check in objectMontitorIterator() was
probably a somewhat misguided attempted to be compatible with older VMs, which
is not something we really strive for anymore.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1329146543
