Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:21:25 GMT, Axel Boldt-Christmas 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 >> * Passed tier 1-5 >> * Passed GHA > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/15782#issuecomment-1736918541
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:21:25 GMT, Axel Boldt-Christmas 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 >> * Passed tier 1-5 >> * Passed GHA > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Looks good. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15782#pullrequestreview-1643345749
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Mon, 25 Sep 2023 14:26:00 GMT, Chris Plummer wrote: >> It is not. It is the value of `_head` when ObjectSynchronizer is >> initialised. My understanding of the serviceability agent is very limited, >> but from what I understood the JVM does not run when we are attached. So no >> code should add to the list. >> >> If that was a requirement then both this and the previous implementation are >> completely broken. > >> My understanding of the serviceability agent is very limited, but from what >> I understood the JVM does not run when we are attached. So no code should >> add to the list. > > Your understanding is correct, although one thing SA has to deal with > (usually by falling on its face) is that the VM could be in an inconsistent > state, and there is no safe pointing, locking, or other synchronization it > can do to protect from that. Thanks - as per another SA issue my understanding of how the SA operated on a live process was incorrect. - PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1336625511
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Mon, 25 Sep 2023 10:19:19 GMT, Axel Boldt-Christmas wrote: >> test/hotspot/jtreg/serviceability/sa/TestObjectMonitorIterate.java line 1: >> >>> 1: /* >> >> This test still barely actually tests the iterator code. > > Agreed. Did not want to spend time learning the SA, but maybe I should. So I > can write a proper test. > It does however seem very hard to write a proper test. Also not sure how to > make it non-flaky, maybe start a lingering App, let it reach steady state, > get the monitor list from some other API (maybe there is a jcmd, or we have > to use the whitebox API from the actual process and print it somewhere, and > hope the state of which monitors exists does not change), then attach the SA > and run and compare the lists. > > It seems hard to me to ensure that the SA agent prints all monitors in the > `_in_use_list` with a test. > > Or you would have to maintain a list of monitors for each locking mode and > the lingering app. Right. You would have to write the test as a state machine, very carefully orchestrating when monitors become in-use and when they cease to be in-use, and then attaching (the detaching) the SA when it reached each state and then checking the expected conditions for that state. Certainly a very non-trivial thing to try and do. - PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1336626565
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Mon, 25 Sep 2023 14:33:15 GMT, Chris Plummer wrote: > This code is called when you choose the hsdb "Tools -> Monitor Cache Dump" > menu item. Have you tried doing that? I tried running it against a clhsdb > process, and currently it doesn't seem to show any monitors. Does it show one > after your fix? I just tried with your fix and now it shows one monitor: Monitor Cache Dump (not including JVMTI raw monitors): ObjectMonitor@0x7f26040010b0 _header: 0x1 _object: 0xa0001750, a java/lang/ref/NativeReferenceQueue$Lock _owner: null _contentions: 0 _waiters: 1 _recursions: 0 - PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1336408860
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:21:25 GMT, Axel Boldt-Christmas
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
>> * Passed tier 1-5
>> * Passed GHA
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update copyright year
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/MonitorCacheDumpPanel.java
line 101:
> 99: tty.println("You need 1.4.0_04, 1.4.1_01 or later versions");
> 100: return;
> 101: }
This code is called when you choose the hsdb "Tools -> Monitor Cache Dump" menu
item. Have you tried doing that? I tried running it against a clhsdb process,
and currently it doesn't seem to show any monitors. Does it show one after your
fix?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1335978134
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Mon, 25 Sep 2023 10:05:12 GMT, Axel Boldt-Christmas wrote: > My understanding of the serviceability agent is very limited, but from what I > understood the JVM does not run when we are attached. So no code should add > to the list. Your understanding is correct, although one thing SA has to deal with (usually by falling on its face) is that the VM could be in an inconsistent state, and there is no safe pointing, locking, or other synchronization it can do to protect from that. - PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1335968189
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Mon, 25 Sep 2023 03:40:53 GMT, David Holmes wrote: >> Axel Boldt-Christmas has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update copyright year > > test/hotspot/jtreg/serviceability/sa/TestObjectMonitorIterate.java line 1: > >> 1: /* > > This test still barely actually tests the iterator code. Agreed. Did not want to spend time learning the SA, but maybe I should. So I can write a proper test. It does however seem very hard to write a proper test. Also not sure how to make it non-flaky, maybe start a lingering App, let it reach steady state, get the monitor list from some other API (maybe there is a jcmd, or we have to use the whitebox API from the actual process and print it somewhere, and hope the state of which monitors exists does not change), then attach the SA and run and compare the lists. It seems hard to me to ensure that the SA agent prints all monitors in the `_in_use_list` with a test. Or you would have to maintain a list of monitors for each locking mode and the lingering app. - PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1335687908
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Mon, 25 Sep 2023 02:08:36 GMT, David Holmes wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update copyright year
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java
> line 49:
>
>> 47: Type monitorListType = db.lookupType("MonitorList");
>> 48: Address monitorListAddr =
>> objectSynchronizerType.getField("_in_use_list").getStaticFieldAddress();
>> 49: inUseListHead =
>> monitorListType.getAddressField("_head").getAddress(monitorListAddr);
>
> So is `inUseListHead` effectively a "pointer" to the `_head` field of the
> `_in_use_list` `MonitorList`, which is dereferenced when we read it? Else I'm
> not seeing how we iterate the current set of in-use monitors.
It is not. It is the value of `_head` when ObjectSynchronizer is initialised.
My understanding of the serviceability agent is very limited, but from what I
understood the JVM does not run when we are attached. So no code should add to
the list.
If that was a requirement then both this and the previous implementation are
completely broken.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1335673227
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:21:25 GMT, Axel Boldt-Christmas
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
>> * Passed tier 1-5
>> * Passed GHA
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update copyright year
Seems to fix the two issues pointed out in JBS but I have two comments below
Thanks.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java
line 49:
> 47: Type monitorListType = db.lookupType("MonitorList");
> 48: Address monitorListAddr =
> objectSynchronizerType.getField("_in_use_list").getStaticFieldAddress();
> 49: inUseListHead =
> monitorListType.getAddressField("_head").getAddress(monitorListAddr);
So is `inUseListHead` effectively a "pointer" to the `_head` field of the
`_in_use_list` `MonitorList`, which is dereferenced when we read it? Else I'm
not seeing how we iterate the current set of in-use monitors.
test/hotspot/jtreg/serviceability/sa/TestObjectMonitorIterate.java line 1:
> 1: /*
This test still barely actually tests the iterator code.
-
PR Review: https://git.openjdk.org/jdk/pull/15782#pullrequestreview-1641218470
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1335312311
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1335344790
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:21:13 GMT, Axel Boldt-Christmas
wrote:
>> `inUseList` will end up with the same value as `inUseListHead`. The reason
>> the old code worked is because `getAddressField` does not type check and
>> `reinterpret_cast(::_in_use_list) ==
>> reinterpret_cast(::_in_use_list._head)`
>>
>> Effectively I changed this to load it correctly (regardless of what
>> `offset_of(MonitorList, _head)` ends up being) and name the variables more
>> appropriately.
>>
>> C++ interpretation of what the java change does:
>> ```C++
>> // Old code
>> // Type type = db.lookupType("ObjectSynchronizer");
>> // inUseList = type.getAddressField("_in_use_list").getValue();
>> address inUseList =
>> *(reinterpret_cast(::_in_use_list));
>>
>> // New code
>> // Type objectSynchronizerType = db.lookupType("ObjectSynchronizer");
>> // Type monitorListType = db.lookupType("MonitorList");
>> // Address monitorListAddr =
>> objectSynchronizerType.getField("_in_use_list").getStaticFieldAddress();
>> // inUseListHead =
>> monitorListType.getAddressField("_head").getAddress(monitorListAddr);
>> address monitorListAddr =
>> reinterpret_cast(::_in_use_list);
>> address inUseList = *(reinterpret_cast(monitorListAddr +
>> offset_of(MonitorList, _head)));
>
> Just to clarify what might cause confusion (at least it is what confused me
> at first when I read this code) is that `getAddress()`/
> `getAddressField(...).getValue()` does not return the address of the field.
> It returns the value of the field (loaded and) interpreted as an address.
I see now. _in_use_list is a MonitorList (not a pointer to one) and the first
field of a MonitorList is the _head field. So that means the address of the
_in_use_list field is also the address of the _head field.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r190292
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:21:25 GMT, Axel Boldt-Christmas 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 >> * Passed tier 1-5 >> * Passed GHA > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15782#pullrequestreview-1638371175
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:11:42 GMT, Axel Boldt-Christmas
wrote:
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java
>> line 86:
>>
>>> 84:
>>> 85: ObjectMonitorIterator() {
>>> 86: mon = new ObjectMonitor(inUseList);
>>
>> How did this ever work? `inUseList` is an address that points to a
>> `MonitorList`, not an `ObjectMonitor`. Your new code has it right, but it
>> seems that this old code would have failed either during construction, or
>> during the first `mon.nextOM()` call.
>
> `inUseList` will end up with the same value as `inUseListHead`. The reason
> the old code worked is because `getAddressField` does not type check and
> `reinterpret_cast(::_in_use_list) ==
> reinterpret_cast(::_in_use_list._head)`
>
> Effectively I changed this to load it correctly (regardless of what
> `offset_of(MonitorList, _head)` ends up being) and name the variables more
> appropriately.
>
> C++ interpretation of what the java change does:
> ```C++
> // Old code
> // Type type = db.lookupType("ObjectSynchronizer");
> // inUseList = type.getAddressField("_in_use_list").getValue();
> address inUseList =
> *(reinterpret_cast(::_in_use_list));
>
> // New code
> // Type objectSynchronizerType = db.lookupType("ObjectSynchronizer");
> // Type monitorListType = db.lookupType("MonitorList");
> // Address monitorListAddr =
> objectSynchronizerType.getField("_in_use_list").getStaticFieldAddress();
> // inUseListHead =
> monitorListType.getAddressField("_head").getAddress(monitorListAddr);
> address monitorListAddr =
> reinterpret_cast(::_in_use_list);
> address inUseList = *(reinterpret_cast(monitorListAddr +
> offset_of(MonitorList, _head)));
Just to clarify what might cause confusion (at least it is what confused me at
first when I read this code) is that `getAddress()`/
`getAddressField(...).getValue()` does not return the address of the field. It
returns the value of the field (loaded and) interpreted as an address.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1332528142
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v5]
On Tue, 19 Sep 2023 22:19:06 GMT, Chris Plummer wrote: >> Axel Boldt-Christmas has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Removed unnecessary comment > > test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java > line 41: > >> 39: * @build jdk.test.whitebox.WhiteBox >> 40: * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar >> loadclass.jar JCmdTestLingeredApp >> 41: * jdk.test.lib.apps.LingeredApp >> jdk.test.lib.apps.LingeredApp$1 jdk.test.lib.apps.LingeredApp$SteadyStateLock > > Copyright needs updating in this file. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1332523767
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
> 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 > * Passed tier 1-5 > * Passed GHA Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision: Update copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/15782/files - new: https://git.openjdk.org/jdk/pull/15782/files/faa579a9..e77c056c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15782=05 - incr: https://webrevs.openjdk.org/?repo=jdk=15782=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15782.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15782/head:pull/15782 PR: https://git.openjdk.org/jdk/pull/15782
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v5]
On Wed, 20 Sep 2023 22:37:59 GMT, Chris Plummer wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed unnecessary comment
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java
> line 86:
>
>> 84:
>> 85: ObjectMonitorIterator() {
>> 86: mon = new ObjectMonitor(inUseList);
>
> How did this ever work? `inUseList` is an address that points to a
> `MonitorList`, not an `ObjectMonitor`. Your new code has it right, but it
> seems that this old code would have failed either during construction, or
> during the first `mon.nextOM()` call.
`inUseList` will end up with the same value as `inUseListHead`. The reason the
old code worked is because `getAddressField` does not type check and
`reinterpret_cast(::_in_use_list) ==
reinterpret_cast(::_in_use_list._head)`
Effectively I changed this to load it correctly (regardless of what
`offset_of(MonitorList, _head)` ends up being) and name the variables more
appropriately.
C++ interpretation of what the java change does:
```C++
// Old code
// Type type = db.lookupType("ObjectSynchronizer");
// inUseList = type.getAddressField("_in_use_list").getValue();
address inUseList =
*(reinterpret_cast(::_in_use_list));
// New code
// Type objectSynchronizerType = db.lookupType("ObjectSynchronizer");
// Type monitorListType = db.lookupType("MonitorList");
// Address monitorListAddr =
objectSynchronizerType.getField("_in_use_list").getStaticFieldAddress();
// inUseListHead =
monitorListType.getAddressField("_head").getAddress(monitorListAddr);
address monitorListAddr =
reinterpret_cast(::_in_use_list);
address inUseList = *(reinterpret_cast(monitorListAddr +
offset_of(MonitorList, _head)));
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1332516949
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v5]
On Tue, 19 Sep 2023 09:25:33 GMT, Axel Boldt-Christmas
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
>> * Passed tier 1-5
>> * Passed GHA
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Removed unnecessary comment
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java
line 86:
> 84:
> 85: ObjectMonitorIterator() {
> 86: mon = new ObjectMonitor(inUseList);
How did this ever work? `inUseList` is an address that points to a
`MonitorList`, not an `ObjectMonitor`. Your new code has it right, but it seems
that this old code would have failed either during construction, or during the
first `mon.nextOM()` call.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java
line 41:
> 39: * @build jdk.test.whitebox.WhiteBox
> 40: * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar loadclass.jar
> JCmdTestLingeredApp
> 41: * jdk.test.lib.apps.LingeredApp
> jdk.test.lib.apps.LingeredApp$1 jdk.test.lib.apps.LingeredApp$SteadyStateLock
Copyright needs updating in this file.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1332246787
PR Review Comment: https://git.openjdk.org/jdk/pull/15782#discussion_r1330761695
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v3]
On Tue, 19 Sep 2023 07:28:52 GMT, Axel Boldt-Christmas wrote: > All the failing CDS tests pass now. Will rerun a full test suite to be sure. > (There were two CDS test files that needed to be updated) [JDK-8293507](https://bugs.openjdk.org/browse/JDK-8293507) is suppose to address the CDS issue you are seeing, but for now your fix is appropriate. - PR Comment: https://git.openjdk.org/jdk/pull/15782#issuecomment-1726604921
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v5]
> 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: Removed unnecessary comment - Changes: - all: https://git.openjdk.org/jdk/pull/15782/files - new: https://git.openjdk.org/jdk/pull/15782/files/48a6ba80..faa579a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15782=04 - incr: https://webrevs.openjdk.org/?repo=jdk=15782=03-04 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15782.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15782/head:pull/15782 PR: https://git.openjdk.org/jdk/pull/15782
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v3]
On Mon, 18 Sep 2023 12:11:24 GMT, Axel Boldt-Christmas 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 All the failing CDS tests pass now. Will rerun a full test suite to be sure. (There were two CDS test files that needed to be updated) `objectMonitorIterator()` now returns an empty iterator even if `initialize` fails. - PR Comment: https://git.openjdk.org/jdk/pull/15782#issuecomment-1724971537
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v4]
> 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 four additional commits since the last revision: - Always return a ObjectMonitorIterator - Fix CDS tests - Revert "Avoid changing the LingeredApp class hierarchy for CDS tests ergonomics" This reverts commit 9376d8fee715d1049b5c5f8e52605f3f4d083601. - Revert "Copyright changes too" This reverts commit 973646ed3868bdbc601075f50f9193b7e2deafbe. - Changes: - all: https://git.openjdk.org/jdk/pull/15782/files - new: https://git.openjdk.org/jdk/pull/15782/files/973646ed..48a6ba80 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15782=03 - incr: https://webrevs.openjdk.org/?repo=jdk=15782=02-03 Stats: 20 lines in 6 files changed: 2 ins; 11 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/15782.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15782/head:pull/15782 PR: https://git.openjdk.org/jdk/pull/15782
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists
On Mon, 18 Sep 2023 18:49:43 GMT, Chris Plummer wrote: > > CDS tests are not happy with changing the class hierarchy of the > > LingeredApp. Unless it is easily solved for the CDS test I will revert > > those changes and have the 'TestObjectMonitorIterate' just do a less > > precise check of a lock on a Object. In the current JDK for LM_LEGACY and > > LM_LIGHTWEIGHT the `steadyStateObj` is the only Object object with a > > monitor when the SA agent queries. > > I preferred your original fix to the test. Can you explain what the CDS issue > test was? The tests shouldn't be written in such a way that makes it hard to > extend functionality in LingeredApp. I had the same issue when I fixed the InMemoryCompiler test lib we have. That time I had to update quite a few CDS files to make it work (so in the end the fix was rewritten to no change the class hierarchy). Is seems like the Strings for the `LingeredApp` CDS tests are all in one shared file. So I will update the relevant CDS file and test that it works. As revert the last changes. I agree that it is much better with a named class. There are quite some parts of the test libraries that the CDS tests have dependencies on. - PR Comment: https://git.openjdk.org/jdk/pull/15782#issuecomment-1724955489
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v3]
On Mon, 18 Sep 2023 18:46:19 GMT, Chris Plummer 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
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v3]
On Mon, 18 Sep 2023 12:11:24 GMT, Axel Boldt-Christmas
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
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists
On Mon, 18 Sep 2023 11:55:24 GMT, Axel Boldt-Christmas wrote: > CDS tests are not happy with changing the class hierarchy of the LingeredApp. > Unless it is easily solved for the CDS test I will revert those changes and > have the 'TestObjectMonitorIterate' just do a less precise check of a lock on > a Object. In the current JDK for LM_LEGACY and LM_LIGHTWEIGHT the > `steadyStateObj` is the only Object object with a monitor when the SA agent > queries. I preferred your original fix to the test. Can you explain what the CDS issue test was? The tests shouldn't be written in such a way that makes it hard to extend functionality in LingeredApp. - PR Comment: https://git.openjdk.org/jdk/pull/15782#issuecomment-1724188868
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v3]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/15782/files - new: https://git.openjdk.org/jdk/pull/15782/files/9376d8fe..973646ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15782=02 - incr: https://webrevs.openjdk.org/?repo=jdk=15782=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15782.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15782/head:pull/15782 PR: https://git.openjdk.org/jdk/pull/15782
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v2]
> 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: Avoid changing the LingeredApp class hierarchy for CDS tests ergonomics - Changes: - all: https://git.openjdk.org/jdk/pull/15782/files - new: https://git.openjdk.org/jdk/pull/15782/files/1c29f066..9376d8fe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15782=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15782=00-01 Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15782.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15782/head:pull/15782 PR: https://git.openjdk.org/jdk/pull/15782
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists
On Mon, 18 Sep 2023 09:54:18 GMT, Axel Boldt-Christmas 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 CDS tests are not happy with changing the class hierarchy of the LingeredApp. Unless it is easily solved for the CDS test I will revert those changes and have the 'TestObjectMonitorIterate' just do a less precise check of a lock on a Object. In the current JDK for LM_LEGACY and LM_LIGHTWEIGHT the `steadyStateObj` is the only object with a monitor when the SA agent queries. - PR Comment: https://git.openjdk.org/jdk/pull/15782#issuecomment-1723259794
