On Mon, 18 Sep 2023 18:46:19 GMT, Chris Plummer <[email protected]> wrote:
>> 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.
`inUseListHead` will be null both if `initialize` throws an exception and if
the monitor list is null. The previous semantics was to return null if we
failed to get the monitor list (because `initialize` failed). But with only
`inUseListHead` we cannot differentiate between the two states. If it is null
and `initialize` did not fail, an empty `ObjectMontiorIterator` should be
returned instead of null.
But if it is as you say that this is not a supported use case we could just
return an empty `ObjectMontiorIterator` for both cases and rely on the handling
of the exception thrown by `initialize`.
I did not want to change the interface. But I'll follow your initiative here
and remove the `null` return from `objectMonitorIterator()`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1329649000
