Re: RFR: 8341246: Test com/sun/tools/attach/PermissionTest.java fails access denied after JDK-8327114 [v2]
On Tue, 1 Oct 2024 15:38:56 GMT, SendaoYan wrote: >> Hi all, >> Test `com/sun/tools/attach/PermissionTest.java` fails access denied after >> [JDK-8327114](https://bugs.openjdk.org/browse/JDK-8327114). This testcase >> need the `readlink` permission of file `/proc/self/ns/mnt` after >> [JDK-8327114](https://bugs.openjdk.org/browse/JDK-8327114). >> So this PR add `readlink` permission to make this test work normally. >> Before this PR, test run failed, after this PR, test run success. Test fix >> only, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > merge two comments enclosed with /* ... */ on two lines @sendaoYan - This failure is quite noisy in the Oracle CI. We see many failures in Tier[567]. When will you be integrating your fix? - PR Comment: https://git.openjdk.org/jdk/pull/21269#issuecomment-2388992950
Re: RFR: 8339289: Parameter size mismatch between client and VM sides of the Attach API - Windows
On Fri, 30 Aug 2024 00:07:50 GMT, Alex Menkov wrote: > The fix improves Attch API protocol and implements updated protocol on > windows; shared code is ready to implement updated protocol support on other > platforms. > More detailed explanations on the 1st comment. > > Testing: tier1,tier2,tier3,tier4,hs-tier5-svc > manually tested backward compatibility (old tools can attach to current > VMs, current tools can attach to older VMs) on Windows with jdk21u and jdk8u. Thanks for putting the details in a separate comment. That will make the emails that get sent for this PR much shorter. You use the word `symbols`, e.g., `each argument is resticted by 1024 symbols`. Do you really mean characters or bytes? Also thanks for including all the testing info! - PR Comment: https://git.openjdk.org/jdk/pull/20782#issuecomment-2322099619 PR Comment: https://git.openjdk.org/jdk/pull/20782#issuecomment-2322100213
Re: RFR: 8337563: NMT: rename MEMFLAGS to MemFlag
On Wed, 7 Aug 2024 17:13:06 GMT, Gerard Ziemski wrote: > Please review this cleanup, where we rename `MEMFLAGS` to `MemType`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemType` is much more suitable name. > > There is a bunch of other related cleanup that we can do, but I will leave > for follow up issues such as [NMT: rename NMTUtil::flag to > NMTUtil::type](https://bugs.openjdk.org/browse/JDK-8337836) I think it must have `NMT` somewhere in the name. `NMTGroup` sounds like there could more than one value set at one time, but that might be just me. `NMTCat` (and `NMTGroup`) lose the idea that `Type` should be in there somewhere. For ease of typing, I'm not fond of `NMT::MemType` but it has the appeal of being in a namespace... So I'm going with: NMT::MemType - 3 points NMT_MemType/NMTMemType(prefer for ease of typing) - 2 points and I'm skipping a 1 point assignment. - PR Comment: https://git.openjdk.org/jdk/pull/20497#issuecomment-2315871575
Re: RFR: 8204681: Option to include timestamp in hprof filename
On Thu, 22 Aug 2024 10:59:58 GMT, Kevin Walls wrote: > which says "%t => -MM-DD_HH-MM-SS" so maybe we could follow that pattern Another vote for the above time stamp pattern. - PR Comment: https://git.openjdk.org/jdk/pull/20568#issuecomment-2305273185
Re: [jdk23] RFR: 8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
On Tue, 20 Aug 2024 12:47:59 GMT, Stefan Karlsson wrote: > Hi all, > > This pull request contains a backport of commit > [9775d571](https://github.com/openjdk/jdk/commit/9775d57168695dc0d758e017fe5069d93d593f3e) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Stefan Karlsson on 20 Aug 2024 > and was reviewed by Leonid Mesnik, Daniel D. Daugherty and David Holmes. > > Thanks! Thumbs up. Re-reviewed via GitHub and compared the JDK24 patch file with the JDK23 patch file. - PR Review: https://git.openjdk.org/jdk/pull/20643#pullrequestreview-2248424096 PR Comment: https://git.openjdk.org/jdk/pull/20643#issuecomment-2299210408
Re: RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods [v2]
On Tue, 20 Aug 2024 06:13:18 GMT, Stefan Karlsson wrote: >> So in this test case: >> `-Xlog:gc,gc+init` >> you have it so that `true` is returned. >> >> With this line of code: >> ``` >>const bool is_gc_exact_match = ts->contains(LogTag::_gc) && ts->ntags() >> == 1; >> >> I would think that this part is `false`: `ts->ntags() == 1` >> for the `-Xlog:gc,gc+init` test case so we'll be returning `false`. >> >> What am I missing? > > The important part is that `ts` contains one single tag set and that > `gc,gc+init` is a string that specifies the two tag sets `gc` and `gc+init`. > Also, note that `-Xlog:gc,gc+init` and `-Xlog:gc -Xlog:gc+init` is > effectively the same. > > It is also important to understand that the for loop iterates over all tag > sets that have been used in the HotSpot code. It is not iterating over the > tag sets specified by -Xlog lines. OK, I get it now. The loop cycles thru the tag set and finds one that is a singleton and an exact match, i.e., `-Xlog:gc`. With `-Xlog:gc+init`, the `ts->ntags() == 1` would be `false` because that one is not an exact match. I misunderstood the algorithm a bit. Sorry for the noise. - PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1723555244
Re: RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods [v2]
On Mon, 19 Aug 2024 19:56:26 GMT, Stefan Karlsson wrote: >> The `ClassLoadingMXBean` and `MemoryMXBean` APIs have `setVerbose` methods >> to control verbose mode and `isVerbose` methods to query it. Some JCK tests >> expect `setVerbose(false)` to disable verbose mode and, subsequently, >> `isVerbose()` to return false. However, if logging to a file is enabled by >> using -Xlog on the java launcher command line then `isVerbose()` returns >> true even after calling `setVerbose(false)`. >> >> The proposed patch solves this by introducing two changes: >> >> 1) The previous implementation used the `log_is_enabled` functionality to >> check if logging was enabled for the given tag set. This returns true if >> logging has been turned on for *any* output. The patch changes this so that >> `isVerbose` only checks what has been configured for stdout, which is the >> output that `setVerbose` configures. >> >> 2) The previous implementation of `setVerbose` turned on `class+load*` >> (notice the star) but then `isVerbose` only checked `class+load` (without >> the star). The patch changes this so that the `isVerbose` in-effect checks >> `class+load*`. (The `gc` part of the patch did not have this problem) >> >> The main focus on this patch is to fix the JCK failure, with an >> implementation that follows the API documentation. While looking at this >> area of the code it is clear that there are other problems that we might >> want to addressed in the future, but we're intentionally keeping this patch >> limited in scope so that it can be backported to JDK 23. >> >> A CSR for this change has been created. >> >> Testing: >> * The newly implemented tests >> * The failing JCK tests with the corresponding -Xlog lines >> * Tier1-7 (running) >> >> The patch is co-authored by me and David Holmes > > Stefan Karlsson has updated the pull request incrementally with two > additional commits since the last revision: > > - Test -Xlog:gc,gc+init > - Review comments Marked as reviewed by dcubed (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20628#pullrequestreview-2246574938
Re: RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods [v2]
On Mon, 19 Aug 2024 19:56:26 GMT, Stefan Karlsson wrote: >> The `ClassLoadingMXBean` and `MemoryMXBean` APIs have `setVerbose` methods >> to control verbose mode and `isVerbose` methods to query it. Some JCK tests >> expect `setVerbose(false)` to disable verbose mode and, subsequently, >> `isVerbose()` to return false. However, if logging to a file is enabled by >> using -Xlog on the java launcher command line then `isVerbose()` returns >> true even after calling `setVerbose(false)`. >> >> The proposed patch solves this by introducing two changes: >> >> 1) The previous implementation used the `log_is_enabled` functionality to >> check if logging was enabled for the given tag set. This returns true if >> logging has been turned on for *any* output. The patch changes this so that >> `isVerbose` only checks what has been configured for stdout, which is the >> output that `setVerbose` configures. >> >> 2) The previous implementation of `setVerbose` turned on `class+load*` >> (notice the star) but then `isVerbose` only checked `class+load` (without >> the star). The patch changes this so that the `isVerbose` in-effect checks >> `class+load*`. (The `gc` part of the patch did not have this problem) >> >> The main focus on this patch is to fix the JCK failure, with an >> implementation that follows the API documentation. While looking at this >> area of the code it is clear that there are other problems that we might >> want to addressed in the future, but we're intentionally keeping this patch >> limited in scope so that it can be backported to JDK 23. >> >> A CSR for this change has been created. >> >> Testing: >> * The newly implemented tests >> * The failing JCK tests with the corresponding -Xlog lines >> * Tier1-7 (running) >> >> The patch is co-authored by me and David Holmes > > Stefan Karlsson has updated the pull request incrementally with two > additional commits since the last revision: > > - Test -Xlog:gc,gc+init > - Review comments Marked as reviewed by dcubed (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20628#pullrequestreview-2246622538
Re: RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods [v2]
On Mon, 19 Aug 2024 19:52:02 GMT, Stefan Karlsson wrote: >> Update: Looks like the CSR says that's exactly what we want. > > Wait. This is not what the implementation does. If you specify -Xlog:gc and > -Xlog:gc+init (using a real tag instead of foo), `get_verbose` will return > true. It only cares about the tag set that is exactly 'gc' and ignores the > 'gc+init' tag set. > > I'm adding a test line to show that. So in this test case: `-Xlog:gc,gc+init` you have it so that `true` is returned. With this line of code: ``` const bool is_gc_exact_match = ts->contains(LogTag::_gc) && ts->ntags() == 1; I would thing that this part is `false`: `ts->ntags() == 1` for the `-Xlog:gc,gc+init` test case so we'll be returning `false`. What am I missing? >> Update: The CSR makes it clear that it is `-Xlog:class+load*` so all have to >> be set. > > This is one of those places where we changed the behavior. Previously, > 'isVerbose` would return true here. Yup. We're in agreement here. - PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722442004 PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722421913
Re: RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
On Mon, 19 Aug 2024 16:30:09 GMT, Daniel D. Daugherty wrote: >> The `ClassLoadingMXBean` and `MemoryMXBean` APIs have `setVerbose` methods >> to control verbose mode and `isVerbose` methods to query it. Some JCK tests >> expect `setVerbose(false)` to disable verbose mode and, subsequently, >> `isVerbose()` to return false. However, if logging to a file is enabled by >> using -Xlog on the java launcher command line then `isVerbose()` returns >> true even after calling `setVerbose(false)`. >> >> The proposed patch solves this by introducing two changes: >> >> 1) The previous implementation used the `log_is_enabled` functionality to >> check if logging was enabled for the given tag set. This returns true if >> logging has been turned on for *any* output. The patch changes this so that >> `isVerbose` only checks what has been configured for stdout, which is the >> output that `setVerbose` configures. >> >> 2) The previous implementation of `setVerbose` turned on `class+load*` >> (notice the star) but then `isVerbose` only checked `class+load` (without >> the star). The patch changes this so that the `isVerbose` in-effect checks >> `class+load*`. (The `gc` part of the patch did not have this problem) >> >> The main focus on this patch is to fix the JCK failure, with an >> implementation that follows the API documentation. While looking at this >> area of the code it is clear that there are other problems that we might >> want to addressed in the future, but we're intentionally keeping this patch >> limited in scope so that it can be backported to JDK 23. >> >> A CSR for this change has been created. >> >> Testing: >> * The newly implemented tests >> * The failing JCK tests with the corresponding -Xlog lines >> * Tier1-7 (running) >> >> The patch is co-authored by me and David Holmes > > src/hotspot/share/services/memoryService.cpp line 218: > >> 216: >> 217: return false; >> 218: } > > If we have `-Xlog:gc` and `-Xlog:gc+foo` set I think this version of > `MemoryService::get_verbose()` will return `false`. Is that really > what we want? Update: Looks like the CSR says that's exactly what we want. > test/jdk/java/lang/management/ClassLoadingMXBean/TestVerboseClassLoading.java > line 36: > >> 34: * @run main/othervm -Xlog:class+load=trace TestVerboseClassLoading false >> 35: * @run main/othervm -Xlog:class+load=debug TestVerboseClassLoading false >> 36: * @run main/othervm -Xlog:class+load=info TestVerboseClassLoading false > > Hmm... I was expecting these to be `true`. What am I missing? Update: The CSR makes it clear that it is `-Xlog:class+load*` so all have to be set. - PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722075468 PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722069521
Re: RFR: 8338139: {ClassLoading, Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
On Mon, 19 Aug 2024 15:39:02 GMT, Stefan Karlsson wrote: > The `ClassLoadingMXBean` and `MemoryMXBean` APIs have `setVerbose` methods to > control verbose mode and `isVerbose` methods to query it. Some JCK tests > expect `setVerbose(false)` to disable verbose mode and, subsequently, > `isVerbose()` to return false. However, if logging to a file is enabled by > using -Xlog on the java launcher command line then `isVerbose()` returns true > even after calling `setVerbose(false)`. > > The proposed patch solves this by introducing two changes: > > 1) The previous implementation used the `log_is_enabled` functionality to > check if logging was enabled for the given tag set. This returns true if > logging has been turned on for *any* output. The patch changes this so that > `isVerbose` only checks what has been configured for stdout, which is the > output that `setVerbose` configures. > > 2) The previous implementation of `setVerbose` turned on `class+load*` > (notice the star) but then `isVerbose` only checked `class+load` (without the > star). The patch changes this so that the `isVerbose` in-effect checks > `class+load*`. (The `gc` part of the patch did not have this problem) > > The main focus on this patch is to fix the JCK failure, with an > implementation that follows the API documentation. While looking at this area > of the code it is clear that there are other problems that we might want to > addressed in the future, but we're intentionally keeping this patch limited > in scope so that it can be backported to JDK 23. > > A CSR for this change has been created. > > Testing: > * The newly implemented tests > * The failing JCK tests with the corresponding -Xlog lines > * Tier1-7 (running) > > The patch is co-authored by me and David Holmes Thumbs up. src/hotspot/share/services/memoryService.cpp line 218: > 216: > 217: return false; > 218: } If we have `-Xlog:gc` and `-Xlog:gc+foo` set I think this version of `MemoryService::get_verbose()` will return `false`. Is that really what we want? test/jdk/java/lang/management/ClassLoadingMXBean/TestVerboseClassLoading.java line 36: > 34: * @run main/othervm -Xlog:class+load=trace TestVerboseClassLoading false > 35: * @run main/othervm -Xlog:class+load=debug TestVerboseClassLoading false > 36: * @run main/othervm -Xlog:class+load=info TestVerboseClassLoading false Hmm... I was expecting these to be `true`. What am I missing? test/jdk/java/lang/management/ClassLoadingMXBean/TestVerboseClassLoading.java line 56: > 54: */ > 55: > 56: import java.lang.management.*; I thought we tried to avoid wild-card imports. test/jdk/java/lang/management/MemoryMXBean/TestVerboseMemory.java line 50: > 48: */ > 49: > 50: import java.lang.management.*; I thought we tried to avoid wild-card imports. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20628#pullrequestreview-2245990507 PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722060516 PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722064641 PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722072308 PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722072710
Re: RFR: 8338469: com/sun/jdi/DataDumpTest.java failed with Not a debuggee, or not listening for debugger to attach [v2]
On Fri, 16 Aug 2024 16:31:01 GMT, Chris Plummer wrote: >> There are issues with the test attaching to the debuggee too soon, and the >> debug agent isn't ready. yet. This test is based on ProcessAttachTest, which >> does not have this issue. I eventually realized the reason why is because >> ProcessAttachTest has this little snippet of code, which I had removed from >> DataDumpTest: >> >> // Wait for the process to start >> InputStream is = p.getInputStream(); >> is.read(); >> >> This is waiting for the start of the debug agent's "Listening..." message. >> I've re-added this to DataDumpTest, and it seems to fix the issue. Tested by >> running 20 times with `-Xcomp` on all supported platforms with no failures. >> It used to fail about 2/3 of the time. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo in output Thumbs up. Thanks for fixing the typo. This is a trivial fix. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20604#pullrequestreview-2243090019
Re: RFR: 8338469: com/sun/jdi/DataDumpTest.java failed with Not a debuggee, or not listening for debugger to attach
On Thu, 15 Aug 2024 21:12:20 GMT, Chris Plummer wrote:
> There are issues with the test attaching to the debuggee too soon, and the
> debug agent isn't ready. yet. This test is based on ProcessAttachTest, which
> does not have this issue. I eventually realized the reason why is because
> ProcessAttachTest has this little snippet of code, which I had removed from
> DataDumpTest:
>
> // Wait for the process to start
> InputStream is = p.getInputStream();
> is.read();
>
> This is waiting for the start of the debug agent's "Listening..." message.
> I've re-added this to DataDumpTest, and it seems to fix the issue. Tested by
> running 20 times with `-Xcomp` on all supported platforms with no failures.
> It used to fail about 2/3 of the time.
Thumbs up. Thanks for including `firstChar` in the debuggee output.
I think this is a trivial fix.
test/jdk/com/sun/jdi/DataDumpTest.java line 100:
> 98: out.waitFor(); // Wait for the debuggee to exit
> 99:
> 100: System.out.println("Deuggee output:");
nit typo: s/Deuggee/Debuggee/
Not your typo, but could you fix it while you are there?
-
Marked as reviewed by dcubed (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20604#pullrequestreview-2243069811
PR Review Comment: https://git.openjdk.org/jdk/pull/20604#discussion_r1720054663
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 17:24:19 GMT, Daniel D. Daugherty
wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove the last OMWorld references
>> - Rename omworldtable_work to object_monitor_table_work
>
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 341:
>
>> 339:
>> 340: ObjectMonitor*
>> LightweightSynchronizer::get_or_insert_monitor_from_table(oop object,
>> JavaThread* current, bool* inserted) {
>> 341: assert(LockingMode == LM_LIGHTWEIGHT, "must be");
>
> Do you want to assert: `inserted != nullptr`?
What was the resolution? I don't see a reply or a change here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1717543005
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v19]
On Wed, 14 Aug 2024 09:24:34 GMT, Axel Boldt-Christmas wrote: >> When inflating a monitor the `ObjectMonitor*` is written directly over the >> `markWord` and any overwritten data is displaced into a displaced >> `markWord`. This is problematic for concurrent GCs which needs extra care or >> looser semantics to use this displaced data. In Lilliput this data also >> contains the klass forcing this to be something that the GC has to take into >> account everywhere. >> >> This patch introduces an alternative solution where locking only uses the >> lock bits of the `markWord` and inflation does not override and displace the >> `markWord`. This is done by keeping associations between objects and >> `ObjectMonitor*` in an external hash table. Different caching techniques are >> used to speedup lookups from compiled code. >> >> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is >> only supported in combination with the LM_LIGHTWEIGHT locking mode (the >> default). >> >> This patch has been evaluated to be performance neutral when >> `UseObjectMonitorTable` is turned off (the default). >> >> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` >> and `UseObjectMonitorTable` works. >> >> # Cleanups >> >> Cleaned up displaced header usage for: >> * BasicLock >> * Contains some Zero changes >> * Renames one exported JVMCI field >> * ObjectMonitor >> * Updates comments and tests consistencies >> >> # Refactoring >> >> `ObjectMonitor::enter` has been refactored an a >> `ObjectMonitorContentionMark` witness object has been introduced to the >> signatures. Which signals that the contentions reference counter is being >> held. More details are given below in the section about deflation. >> >> The initial purpose of this was to allow `UseObjectMonitorTable` to interact >> more seamlessly with the `ObjectMonitor::enter` code. >> >> _There is even more `ObjectMonitor` refactoring which can be done here to >> create a more understandable and enforceable API. There are a handful of >> invariants / assumptions which are not always explicitly asserted which >> could be trivially abstracted and verified by the type system by using >> similar witness objects._ >> >> # LightweightSynchronizer >> >> Working on adapting and incorporating the following section as a comment in >> the source code >> >> ## Fast Locking >> >> CAS on locking bits in markWord. >> 0b00 (Fast Locked) <--> 0b01 (Unlocked) >> >> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to >> avoid inflating by spinning a bit. >> >> If 0b10 (Inflated) is observed or there is to... > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Use jdk.test.lib.Utils.getRandomInstance() Just a couple of comments this time. I originally reviewed v10 and this time I reviewed v10..v18. src/hotspot/share/runtime/synchronizer.hpp line 126: > 124: > 125: static bool quick_notify(oopDesc* obj, JavaThread* current, bool All); > 126: Why add the extra blank line? - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20067#pullrequestreview-2239178344 PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1717549253
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
On Wed, 14 Aug 2024 09:35:10 GMT, Axel Boldt-Christmas
wrote:
>> src/hotspot/share/runtime/vframe.cpp line 252:
>>
>>> 250: if (mark.has_monitor()) {
>>> 251: ObjectMonitor* mon =
>>> ObjectSynchronizer::read_monitor(current, monitor->owner(), mark);
>>> 252: if (// if the monitor is null we must be in the process of
>>> locking
>>
>> nit - please add a space after `(`
>
> Should I align the rest of the lines? Adding the extra space here looks
> strange to me. But the inlined comments looks strange as well. This is all
> pre-existing code that just moved around a bit.
I'm just not fond of a `// comment` butting right against code.
Your call on whether to leave it alone or how to reformat it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1717114224
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
On Tue, 13 Aug 2024 16:30:12 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Whitespace and nits
Just for clarity, I think this is the one real bug that I found:
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 126:
> 124:
> 125: static void dec_items_count() {
> 126: Atomic::inc(&_items_count);
Shouldn't this be `Atomic::dec`?
-
PR Comment: https://git.openjdk.org/jdk/pull/20067#issuecomment-2287237680
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 14:56:32 GMT, Coleen Phillimore wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove the last OMWorld references
>> - Rename omworldtable_work to object_monitor_table_work
>
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 172:
>
>> 170: static void create() {
>> 171: _table = new ConcurrentTable(initial_log_size(),
>> 172: max_log_size(),
>
> nit, can you line up these parameters?
Or put them all on L171 if that doesn't make it too long...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715648059
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 14:52:03 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Remove the last OMWorld references
> - Rename omworldtable_work to object_monitor_table_work
src/hotspot/share/runtime/basicLock.inline.hpp line 45:
> 43: return reinterpret_cast(get_metadata());
> 44: #else
> 45: // Other platforms does not make use of the cache yet,
nit typo: s/does not/do not/
src/hotspot/share/runtime/basicLock.inline.hpp line 54:
> 52: inline void BasicLock::clear_object_monitor_cache() {
> 53: assert(UseObjectMonitorTable, "must be");
> 54: set_metadata(0);
Should this be a literal `0` or should it be `nullptr`?
Update: The metadata field is of type `unintptr_t`. Got it.
src/hotspot/share/runtime/deoptimization.cpp line 1650:
> 1648: mon_info->lock()->set_bad_metadata_deopt();
> 1649: }
> 1650: #endif
I like this!
src/hotspot/share/runtime/globals.hpp line 1964:
> 1962:
> \
> 1963: product(int, LightweightFastLockingSpins, 13, DIAGNOSTIC,
> \
> 1964: "Specifies the number of time lightweight fast locking will "
> \
nit typo: s/number of time/number of times/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 34:
> 32: #include "oops/oop.inline.hpp"
> 33: #include "runtime/atomic.hpp"
> 34: #include "memory/allStatic.hpp"
nit: this include is out of order.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 43:
> 41: #include "runtime/mutexLocker.hpp"
> 42: #include "runtime/objectMonitor.hpp"
> 43: #include "runtime/objectMonitor.inline.hpp"
Shouldn't have both includes here.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 81:
> 79: oop _obj;
> 80:
> 81: public:
nit - please indent by one more space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 86:
> 84: uintx get_hash() const {
> 85: uintx hash = _obj->mark().hash();
> 86: assert(hash != 0, "should have a hash");
Hmmm... I can remember seeing hash values of zero in some
of my older legacy inflation stress runs. Is a hash value of zero
not a thing with lightweight locking?
src/hotspot/share/runtime/lightweightSynchronizer.c
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v6]
On Mon, 15 Jul 2024 00:45:10 GMT, Axel Boldt-Christmas
wrote:
>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 401:
>>
>>> 399:
>>> 400: if (inserted) {
>>> 401: // Hopefully the performance counters are allocated on distinct
>>
>> It doesn't look like the counters are on distinct cache lines (see
>> objectMonitor.hpp, lines 212ff). If this is a concern, file a bug to
>> investigate it later? The comment here is a bit misplaced, IMO.
>
> It originates from
> https://github.com/openjdk/jdk/blob/15997bc3dfe9dddf21f20fa189f97291824892de/src/hotspot/share/runtime/synchronizer.cpp#L1543
>
>
> I think we just kept it and did not think more about it.
>
> Not sure what it is referring to. Maybe @dcubed-ojdk knows more, they
> originated from him (9 years old comment).
I don't think we ever got around to experimenting with putting the perf counters
on distinct cache lines. We've always had bigger fish to fry.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715669185
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 16:49:42 GMT, Daniel D. Daugherty
wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove the last OMWorld references
>> - Rename omworldtable_work to object_monitor_table_work
>
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 86:
>
>> 84: uintx get_hash() const {
>> 85: uintx hash = _obj->mark().hash();
>> 86: assert(hash != 0, "should have a hash");
>
> Hmmm... I can remember seeing hash values of zero in some
> of my older legacy inflation stress runs. Is a hash value of zero
> not a thing with lightweight locking?
Update: My memory was wrong. When zero is encountered as a
hash value, it is replaced with `0xBAD`.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 414:
>
>> 412:
>> 413: intptr_t hash = obj->mark().hash();
>> 414: assert(hash != 0, "must be set when claiming the object monitor");
>
> Hmmm... I can remember seeing hash values of zero in some
> of my older legacy inflation stress runs. Is a hash value of zero
> not a thing with lightweight locking?
Update: My memory was wrong. When zero is encountered as a
hash value, it is replaced with `0xBAD`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715952007
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715952460
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v11]
On Mon, 12 Aug 2024 15:58:14 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Missing DEBUG_ONLY
src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 632:
> 630: bool success = false;
> 631: if (LockingMode == LM_LEGACY) {
> 632:// Traditional lightweight locking.
The if-statement is for legacy locking so the comment about lightweight
locking seems wrong.
src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 736:
> 734: bool success = false;
> 735: if (LockingMode == LM_LEGACY) {
> 736: // traditional lightweight locking
The if-statement is for legacy locking so the comment about lightweight
locking seems wrong.
src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 1671:
> 1669: bool success = false;
> 1670: if (LockingMode == LM_LEGACY) {
> 1671: // traditional lightweight locking
The if-statement is for legacy locking so the comment about lightweight
locking seems wrong.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1503:
> 1501:
> 1502: if (mon != nullptr) {
> 1503: assert(mon != nullptr, "must have monitor");
With the new if-statement on L1502, the assert is not needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714439929
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714440641
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714441506
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714448544
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
On Tue, 13 Aug 2024 16:30:12 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Whitespace and nits
I finished my first pass crawl thru on these changes. I need to mull
on these changes a bit before I make another pass. I think there's
only one real bug buried in all of my comments.
src/hotspot/share/runtime/objectMonitor.cpp line 377:
> 375:
> 376: if (cur == current) {
> 377: // TODO-FIXME: check for integer overflow! BUGID 6557169.
Thanks for removing this comment. JDK-6557169 was closed as
"Will Not FIx" in 2017.
src/hotspot/share/runtime/synchronizer.cpp line 970:
> 968: if (value == 0) value = 0xBAD;
> 969: assert(value != markWord::no_hash, "invariant");
> 970: return value;
In the elided part above this line, we have:
if (value == 0) value = 0xBAD;
assert(value != markWord::no_hash, "invariant");
so my memory about zero being returnable as a hash value is wrong.
src/hotspot/share/runtime/synchronizer.cpp line 977:
> 975:
> 976: markWord mark = obj->mark_acquire();
> 977: for(;;) {
nit - please insert space before `(`
src/hotspot/share/runtime/synchronizer.cpp line 997:
> 995: // Since the monitor isn't in the object header, it can simply be
> installed.
> 996: if (UseObjectMonitorTable) {
> 997: return install_hash_code(current, obj);
Perhaps:
if (UseObjectMonitorTable) {
// Since the monitor isn't in the object header, the hash can simply be
// installed in the object header.
return install_hash_code(current, obj);
src/hotspot/share/runtime/synchronizer.cpp line 1271:
> 1269: _no_progress_cnt >= NoAsyncDeflationProgressMax) {
> 1270: double remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
> 1271: size_t new_ceiling = ceiling / remainder + 1;
Why was the `new_ceiling` calculation changed?
I think the `new_ceiling` value is going to lower than the old ceiling value.
src/hotspot/share/runtime/synchronizer.inline.hpp line 83:
> 81:
> 82:
> 83: #endif // SHARE_RUNTIME_SYNCHRONIZER_INLINE_HPP
nit - please delete one of the blank lines.
src/hotspot/share/runtime/vframe.cpp line 252:
> 250: if (mark.has_monitor()) {
> 251: ObjectMonitor* mon =
> ObjectSynchronizer::read_monitor(current, mon
Re: RFR: 8337473: Remove sun/management/jdp tests from ProblemList on Linux-aarch64, MacOSX
On Wed, 31 Jul 2024 10:52:06 GMT, Kevin Walls wrote: > Problemlist update only, should be trivial: > > Remove problemlist entries for sun/management/jdp tests on MacOS and Linux. > > 8241865 was a failure last seen July 2022. > Was related to > 8241530 com/sun/jdi tests fail due to network issues on OSX 10.15 > Originally logged for macOS failures but Linux failures also seen. > > The tests all pass 20 times in a row on the affected platforms. > > They remain problemlisted for JDK-8308807 on AIX. Thumbs up. This is a trivial fix. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20402#pullrequestreview-2211276376
Re: RFR: 8336420: Add JVMTI setfldw001 and setfmodw001 tests to Xcomp problem list
On Tue, 16 Jul 2024 18:56:53 GMT, Chris Plummer wrote: > The following two Xcomp problem list entries were removed: > > vmTestbase/nsk/jvmti/SetFieldAccessWatch/setfldw001/TestDescription.java > 8205957 generic-all > vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001/TestDescription.java > 8205957 linux-x64,windows-x64 > > Each of these tests now has #id0 and #logging versions, and they are failing, > so they need to be problem listed. Thumbs up. This is a trivial fix. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20201#pullrequestreview-2181250160
Re: RFR: 8336284: Test TestClhsdbJstackLock.java/TestJhsdbJstackLock.java fails with -Xcomp after JDK-8335743
On Fri, 12 Jul 2024 03:24:42 GMT, SendaoYan wrote: > Hi all, > Test TestClhsdbJstackLock.java/TestJhsdbJstackLock.java fails with -Xcomp > after [JDK-8335743](https://bugs.openjdk.org/browse/JDK-8335743). I think the > new failures was testcase bug. > > https://github.com/openjdk/jdk/blob/627a32d6722c92f814c1ddd1c2fdf9a3b28cd655/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaVFrame.java#L112 > Locals are not always available, e.g., compiled native frames have no scope > so there are no locals. So the output `- waiting on available>` also acceptable. > > The change has been verified by -Xmixed/-Xcomp(c2)/-Xcomp > -XX:TieredStopAtLevel=1(c1), only change the testcase, no risk. For noisy test failures, we often waive the 24 hour rule in order to quiet down the CI. - PR Comment: https://git.openjdk.org/jdk/pull/20151#issuecomment-2226526095
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" Is it useful to allow (but ignore) an "empty" command to perhaps allow perf analysis of the dcmd mechanism? - PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2226515899
Re: RFR: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent [v3]
On Fri, 12 Jul 2024 17:02:56 GMT, Chris Plummer wrote: >> The original code had 2 vm.resume() - one on them to match vm.suspend() and >> 2nd one to allow debugee to continue on error. >> Now we have 3 vm.resume() - one is to match vm.suspend() (line 377) and 2 >> conditional (on error). >> Theoretically we can get an error when both vm and thread2 are suspended, so >> 2 vm.resume() looks reasonable. >> Anyway resume() is a nop if the thread is not suspended > > After reaching the 2nd breakpoint, which suspends thread2, we do a > vm.suspend(), which bumps the thread2 suspendCount to 2. However, we do a > eventSet.resume() after this, lowering the suspendCount to 1, and there is no > error bailout point while the suspendCount is 2. Thus only 1 vm.resume() is > needed in the error handling. I think all this discussion about the number of `vm.resume()` calls that are needed or not needed and the fact that one of those `vm.resume()` calls could be replaced by a thread2.resume() call _perfectly_ illustrates just how complicated this test is. Thanks for going thru the effort to get rid of the `sleep()` call. I appreciate it. - PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1676519121
Re: RFR: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent [v3]
On Thu, 11 Jul 2024 22:36:05 GMT, Chris Plummer wrote: >> The test hits a breakpoint on thread2 with SUSPEND_EVENT_THREAD policy, so >> only thread2 is suspended. It then does a vm.suspend(), which suspends all >> threads and bumps the suspendCount of thread2 up to 2. It then does an >> eventSet.resume(), which decrements the thread2 suspendCount to 1, so now >> all threads are suspended with a suspendCount of 1. thread2 is then resumed >> and we expect to hit the next thread2 breakpoint. The problem is that >> thread2 can't hit the breakpoint until the main thread has proceeded far >> enough, and if the vm.suspend() that suspended the main thread happens too >> quickly, it won't have proceeded far enough, so thread2 never hits the >> breakpoint. >> >> Essentially we need a vm.resume() to allow the main thread to run, but we >> need to do it in a way that does nullify part of what the test is testing >> for. So in order to allow the vm.resume() but not subvert the intent of the >> test, we first do a thread2.suspend() so the vm.resume() won't resume >> thread2. >> >> Testing in progress: tier1 and tier5 svc testing > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copyright and jcheck error Is there any chance that all the `Breakpint` typos can be fixed? - PR Comment: https://git.openjdk.org/jdk/pull/20088#issuecomment-2226446333
Re: RFR: 8336257: Additional tests in jmxremote/startstop to match on PID not app name [v2]
On Thu, 11 Jul 2024 15:38:29 GMT, Kevin Walls wrote: >> Follow-on from JDK-8207908. >> >> Two tests are broken by that change: >> sun/management/jmxremote/startstop/JMXStartStopTest.java >> sun/management/jmxremote/startstop/JMXStatusPerfCountersTest.java >> >> These are additional tests which use jcmd and an application name pattern to >> find a process. >> They should use the PID to avoid finding a process from some other >> concurrent test invocation. >> So it's good to have the same kind of change applied here too. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > line The fix looks good to me. I presume it has been tested with a Mach5 Tier3 job set. @kevinjwalls - When do you expect to integrate this fix? It looks like we're seeing 34 failures per Tier3 job set due to this issue. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20138#pullrequestreview-2172977359 PR Comment: https://git.openjdk.org/jdk/pull/20138#issuecomment-2223872373
Re: RFR: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent
On Wed, 10 Jul 2024 21:28:50 GMT, Chris Plummer wrote: >> Thank you for the confirming the reason of the timeout. >> >> To be more clear about my point: >> The test has 3 scenarios (see the test description): >> ThreadReference.resume() resumes the thread suspended with: >> * - with thread2.suspend() >> * - at a breakpoint >> * - with VirtualMachine.suspend() >> >> So for 3rd scenario we should not call vm.resume() (as it converts 3rd >> scenario to 1st scenario). >> The test can be fixed by different ways, to me remove logging between >> breakpoint2 and breakpoint3 is the simplest way. >> Note that breakpoint2 is "runt2(), line 2" and breakpoint3 is "runt1(), line >> 7", there are 2 log statements. We can move breakpoint 3 to "runt2(), line >> 3" (I don't see much sense to have breakpoint 3 so far from breakpoint2 - we >> just need to ensure the thread was resumed ) > > Removing log() statements to fix the problem can be risky because someone > could re-add them in the future. What about my idea of doing a short sleep > before the vm.suspend() to make sure the main thread has advanced to the > pipe.readln(), and won't be doing any more log calls until it gets the next > command from the debugger (which should be "quit"). Adding a `sleep()` between two statements does not scale when the test in question is under different loads or run with different options. All it will do is make a hang more rare (and frustrating to analyze). We do use short sleeps when we are in a while-loop checking on a condition of some sort that indicates that the "thing" for which we are waiting to happen has happened. - PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1673123705
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > fix Boot-Class-Path value for Windows Ping @sspitsyn - who handles JLI/JPLIS reviews for the Serviceability team? - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2142976174
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Wed, 15 May 2024 06:00:46 GMT, Serguei Spitsyn wrote: >> I'm not sure this answered Chris' query properly. Or I'm reading Chris' >> query wrong. >> >> Perhaps this is not what Chris had in mind, but I'm wondering what happens >> in some >> Thread-A when it is checked and passed by but then Thread-A sets the flag in >> itself >> after the for-loop has passed it by. Does that Thread-A flag value get lost? > >> Perhaps this is not what Chris had in mind, but I'm wondering what happens >> in some >> Thread-A when it is checked and passed by but then Thread-A sets the flag in >> itself >> after the for-loop has passed it by. Does that Thread-A flag value get lost? > > Thank you for the question. > The Thread-A sets the flag optimistically and then re-checks if > `sync_protocol_enabled()` and any disabler exists. It can be global disbaler > (`_VTMS_transition_disable_for_all_count > 0`) or disabler of `Thread-A` only > (`java_lang_Thread::VTMS_transition_disable_count(vth()) > 0`). If any > disabler exists then `Thread-A` clears the optimistic settings and goes with > the pessimistic approach under protection of `JvmtiVTMSTransition_lock`. > > Please, let me know if you still have questions. This algorithm sounds correct. Thanks for closing the loop on my belated comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1603957324
Re: RFR: 8332042: Move MEMFLAGS to its own include file [v2]
On Mon, 13 May 2024 14:44:05 GMT, Stefan Karlsson wrote: >> I rather have this explicit check. If MEMFLAGS>1byte, things break, and I >> would like to make that explicit. >> >> That said, I can move this static assert to the header. I just wanted to >> avoid including debug.hpp. My original intent was for this cpp file to be >> the place in the future for any MEMFLAGS related utility functions, e.g. >> to-and-from-string conversations. > > Could you instead put the static_assert near the code that will break? Right > now it looks obscure and weird to have this check when it is obviously > correct as long as no one changes the definition. Would it be enough to write > a comment in the header that this needs to be 1 byte? To quote @robehn - Why write a comment for a rule if you can enforce it with code instead... - PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598665179
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Tue, 30 Apr 2024 01:49:31 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/prims/jvmtiThreadState.cpp line 366: >> >>> 364: attempts--; >>> 365: } >>> 366: DEBUG_ONLY(if (attempts == 0) break;) >> >> Previously `_VTMS_transition_count` considered all threads at the same time. >> Now you are iterating through the threads and looking at a flag in each one. >> Is it guaranteed that once the `_VTMS_transition_mark` flag has been >> verified not to be set in a thread it won't get set while still iterating in >> the threads loop? > > Thank you for the comment. It is thinking in a right direction. > Each `JavaThread` set the `VTM_transition_mark` only once and then checks for > disable counters: > - `_VTMS_transition_disable_for_all_count` > - `java_lang_Thread::VTMS_transition_disable_count(vth())` > > If any of the disable counters is not zero then each `JavaThread` clears the > optimistically set mark and continues under protection of the > `JvmtiVTMSTransition_lock`. I'm not sure this answered Chris' query properly. Or I'm reading Chris' query wrong. Perhaps this is not what Chris had in mind, but I'm wondering what happens in some Thread-A when it is checked and passed by but then Thread-A sets the flag in itself after the for-loop has passed it by. Does that Thread-A flag value get lost? - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1597266296
Re: RFR: 8331466: Problemlist serviceability/dcmd/gc/RunFinalizationTest.java on macos-aarch64 [v2]
On Wed, 8 May 2024 01:19:05 GMT, SendaoYan wrote: >> Hi, >> GHA >> [runner](https://github.com/sendaoYan/jdk-ysd/actions/runs/8881868940/job/24386063136) >> shows that serviceability/dcmd/gc/RunFinalizationTest.java intermittent >> fail on macos-aarch64. The testcase has been problemlisted on >> linux-all,windows-x64,aix-ppc64. I think we should add the problemlist with >> macos-aarch64. >> >> Only change the ProblemList, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > problemlist serviceability/dcmd/gc/RunFinalizationTest.java on generic-all I've updated the bug's sysnopsis: s/macos-aarch64/generic-all/ The fastest way to update the PR's title is with `/issue JDK-8331466` - PR Comment: https://git.openjdk.org/jdk/pull/19033#issuecomment-210324
Re: RFR: 8329113: Deprecate -XX:+UseNotificationThread
On Fri, 19 Apr 2024 01:16:46 GMT, Alex Menkov wrote: > The fix deprecates -XX:+UseNotificationThread VM option > > Testing: tier1-3 Thumbs up. Have you looked for any tests that are using this option? - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18852#pullrequestreview-2011868665
Re: RFR: 8330131: Problemlist serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
On Sat, 13 Apr 2024 23:35:07 GMT, Leonid Mesnik wrote: > Please review the fix which excludes the test that failing intermittently. Thumbs up. This is a trivial fix. You originally had 8330131 as a sub-task of 8318729 which has only a single sighting. Then I realized you were also including 8318090 which has 15 sightings. I made 8330131 a sub-task of the earlier/older bug: 8318090. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18772#pullrequestreview-1999660646
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v3]
On Wed, 3 Apr 2024 23:05:24 GMT, Coleen Phillimore wrote: >> This change simplifies the code that grows the jmethodID cache in >> InstanceKlass. Instead of lazily, when there's a rare request for a >> jmethodID for an obsolete method, the jmethodID cache is grown during the >> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily >> allocated when there's a jmethodID allocated, so not every InstanceKlass has >> a cache, but the growth now only happens in a safepoint. This code will >> become racy with the potential change for deallocating jmethodIDs. >> >> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in >> case they're not in tier1-4). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix spacing and punctuation. make log_info into log_debug. Thanks for the fixes. There are a couple that you missed. src/hotspot/share/oops/instanceKlass.cpp line 2313: > 2311: size_t size = idnum_allocated_count(); > 2312: assert(size > (size_t)idnum, "should already have space"); > 2313: jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass); nit: spaces around operator `+` src/hotspot/share/oops/instanceKlass.cpp line 2346: > 2344: // Allocate a larger one and copy entries to the new one. > 2345: // They've already been updated to point to new methods where > applicable (i.e., not obsolete). > 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1, > mtClass); nit: spaces around operator `+` - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1978305784 PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550653423 PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550653504
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 13:25:36 GMT, Coleen Phillimore wrote:
>> This change simplifies the code that grows the jmethodID cache in
>> InstanceKlass. Instead of lazily, when there's a rare request for a
>> jmethodID for an obsolete method, the jmethodID cache is grown during the
>> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily
>> allocated when there's a jmethodID allocated, so not every InstanceKlass has
>> a cache, but the growth now only happens in a safepoint. This code will
>> become racy with the potential change for deallocating jmethodIDs.
>>
>> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in
>> case they're not in tier1-4).
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Refactoring suggested by Serguei.
Okay I've crawled thru the changes twice and I went back thru
the bug history for this code and added some notes and links
to the bug ID.
Modulo the nits that I flagged, I think the changes are fine. Making
cache growth only happen in the RedefineClasses safepoint is
definite improvement.
I see that you've run JVM/TI and JLI tests. You should also run JDI
tests. Basically for a low level fix like this that affects JVM/TI, you
should run Mach5 Tier[1-6].
src/hotspot/share/oops/instanceKlass.cpp line 2272:
> 2270: jmethodID InstanceKlass::update_jmethod_id(jmethodID* jmeths, Method*
> method, int idnum) {
> 2271: if (method->is_old() && !method->is_obsolete()) {
> 2272: // If the method passed in is old (but not obsolete), use the
> current version
nit: should end with a period.
src/hotspot/share/oops/instanceKlass.cpp line 2277:
> 2275: }
> 2276: jmethodID new_id = Method::make_jmethod_id(class_loader_data(),
> method);
> 2277: Atomic::release_store(&jmeths[idnum+1], new_id);
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2304:
> 2302: //
> 2303: // If the RedefineClasses() API has been used, then this cache grows
> 2304: // in the redefinition safepoint.
Much easier to reason about. Thanks for simplifying it.
src/hotspot/share/oops/instanceKlass.cpp line 2314:
> 2312: assert(size > (size_t)idnum, "should already have space");
> 2313: jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass);
> 2314: memset(jmeths, 0, (size+1)*sizeof(jmethodID));
nit: spaces around operator `+` (two places)
nit: spaces around operator `*`
src/hotspot/share/oops/instanceKlass.cpp line 2325:
> 2323: }
> 2324:
> 2325: jmethodID id = Atomic::load_acquire(&jmeths[idnum+1]);
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2328:
> 2326: if (id == nullptr) {
> 2327: MutexLocker ml(JmethodIdCreation_lock,
> Mutex::_no_safepoint_check_flag);
> 2328: id = jmeths[idnum+1];
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2343:
> 2341: size_t size = idnum_allocated_count();
> 2342: size_t old_size = (size_t)cache[0];
> 2343: if (old_size < size+1) {
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2344:
> 2342: size_t old_size = (size_t)cache[0];
> 2343: if (old_size < size+1) {
> 2344: // allocate a larger one and copy entries to the new one.
nit typo: s/allocate/Allocate/
src/hotspot/share/oops/instanceKlass.cpp line 2345:
> 2343: if (old_size < size+1) {
> 2344: // allocate a larger one and copy entries to the new one.
> 2345: // They've already been updated to point to new methods where
> applicable (ie. not obsolete)
nit typo: s/ie./i.e.,/
Please add a period at the end of the sentence.
src/hotspot/share/oops/instanceKlass.cpp line 2347:
> 2345: // They've already been updated to point to new methods where
> applicable (ie. not obsolete)
> 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1,
> mtClass);
> 2347: memset(new_cache, 0, (size+1)*sizeof(jmethodID));
nit: spaces around operator `+` (two places)
nit: spaces around operator `*`
src/hotspot/share/oops/instanceKlass.cpp line 2348:
> 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1,
> mtClass);
> 2347: memset(new_cache, 0, (size+1)*sizeof(jmethodID));
> 2348: // cache size is stored in element[0], other elements offset by
> one
nit typo: s/cache/Cache/
Please add a period at the end.
src/hotspot/share/oops/instanceKlass.cpp line 2384:
> 2382: int idnum = method->method_idnum();
> 2383: jmethodID* jmeths = methods_jmethod_ids_acquire();
> 2384: return (jmeths != nullptr) ? jmeths[idnum+1] : nullptr;
nit: spaces around operator `+`
src/hotspot/share/oops/method.cpp line 2200:
> 2198:
> 2199: ResourceMark rm;
> 2200: log_info(jmethod)("Creating jmethodID for Method %s",
> m->external_name());
Hmmm... will this be too noisy for `info` level?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4353:
> 435
Integrated: 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64
On Mon, 1 Apr 2024 21:07:34 GMT, Daniel D. Daugherty wrote: > Trivial fixes to ProblemList noisy tests: > > [JDK-8329425](https://bugs.openjdk.org/browse/JDK-8329425) ProblemList > containers/docker/TestJFREvents.java on linux-x64 > [JDK-8329426](https://bugs.openjdk.org/browse/JDK-8329426) ProblemList > vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java > with Xcomp on windows-x64 > [JDK-8329427](https://bugs.openjdk.org/browse/JDK-8329427) ProblemList > javax/sound/sampled/Clip/ClipFlushCrash.java on linux-x64 > [JDK-8329428](https://bugs.openjdk.org/browse/JDK-8329428) ProblemList > vmTestbase/nsk/stress/thread/thread006.java on linux-all in Xcomp This pull request has now been integrated. Changeset: c2979c15 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/c2979c150bdbcc2a9e6026347dc590e6a7e86595 Stats: 5 lines in 3 files changed: 4 ins; 0 del; 1 mod 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64 8329426: ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on windows-x64 8329427: ProblemList javax/sound/sampled/Clip/ClipFlushCrash.java on linux-x64 8329428: ProblemList vmTestbase/nsk/stress/thread/thread006.java on linux-all in Xcomp Reviewed-by: dholmes - PR: https://git.openjdk.org/jdk/pull/18568
Re: RFR: 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64
On Mon, 1 Apr 2024 22:15:06 GMT, David Holmes wrote: >> Trivial fixes to ProblemList noisy tests: >> >> [JDK-8329425](https://bugs.openjdk.org/browse/JDK-8329425) ProblemList >> containers/docker/TestJFREvents.java on linux-x64 >> [JDK-8329426](https://bugs.openjdk.org/browse/JDK-8329426) ProblemList >> vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java >> with Xcomp on windows-x64 >> [JDK-8329427](https://bugs.openjdk.org/browse/JDK-8329427) ProblemList >> javax/sound/sampled/Clip/ClipFlushCrash.java on linux-x64 >> [JDK-8329428](https://bugs.openjdk.org/browse/JDK-8329428) ProblemList >> vmTestbase/nsk/stress/thread/thread006.java on linux-all in Xcomp > > Seems okay. Thanks @dholmes-ora - Thanks for the lightning fast review! - PR Comment: https://git.openjdk.org/jdk/pull/18568#issuecomment-2030672631
RFR: 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64
Trivial fixes to ProblemList noisy tests: [JDK-8329425](https://bugs.openjdk.org/browse/JDK-8329425) ProblemList containers/docker/TestJFREvents.java on linux-x64 [JDK-8329426](https://bugs.openjdk.org/browse/JDK-8329426) ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on windows-x64 [JDK-8329427](https://bugs.openjdk.org/browse/JDK-8329427) ProblemList javax/sound/sampled/Clip/ClipFlushCrash.java on linux-x64 [JDK-8329428](https://bugs.openjdk.org/browse/JDK-8329428) ProblemList vmTestbase/nsk/stress/thread/thread006.java on linux-all in Xcomp - Commit messages: - 8329428: ProblemList vmTestbase/nsk/stress/thread/thread006.java on linux-all in Xcomp - 8329427: ProblemList javax/sound/sampled/Clip/ClipFlushCrash.java on linux-x64 - 8329426: ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java with Xcomp on windows-x64 - 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64 Changes: https://git.openjdk.org/jdk/pull/18568/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18568&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329425 Stats: 5 lines in 3 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18568.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18568/head:pull/18568 PR: https://git.openjdk.org/jdk/pull/18568
Re: RFR: 8328273: sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java failed with java.rmi.server.ExportException: Port already in use
On Mon, 1 Apr 2024 02:02:40 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix the test > failure reported in https://bugs.openjdk.org/browse/JDK-8328273? > > As noted in that issue, the > `sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java` intermittently > fails with a port already in use error. The test attempts to find a free port > and then uses it during the test. The interesting part is that the test > already has a loop of 10 attempts to retry the test if the port wasn't > actually free. So for the test to fail, it would then mean that each of the > 10 attempts of using a free port failed (which should be extremely rare and > should almost never happen). > > I didn't have an answer for that until today and had it on my TODO to look > further. Credit goes to Kevin @kevinjwalls for identifying the issue - turns > out this is the exact same issue that Kevin fixed in > https://github.com/openjdk/jdk/pull/18470 for a different test. After > noticing that fix, I spotted the same typo in the exception message check in > this test. That explains why it wasn't retrying at most 10 times. The test > was thus immediately failing on first attempt whenever the chosen free port > was in use. > > I have run this test with a test repeat of 50 with this change and the test > now passes always. Without this change and a test repeat of 50, the test > failed 2 times. I've additionally searched for any other similar typos in > other tests and haven't found any (I searched for the string "Exception > thrown by the agent :"). Thumbs up. This is a trivial fix. @jaikiran - Thanks for fixing this bug!! - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18561#pullrequestreview-1972108656
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Fri, 29 Mar 2024 23:02:03 GMT, Serguei Spitsyn wrote: >> So that would mean that the native side would always wait for 100 seconds? >> Or will it wait for some increment of time upto a maximum of 100 seconds? > > It wait for some increment of time upto a maximum of 100 seconds. I'm good with that. Thanks for clarifying. - PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544999877
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Fri, 29 Mar 2024 20:44:48 GMT, Serguei Spitsyn wrote:
>> runtime/8176717/TestInheritFD.java has an example of what I'm talking about:
>>
>> public static float timeoutFactor =
>> Float.parseFloat(System.getProperty("test.timeout.factor", "1.0"));
>> public static long subProcessTimeout = (long)(15L * timeoutFactor);
>>
>> so you fetch the test.timeout.factor value and then you scale your delay
>> value.
>>
>> Also:
>>
>> nit typo: s/waitig/waiting/
>
> Thank you for the example and for catching the typo. The timeout factor also
> needs to be passed to the native side. I think, this fragment is not worth
> this kind of extra complexity. One approach would be to just make it big
> enough, eg. make it 100 seconds instead of 10. Another - to get rid of this
> trap at all. What would you prefer?
So that would mean that the native side would always wait for 100 seconds?
Or will it wait for some increment of time upto a maximum of 100 seconds?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544915772
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Thu, 28 Mar 2024 23:29:53 GMT, Serguei Spitsyn wrote:
>> Caught this comment in passing. Delays like this should be scaled with
>> defaultTimeoutFactor so that test tasks that invoke tests with options
>> that can slow the test down, e.g., `-Xcomp`, can be accommodated.
>>
>> I believe the defaultTimeoutFact for `-Xcomp` test tasks gets bumped
>> from 4 to 10.
>
> Thanks for the comments, Chris and Dan. Updated as Chris suggested. I've
> added this with `-Xcomp` consideration as the worst case scenario in mind.
> Now, I think it is more save to make it 10 seconds instead of one. Is it
> going to be good enough? In fact, I've added this for manual testing to save
> time in waiting for test completion when it is deadlocked. Also, this is
> better for diagnosability.
runtime/8176717/TestInheritFD.java has an example of what I'm talking about:
public static float timeoutFactor =
Float.parseFloat(System.getProperty("test.timeout.factor", "1.0"));
public static long subProcessTimeout = (long)(15L * timeoutFactor);
so you fetch the test.timeout.factor value and then you scale your delay value.
Also:
nit typo: s/waitig/waiting/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544497738
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Wed, 27 Mar 2024 20:08:19 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: improve diagnostics and reliability
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp
> line 163:
>
>> 161: LOG("Main: ensureAtBreakpoint: waitig 5 millis\n");
>> 162: if (++attempts > 100) {
>> 163: fatal(jni, "Main: ensureAtBreakpoint: waited 1 sec");
>
> 1 second isn't very long when you are talking about something that is relying
> on debugger/debuggee communication. Maybe wait 100ms at a time for a total of
> 10 seconds.
Caught this comment in passing. Delays like this should be scaled with
defaultTimeoutFactor so that test tasks that invoke tests with options
that can slow the test down, e.g., `-Xcomp`, can be accommodated.
I believe the defaultTimeoutFact for `-Xcomp` test tasks gets bumped
from 4 to 10.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1541986223
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v2]
On Fri, 22 Mar 2024 19:26:33 GMT, Alex Menkov wrote:
>> The change fixes 3 nsk JDI tests.
>> Root cause in all 3 tests is the same - the tests requests JDI event with
>> SUSPEND_ALL policy, but event handler thread stops handle incoming event and
>> this causes debuggee to hang (suspended by JDI event).
>>
>> All 3 tests are updated to exit event handler thread after getting
>> VMDeathEvent or VMDisconnectEvent (and resume debuggee after any other
>> events).
>> ClassPrepareEvent tests need to wait some time to allow handle all expected
>> events before terminate the debuggee. The logic was implemented by using
>> CountDownLatch.
>>
>> All tests are passed with "--test-repeat 20"
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback
Thumbs up. I only have nit comments.
test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
line 117:
> 115: boolean isConnected = true;
> 116: boolean eventsReceived = false;
> 117: // handle events until debugee is disconnected
Nit typo: s/debugee/debuggee/
But a lot of the NSK tests have this typo...
test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
line 203:
> 201: }
> 202:
> 203: // Check that all expected
> ClassPrepareEvent are received
nit typo: s/ClassPrepareEvent/ClassPrepareEvent(s)/
nit typo: Since this comment starts with a capital, it should have a period at
the end.
test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
line 207:
> 205: eventsReceived = true;
> 206: for (int i = 0; i <
> checkedTypes.length; i++) {
> 207: if
> (checkedTypes[i][2] == "0") {
This if-statement could use a comment to explain the logic.
test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/thread/thread001.java
line 138:
> 136: boolean isConnected = true;
> 137: boolean eventsReceived = false;
> 138: // handle events until debugee is disconnected
Nit typo: s/debugee/debuggee/
But a lot of the NSK tests have this typo...
test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/thread/thread001.java
line 233:
> 231: }
> 232:
> 233: // Check that all expected
> ClassPrepareEvent are received
nit typo: s/ClassPrepareEvent/ClassPrepareEvent(s)/
nit typo: Since this comment starts with a capital, it should have a period at
the end.
test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/thread/thread001.java
line 237:
> 235: eventsReceived = true;
> 236: for (int i = 0; i <
> checkedThreads.length; i++) {
> 237: if
> (checkedThreads[i][2] == "0") {
This if-statement could use a comment to explain the logic.
-
Marked as reviewed by dcubed (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18442#pullrequestreview-1955733734
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536172906
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536176808
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536178687
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536181360
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536183605
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536184067
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v8]
On Fri, 8 Mar 2024 06:05:09 GMT, Serguei Spitsyn wrote: >> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not >> match the spec. It can sometimes return an incorrect information about the >> contended monitor. Such a behavior does not match the function spec. >> With this update the `GetCurrentContendedMonitor()` is returning the monitor >> only when the specified thread is waiting to enter or re-enter the monitor, >> and the monitor is not returned when the specified thread is waiting in the >> `java.lang.Object.wait` to be notified. >> >> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` >> command and JDI `ThreadReference.currentContendedMonitor()` method are based >> and depend on this JVMTI function. The JDWP command and the JDI method were >> specified incorrectly and had incorrect behavior. The fix slightly corrects >> both the JDWP and JDI specs. The JDWP and JDI implementations have been also >> fixed because they use this JVM TI update. Please, see and review the >> related CSR and Release-Note. >> >> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI >> GetCurrentContendedMonitor is implemented incorrectly >> RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: >> JVM TI GetCurrentContendedMonitor is implemented incorrectly >> >> Testing: >> - tested with the mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge > - review: updated test desciption for two modified tests > - review: added new internal function > JvmtiEnvBase::get_thread_or_vthread_state > - review: replaced timed with timed-out in JDWP and JDI spec > - Merge > - Merge > - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI > ThreadReference.currentContendedMonitor method > - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly I looked at the commit and it appears that not all copyright years got fixed. - PR Comment: https://git.openjdk.org/jdk/pull/17944#issuecomment-1989283515
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v8]
On Fri, 8 Mar 2024 06:05:09 GMT, Serguei Spitsyn wrote: >> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not >> match the spec. It can sometimes return an incorrect information about the >> contended monitor. Such a behavior does not match the function spec. >> With this update the `GetCurrentContendedMonitor()` is returning the monitor >> only when the specified thread is waiting to enter or re-enter the monitor, >> and the monitor is not returned when the specified thread is waiting in the >> `java.lang.Object.wait` to be notified. >> >> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` >> command and JDI `ThreadReference.currentContendedMonitor()` method are based >> and depend on this JVMTI function. The JDWP command and the JDI method were >> specified incorrectly and had incorrect behavior. The fix slightly corrects >> both the JDWP and JDI specs. The JDWP and JDI implementations have been also >> fixed because they use this JVM TI update. Please, see and review the >> related CSR and Release-Note. >> >> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI >> GetCurrentContendedMonitor is implemented incorrectly >> RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: >> JVM TI GetCurrentContendedMonitor is implemented incorrectly >> >> Testing: >> - tested with the mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge > - review: updated test desciption for two modified tests > - review: added new internal function > JvmtiEnvBase::get_thread_or_vthread_state > - review: replaced timed with timed-out in JDWP and JDI spec > - Merge > - Merge > - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI > ThreadReference.currentContendedMonitor method > - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly Thumbs up. I'm not sure how I missed reviewing this change so I did a post-integration review. - PR Review: https://git.openjdk.org/jdk/pull/17944#pullrequestreview-1928978384
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Fri, 8 Mar 2024 06:11:23 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 26 commits:
>
> - Merge
> - review: minor tweak in test description of ObjectMonitorUsage.java
> - review: addressed more comments on the fix and new test
> - rename after merge: jvmti_common.h to jvmti_common.hpp
> - Merge
> - review: update comment in threads.hpp
> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
> - resolve merge conflict for deleted file objmonusage003.cpp
> - fix a typo in libObjectMonitorUsage.cpp
> - fix potential sync gap in the test ObjectMonitorUsage
> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
Thumbs up on the v22 version of this change. Of course, I'm assuming
that the updated tests pass and that there are no new failures in the
usual Mach5 testing.
Sorry for the delay in getting back to this review. A combination of
vacation following by illness delayed me getting back to this PR.
Please note that I did not try to compare the (deleted) objmonusage003
version of the test with the (new) ObjectMonitorUsage version. I didn't
think that would be useful.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1483:
> 1481: markWord mark = hobj->mark();
> 1482: ResourceMark rm(current_thread);
> 1483: GrowableArray* wantList = nullptr;
Thanks for refactoring `JvmtiEnvBase::get_object_monitor_usage()` to
be much more clear. It was difficult to review in GitHub so I switched to
the webrev frames view so that I could scroll the two sides back and forth
and determine equivalence.
src/hotspot/share/runtime/threads.cpp line 1186:
> 1184:
> 1185: #if INCLUDE_JVMTI
> 1186: // Get count Java threads that are waiting to enter or re-enter the
> specified monitor.
nit typo: s/count Java/count of Java/
src/hotspot/share/runtime/threads.hpp line 134:
> 132: static unsigned print_threads_compiling(outputStream* st, char* buf,
> int buflen, bool short_form = false);
> 133:
> 134: // Get count Java threads that are waiting to enter or re-enter the
> specified monitor.
nit typo: s/count Java/count of Java/
Also this file needs a copyright year update.
test/hotspot/jtreg/TEST.quick-groups line 972:
> 970:
> vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001/TestDescription.java
> \
> 971:
> vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage00
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 11 Mar 2024 18:08:14 GMT, Serguei Spitsyn wrote: >> Sorry I missed this response. I can't see a way to address spurious wakeups >> in this case as it needs to be a per-thread flag (so that each thread knows >> it was notified) but you don't know which thread will be notified in any >> given call to `notify()`. I also can't see how you can detect a spurious >> wakeup in this code. If they happen then a subtest may fail due to an >> unexpected number of re-entering threads. >> I think we will just have to see how stable the test is in practice. > > Okay, thanks! I'll add some diagnostic code to catch spurious wakups. > Let's see if we ever encounter any spurious wakeup in this test. I think some sort of comment needs to be added here to document the possibility of this code path being affect by a spurious wakeup. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520260059
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Fri, 8 Mar 2024 06:11:23 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 26 commits:
>
> - Merge
> - review: minor tweak in test description of ObjectMonitorUsage.java
> - review: addressed more comments on the fix and new test
> - rename after merge: jvmti_common.h to jvmti_common.hpp
> - Merge
> - review: update comment in threads.hpp
> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
> - resolve merge conflict for deleted file objmonusage003.cpp
> - fix a typo in libObjectMonitorUsage.cpp
> - fix potential sync gap in the test ObjectMonitorUsage
> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
I last reviewed v02 of this PR so I've jumped forward to the full v22 webrev.
It may take
me a bit of time to redo this review.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1989046765
Integrated: 8326117: ProblemList serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default in Xcomp mode
On Sun, 18 Feb 2024 14:56:03 GMT, Daniel D. Daugherty wrote: > Trivial fixes to ProblemList a couple of tests: > [JDK-8326117](https://bugs.openjdk.org/browse/JDK-8326117) ProblemList > serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default > in Xcomp mode > [JDK-8326120](https://bugs.openjdk.org/browse/JDK-8326120) ProblemList > sun/java2d/X11SurfaceData/SharedMemoryPixmapsTest/SharedMemoryPixmapsTest.sh > on macosx-aarch64 This pull request has now been integrated. Changeset: 099b7442 Author: Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/099b744235a28331b99f7b429cf1e8abcb367c41 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod 8326117: ProblemList serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default in Xcomp mode 8326120: ProblemList sun/java2d/X11SurfaceData/SharedMemoryPixmapsTest/SharedMemoryPixmapsTest.sh on macosx-aarch64 Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17904
Re: RFR: 8326117: ProblemList serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default in Xcomp mode
On Sun, 18 Feb 2024 16:28:51 GMT, Alan Bateman wrote: >> Trivial fixes to ProblemList a couple of tests: >> [JDK-8326117](https://bugs.openjdk.org/browse/JDK-8326117) ProblemList >> serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default >> in Xcomp mode >> [JDK-8326120](https://bugs.openjdk.org/browse/JDK-8326120) ProblemList >> sun/java2d/X11SurfaceData/SharedMemoryPixmapsTest/SharedMemoryPixmapsTest.sh >> on macosx-aarch64 > > Marked as reviewed by alanb (Reviewer). @AlanBateman - Thanks for the review! Especially on a Sunday!! - PR Comment: https://git.openjdk.org/jdk/pull/17904#issuecomment-1951388559
RFR: 8326117: ProblemList serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default in Xcomp mode
Trivial fixes to ProblemList a couple of tests: [JDK-8326117](https://bugs.openjdk.org/browse/JDK-8326117) ProblemList serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default in Xcomp mode [JDK-8326120](https://bugs.openjdk.org/browse/JDK-8326120) ProblemList sun/java2d/X11SurfaceData/SharedMemoryPixmapsTest/SharedMemoryPixmapsTest.sh on macosx-aarch64 - Commit messages: - 8326120: ProblemList sun/java2d/X11SurfaceData/SharedMemoryPixmapsTest/SharedMemoryPixmapsTest.sh on macosx-aarch64 - 8326117: ProblemList serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java#default in Xcomp mode Changes: https://git.openjdk.org/jdk/pull/17904/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17904&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326117 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17904.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17904/head:pull/17904 PR: https://git.openjdk.org/jdk/pull/17904
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v8]
On Wed, 14 Feb 2024 15:25:33 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: added assert to get_pending_threads; added suggested coverage to
> test objmonusage003
CHECK_FOR_BAD_RESULTS and ADD_DELAYS_FOR_RACES flags and
their associated code paths are for temporary debugging only.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1944611040
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Wed, 14 Feb 2024 11:56:27 GMT, Serguei Spitsyn wrote: > Thanks, Dan. I've pushed the suggested test changes but refactored them a > little bit. You are welcome. I presume the revised test passes with your fix in place. I made the test changes on a baseline repo and not a repo that had your changes which is why I had temporary debug flags in the test.. Also, do you agree with my test assertion comments in the verification point comments? Am I properly understanding how we are changing this API? - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1944382731
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 08:39:38 GMT, Serguei Spitsyn wrote: >> Sorry really struggling to understand this now. We have gone from a simple >> miscalculation to apparently doing everything wrong. >> >> IIUC this API does not currently handle virtual threads correctly -i s that >> the case? If so I would like to see that fix factored out and done >> separately before this fix is done. Thanks. > >> Sorry really struggling to understand this now. We have gone from a simple >> miscalculation to apparently doing everything wrong. IIUC this API does not >> currently handle virtual threads correctly - is that the case? If so I would >> like to see that fix factored out and done separately before this fix is >> done. Thanks. > > Thank you for the concern, David. But it is not clear what it is. Could you, > please, explain a little bit? > Why do you think the virtual threads are handled incorrectly? @sspitsyn - When you get the chance, can you checkout these possible changes for the objmonusage001 test? https://github.com/openjdk/jdk/pull/17839 Summary of my test changes: - extend the check() function to verify more of the jvmtiMonitorUsage fields - add detailed comments at each of the verification points so it's clear what is being verified and why - add a better summary for the overall test - make it clear which output lines document failures I've also made some temporary debugging changes that are only useful while updating this test: - temporarily use the "printdump" parameter to enable test output - add a flag to verify the current JVMs bad jvmtiMonitorUsage fields so that the test can pass with an unmodified JVM - add a flag to add a delay that helps check for a lack of races in the verification points These proposed test changes help document how I think the GetObjectMonitorUsage API is supposed to work on a simple test case. - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1942747157
Re: RFR: 8307977: jcmd and jstack broken for target processes running with elevated capabilities
On Tue, 30 Jan 2024 10:47:22 GMT, Sebastian Lövdahl wrote: > 8307977: jcmd and jstack broken for target processes running with elevated > capabilities Cool. Thanks for the confirmation. - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1934542288
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Thu, 8 Feb 2024 10:34:14 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1521:
>>
>>> 1519: // we have contending and/or waiting threads
>>> 1520: if (nWant > 0) {
>>> 1521: // we have contending threads
>>
>> This block includes this logic:
>>
>> // get_pending_threads returns only java thread so we do not need to
>> // check for non java threads.
>> GrowableArray* wantList =
>> Threads::get_pending_threads(tlh.list(), nWant, (address)mon);
>> if (wantList->length() < nWant) {
>> // robustness: the pending list has gotten smaller
>> nWant = wantList->length();
>> }
>>
>> `Threads::get_pending_threads()` only returns threads where the
>> `current_pending_monitor` field is set for the specific monitor. That
>> happens only on contended enter and does not happen on contended
>> re-enter so this logic will already strip out any threads in `wait()` that
>> have not been notified and have not had their wait timers expire.
>> It will also strip out any waiters that have been notified or had
>> their wait timeouts expire.
>>
>> This means even if we fix the reenter code to increment the contentions
>> field, this logic will reduce that `nWant` value. Of course, the way around
>> that is to also update the reenter code to properly set the
>> `current_pending_monitor`
>> field and then the reentering threads won't be filtered out...
>
> Yes, I've figured this out now. Thank you for pointing to it.
> It feels, the counts can be calculated correctly without touching the
> implementation of `current_pending_monitor()`, `current_waiting_monitor()`,
> `_contensions` and `_waiters`.
> At least, I'll try to fix it locally in the
> `JvmtiEnvBase::get_object_monitor_usage()`.
> Please, let me know what do you prefer.
I don't have a preference (yet). Like what David suggested, I think we need to
determine the list of thread and monitor interaction scenarios that want to
exercise with this API. Once we're happy that those scenarios represent the
complete list of possible combinations we should double check that against the
API spec to make sure that those scenarios cover the API spec. Finally, we need
to make sure that we have a test that covers all those scenarios.
It has been a long, long time since I've looked at those NSK tests... :-(
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1483168890
Re: RFR: 8307977: jcmd and jstack broken for target processes running with elevated capabilities
On Tue, 30 Jan 2024 10:47:22 GMT, Sebastian Lövdahl wrote: > 8307977: jcmd and jstack broken for target processes running with elevated > capabilities Will this result in files being left in /tmp that are not cleaned up during test runs? - PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1934350422
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Wed, 7 Feb 2024 07:02:11 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains three additional
> commits since the last revision:
>
> - Merge
> - review: thread in notify waiter list can't be BLOCKED
> - 8324677: Specification clarification needed for JVM TI
> GetObjectMonitorUsage
Okay... so we might have an incorrect implementation of JVM TI
GetObjectMonitorUsage, but I'm not convinced that it is broken
in the way that we think it is broken.
Please read thru my unfortunately long comments on the complicated
logic in `JvmtiEnvBase::get_object_monitor_usage()`. I _think_ things
are broken in a different way than what we're talking about in this bug.
In particular, the logic that populates the `waiters` array and reduces
the `waiter_count` value overrides the value that we think we're fixing
in the beginning of the function.
Similarly, the logic that populates the `notify_waiters` array and reduces
the `notify_waiter_count` value also overrides the value that we think
we're fixing in the beginning of the function.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1489:
> 1487: nWait = mon->waiters(); // # of threads in Object.wait()
> 1488: ret.waiter_count = nWant;
> 1489: ret.notify_waiter_count = nWait;
Please note that the comment on L1487 is accurate. The `waiters` count is
incremented just before the thread that has called `wait()` drops the monitor
and that increased count remains in effect until after the thread has reentered
the monitor after being notified or the wait timeout has expired.
The `contentions` count is not incremented in the re-entry path so a thread
that is in `wait()` that has been notified or the wait timeout has expired is
not
counted as a contending thread.
So if we really want the `waiter_count` field to include threads that have been
notified or the wait timeout has expired, then we have to add some logic to
carefully increment the `contentions` count.
This old logic:
```ret.waiter_count = nWant + nWait;```
over counts because it also includes threads in `wait()` that have not yet
been notified and the wait timeout has not expired. However, including
`nWait` is correct for the situation when all of the waiting threads have
been notified or their wait timeouts have expired.
This als
[jdk22] Integrated: 8324082: more monitoring test timeout adjustments
On Thu, 18 Jan 2024 15:22:19 GMT, Daniel D. Daugherty wrote: > Trivial fixes to adjust more monitoring test timeouts. > > See the bug report for the gory timeout details. This pull request has now been integrated. Changeset: 79c3d47c Author: Daniel D. Daugherty URL: https://git.openjdk.org/jdk22/commit/79c3d47cfe975fd7a6a66464e83f5abf6594f941 Stats: 28 lines in 14 files changed: 0 ins; 0 del; 28 mod 8324082: more monitoring test timeout adjustments Reviewed-by: ayang, kevinw Backport-of: 806ffb108572236cb9908ad6f93d7b09dfc6a600 - PR: https://git.openjdk.org/jdk22/pull/92
Re: [jdk22] RFR: 8324082: more monitoring test timeout adjustments
On Thu, 18 Jan 2024 15:36:29 GMT, Albert Mingkun Yang wrote: >> Trivial fixes to adjust more monitoring test timeouts. >> >> See the bug report for the gory timeout details. > > Marked as reviewed by ayang (Reviewer). @albertnetymk and @kevinjwalls - Thanks for the fast reviews! - PR Comment: https://git.openjdk.org/jdk22/pull/92#issuecomment-1898719581
[jdk22] RFR: 8324082: more monitoring test timeout adjustments
Trivial fixes to adjust more monitoring test timeouts. See the bug report for the gory timeout details. - Commit messages: - Backport 806ffb108572236cb9908ad6f93d7b09dfc6a600 Changes: https://git.openjdk.org/jdk22/pull/92/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=92&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324082 Stats: 28 lines in 14 files changed: 0 ins; 0 del; 28 mod Patch: https://git.openjdk.org/jdk22/pull/92.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/92/head:pull/92 PR: https://git.openjdk.org/jdk22/pull/92
Re: RFR: 8324082: more monitoring test timeout adjustments
On Thu, 18 Jan 2024 05:16:33 GMT, Chris Plummer wrote: >> Trivial fixes to adjust more monitoring test timeouts. >> >> See the bug report for the gory timeout details. > > Shouldn't we use a larger timeoutfactor for slowdebug builds? @plummercj, @kevinjwalls and @sspitsyn - Thanks for the reviews. Copyright years updated. @plummercj - The collection of adjustments I have been making to the monitoring tests have all been made with TimeoutFactor == 1. This is intentional because these tests pass with that timeout factor when they are run by themselves on a system. It is only with extremely high stress levels that we see these slowdebug timeouts. - PR Comment: https://git.openjdk.org/jdk/pull/17478#issuecomment-1898459015
Integrated: 8324082: more monitoring test timeout adjustments
On Thu, 18 Jan 2024 01:28:09 GMT, Daniel D. Daugherty wrote: > Trivial fixes to adjust more monitoring test timeouts. > > See the bug report for the gory timeout details. This pull request has now been integrated. Changeset: 806ffb10 Author: Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/806ffb108572236cb9908ad6f93d7b09dfc6a600 Stats: 28 lines in 14 files changed: 0 ins; 0 del; 28 mod 8324082: more monitoring test timeout adjustments Reviewed-by: kevinw, sspitsyn - PR: https://git.openjdk.org/jdk/pull/17478
Re: RFR: 8324082: more monitoring test timeout adjustments [v2]
> Trivial fixes to adjust more monitoring test timeouts. > > See the bug report for the gory timeout details. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: Update copyright years. - Changes: - all: https://git.openjdk.org/jdk/pull/17478/files - new: https://git.openjdk.org/jdk/pull/17478/files/a31a5d19..db7f12f7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17478&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17478&range=00-01 Stats: 14 lines in 14 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/17478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17478/head:pull/17478 PR: https://git.openjdk.org/jdk/pull/17478
RFR: 8324082: more monitoring test timeout adjustments
Trivial fixes to adjust more monitoring test timeouts. See the bug report for the gory timeout details. - Commit messages: - 8324082: more monitoring test timeout adjustments Changes: https://git.openjdk.org/jdk/pull/17478/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17478&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324082 Stats: 14 lines in 14 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/17478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17478/head:pull/17478 PR: https://git.openjdk.org/jdk/pull/17478
Re: RFR: 8322989: New test serviceability/HeapDump/FullGCHeapDumpLimitTest.java fails [v3]
On Thu, 4 Jan 2024 15:37:33 GMT, Denghui Dong wrote: >> Hi, >> >> Please help review this patch that fixes the failures of >> FullGCHeapDumpLimitTest.java caused by passing other gc flags. >> >> Thanks. > > Denghui Dong has updated the pull request incrementally with one additional > commit since the last revision: > > update ProblemListing PR has been approved and should integrate shortly: https://github.com/openjdk/jdk/pull/17269 Please update your repo to include that fix so that you can remove the test from the ProblemList.txt file in your PR. - PR Comment: https://git.openjdk.org/jdk/pull/17263#issuecomment-1877400820
Integrated: 8323011: ProblemList serviceability/HeapDump/FullGCHeapDumpLimitTest.java
A trivial fix to ProblemList serviceability/HeapDump/FullGCHeapDumpLimitTest.java on all platforms. We're already up to 54 failures in Tier3 and Tier5. - Commit messages: - 8323011: ProblemList serviceability/HeapDump/FullGCHeapDumpLimitTest.java Changes: https://git.openjdk.org/jdk/pull/17269/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17269&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323011 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17269.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17269/head:pull/17269 PR: https://git.openjdk.org/jdk/pull/17269
Re: Integrated: 8323011: ProblemList serviceability/HeapDump/FullGCHeapDumpLimitTest.java
On Thu, 4 Jan 2024 16:27:05 GMT, Alexander Zvegintsev wrote: >> A trivial fix to ProblemList >> serviceability/HeapDump/FullGCHeapDumpLimitTest.java >> on all platforms. >> >> We're already up to 54 failures in Tier3 and Tier5. > > Marked as reviewed by azvegint (Reviewer). @azvegint and @ctornqvi - Thanks for the lightning fast reviews! - PR Comment: https://git.openjdk.org/jdk/pull/17269#issuecomment-1877394481
Integrated: 8323011: ProblemList serviceability/HeapDump/FullGCHeapDumpLimitTest.java
On Thu, 4 Jan 2024 16:24:53 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > serviceability/HeapDump/FullGCHeapDumpLimitTest.java > on all platforms. > > We're already up to 54 failures in Tier3 and Tier5. This pull request has now been integrated. Changeset: ea19e9c6 Author: Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/ea19e9c6aa86034055a39c8780156ae4c569de5b Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8323011: ProblemList serviceability/HeapDump/FullGCHeapDumpLimitTest.java Reviewed-by: azvegint, ctornqvi - PR: https://git.openjdk.org/jdk/pull/17269
Re: RFR: 8322989: New test serviceability/HeapDump/FullGCHeapDumpLimitTest.java fails [v3]
On Thu, 4 Jan 2024 15:37:33 GMT, Denghui Dong wrote: >> Hi, >> >> Please help review this patch that fixes the failures of >> FullGCHeapDumpLimitTest.java caused by passing other gc flags. >> >> Thanks. > > Denghui Dong has updated the pull request incrementally with one additional > commit since the last revision: > > update We're up to 48 test failures so far (8 x 6 Tier3 buildIDs) which is very, very noisy. Since it looks like this PR is still being discussed, I'm going to ProblemList this test in order to get the noise level back down. - PR Comment: https://git.openjdk.org/jdk/pull/17263#issuecomment-1877355597
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 22:51:47 GMT, Serguei Spitsyn wrote: >> This is a trivial fix for a regression caused by: >> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI >> ClassLoad event slows down vthread creation by factor 10 >> >> The fix of 8308614 just triggered a known issue: >> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events >> are not always posted with -Xcomp option >> >> The fix is just a work around with the extra checks with the >> `JvmtiExport::should_post_field_access()` and >> `JvmtiExport::should_post_field_modification()`. >> >> Testing: >> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail >> anymore with this fix >> - In progress: Test with tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: extended comment to cover the watchpoint extra checks Thumbs up. This is a trivial fix. You'll need to fix the whitespace complaint before integration. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16961#pullrequestreview-1766264031
Integrated: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Sat, 2 Dec 2023 17:15:57 GMT, Daniel D. Daugherty wrote: > In the fix for the following bug: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > JvmtiThreadState::state_for_while_locked() was changed to > return nullptr for attaching JNI threads regardless of whether > that JNI thread/JavaThread had a java.lang.Thread object. > > We should only filter out a JavaThread that's attaching via JNI > if it has no java.lang.Thread object. > > This fix has been tested with Mach5 Tier[1-7] and there are no related test > failures. > Mach5 Tier8 is in process. > > I'm going to need @jianglizhou to rerun her testing for: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > since the test(s) for that fix are not yet integrated in the jdk/jdk repo. This pull request has now been integrated. Changeset: 30b5d427 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/30b5d427350d03ec8b9eb39fbf06fbd1b1f66cd8 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 Reviewed-by: dholmes, jiangli, sspitsyn - PR: https://git.openjdk.org/jdk/pull/16934
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 19:01:05 GMT, Jiangli Zhou wrote: >> @dcubed-ojdk Thanks for the notification. I just ran one of our affected >> test 100 times with JDK-8312174 change rolled back and with both following >> applied: >> >> - https://git.openjdk.org/jdk/pull/16642 >> - https://github.com/openjdk/jdk/pull/16934 >> >> All 100 runs passed without failure. I'm going to run all tests tonight, >> will report back later tomorrow if I see any issue. > >> @jianglizhou - Thanks for doing the testing. Can you also do a review? We >> need two reviewers for this change. > > Complete test run finished. Checking the results, looks like there's no issue > related to JVMTIThreadState change. > > @dcubed-ojdk Reducing the check to `(thread->threadObj() == nullptr && > thread->is_attaching_via_jni())` looks ok. > > I just rechecked all usages of setup_jvmti_thread_state(). Currently it's > used in three cases: > - JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() > - JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > - JvmtiSampledObjectAllocEventCollector::start() > > JDK-8319935 ran into issue with > JvmtiSampledObjectAllocEventCollector::start() call path. We changed > JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample() to > avoid sampling if there is no thread obj allocated for the attaching thread. > We also changed JvmtiThreadState::state_for_while_locked to handle the > attaching case and return null. @dcubed-ojdk and @dholmes-ora, is there a > case JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > might also see an attaching thread without the thread obj allocated? > > JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() call path > probably is not affected by this case. @jianglizhou and @sspitsyn - Thanks for the reviews. In the interest of reducing the noise in the Mach5 CI, I'm going ahead with integrating this fix without waiting for a reply to my comment above. If there are remaining issues, then we'll deal with them in a follow-up bug/RFE. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839457963
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 18:13:50 GMT, Daniel D. Daugherty wrote: >> Initially I thought this was not the right fix as we should not be exposing >> an attaching thread that may still have a partially constructed `threadObj`. >> But this issue shows that we must expose such a thread because the >> constructor of the `Thread` object can trigger these events on the current >> thread so it must have a valid JVMTI state! >> >> Thanks. > > @dholmes-ora - Thanks for the review. > @dcubed-ojdk and @dholmes-ora, is there a case > JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > might also see an attaching thread without the thread obj allocated? JvmtiVMObjectAllocEventCollector is the code path that we ran into with the internal stress test, but in the case that tripped, we had a thread obj allocated. If we ran the same code path without the thread obj allocated, then we would return `nullptr` which was the goal of your original fix. I don't know if that specific test case is actually reached by any of our tests, but if such a case occurred, it did not result in the guarantee() firing or any other failure mode that I saw. The `guarantee(state != nullptr) failed: exiting thread called setup_jvmti_thread_state` has not been seen in Mach5 Tier[1-8] testing with this fix in place and I didn't see any other test failures that could be tracked back to problems with this code. A JvmtiVMObjectAllocEventCollector object is created in 34 places in jvm.cpp alone and I haven't checked all of those. I only checked out the one use in JVM_GetStackAccessControlContext. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839456136
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 06:31:43 GMT, David Holmes wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> dholmes CR - adjust a comment. > > Initially I thought this was not the right fix as we should not be exposing > an attaching thread that may still have a partially constructed `threadObj`. > But this issue shows that we must expose such a thread because the > constructor of the `Thread` object can trigger these events on the current > thread so it must have a valid JVMTI state! > > Thanks. @dholmes-ora - Thanks for the review. > src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 90: > >> 88: // Don't add a JvmtiThreadState to a thread that is exiting or is >> attaching. >> 89: // When a thread is attaching, it may not have a Java level thread >> object >> 90: // created yet. > > The comment needs adjusting now - suggestion: > > // Don't add a JvmtiThreadState to a thread that is exiting, or is attaching > // and does not yet have a Java level thread object allocated. Thanks for the suggested change. I agree and I've applied it. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839205636 PR Review Comment: https://git.openjdk.org/jdk/pull/16934#discussion_r1414306493
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 05:16:59 GMT, Jiangli Zhou wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > @dcubed-ojdk Thanks for the notification. I just ran one of our affected test > 100 times with JDK-8312174 change rolled back and with both following applied: > > - https://git.openjdk.org/jdk/pull/16642 > - https://github.com/openjdk/jdk/pull/16934 > > All 100 runs passed without failure. I'm going to run all tests tonight, will > report back later tomorrow if I see any issue. @jianglizhou - Thanks for doing the testing. Can you also do a review? We need two reviewers for this change. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839205099
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
> In the fix for the following bug: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > JvmtiThreadState::state_for_while_locked() was changed to > return nullptr for attaching JNI threads regardless of whether > that JNI thread/JavaThread had a java.lang.Thread object. > > We should only filter out a JavaThread that's attaching via JNI > if it has no java.lang.Thread object. > > This fix has been tested with Mach5 Tier[1-7] and there are no related test > failures. > Mach5 Tier8 is in process. > > I'm going to need @jianglizhou to rerun her testing for: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > since the test(s) for that fix are not yet integrated in the jdk/jdk repo. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: dholmes CR - adjust a comment. - Changes: - all: https://git.openjdk.org/jdk/pull/16934/files - new: https://git.openjdk.org/jdk/pull/16934/files/5c9eb2ec..0de1484f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16934&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16934&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16934.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16934/head:pull/16934 PR: https://git.openjdk.org/jdk/pull/16934
RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
In the fix for the following bug: [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread JvmtiThreadState::state_for_while_locked() was changed to return nullptr for attaching JNI threads regardless of whether that JNI thread/JavaThread had a java.lang.Thread object. We should only filter out a JavaThread that's attaching via JNI if it has no java.lang.Thread object. This fix has been tested with Mach5 Tier[1-7] and there are no related test failures. Mach5 Tier8 is in process. I'm going to need @jianglizhou to rerun her testing for: [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread since the test(s) for that fix are not yet integrated in the jdk/jdk repo. - Commit messages: - 8321069: Test failure after JDK-8319935: exiting thread called setup_jvmti_thread_state Changes: https://git.openjdk.org/jdk/pull/16934/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16934&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321069 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16934.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16934/head:pull/16934 PR: https://git.openjdk.org/jdk/pull/16934
Re: RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v5]
On Thu, 30 Nov 2023 21:27:27 GMT, Serguei Spitsyn wrote:
>> This is a fix of a performance/scalability related issue. The
>> `JvmtiThreadState` objects for virtual thread filtered events enabled
>> globally are created eagerly because it is needed when the
>> `interp_only_mode` is enabled. Otherwise, some events which are generated in
>> `interp_only_mode` from the debugging version of interpreter chunks can be
>> missed.
>> However, it has to be okay to avoid eager creation of these object if no
>> `interp_only_mode` has ever been requested.
>> It seems to be an extremely important optimization to create
>> JvmtiThreadState objects lazily in such cases.
>> It is done by introducing the flag
>> `JvmtiThreadState::_seen_interp_only_mode` which indicates when the
>> `JvmtiThreadState` objects have to be created eagerly.
>>
>> Additionally, the fix includes the following related changes:
>> - Use condition double checking idiom for `MutexLocker
>> mu(JvmtiThreadState_lock)` in the function
>> `JvmtiVTMSTransitionDisabler::VTMS_mount_end` which is on a
>> performance-critical path and looks like this:
>>
>> JvmtiThreadState* state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> MutexLocker mu(JvmtiThreadState_lock);
>> state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> JvmtiEventController::enter_interp_only_mode();
>> }
>> }
>>
>>
>> - Add extra check of `JvmtiExport::can_support_virtual_threads()` when
>> virtual thread mount and unmount are posted.
>> - Minor: Added a `ThreadsListHandle` to the
>> `JvmtiEventControllerPrivate::enter_interp_only_mode`. It is needed because
>> of the dynamic creation of compensating carrier threads which is racy for
>> JVMTI `SetEventNotificationMode` implementation.
>>
>> Performance mesurements:
>> - Without this fix the test provided by the bug submitter gives execution
>> numbers:
>> - no ClassLoad events enabled: 3251 ms
>> - ClassLoad events are enabled: 40534 ms
>>
>> - With the fix:
>> - no ClassLoad events enabled:3270 ms
>> - ClassLoad events are enabled: 3385 ms
>>
>> Testing:
>> - Ran mach5 tiers 1-6, no regressions are noticed
>
> Serguei Spitsyn has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - review: one more minor correction of a comment
> - review: minor correction of a comment
Thanks for fixing the comments.
-
Marked as reviewed by dcubed (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16686#pullrequestreview-1758619790
Re: RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v4]
On Thu, 30 Nov 2023 20:58:41 GMT, Serguei Spitsyn wrote: > It is a little bit ugly to do it for each call site. > The Handshake::execute() can do it instead, so its call sites could be > simplified. > BTW, it is done in the JvmtiHandshake::execute() and one can find it to be > convenient. Agreed, but it is intentional that direct uses of the three parameter version of `Handshake::execute()` with `tlh == nullptr` require that the caller understands the calling context. That means knowing where your ThreadsListHandle is and not getting away with not having one when the caller is current thread. Higher layer callers of `Handshake::execute()` like `JvmtiHandshake::execute()` or `JvmtiEventControllerPrivate::enter_interp_only_mode()` can make that less strict choice if they choose to. - PR Comment: https://git.openjdk.org/jdk/pull/16686#issuecomment-1834675329
Re: RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v4]
On Thu, 30 Nov 2023 02:08:39 GMT, Serguei Spitsyn wrote:
>> This is a fix of a performance/scalability related issue. The
>> `JvmtiThreadState` objects for virtual thread filtered events enabled
>> globally are created eagerly because it is needed when the
>> `interp_only_mode` is enabled. Otherwise, some events which are generated in
>> `interp_only_mode` from the debugging version of interpreter chunks can be
>> missed.
>> However, it has to be okay to avoid eager creation of these object if no
>> `interp_only_mode` has ever been requested.
>> It seems to be an extremely important optimization to create
>> JvmtiThreadState objects lazily in such cases.
>> It is done by introducing the flag
>> `JvmtiThreadState::_seen_interp_only_mode` which indicates when the
>> `JvmtiThreadState` objects have to be created eagerly.
>>
>> Additionally, the fix includes the following related changes:
>> - Use condition double checking idiom for `MutexLocker
>> mu(JvmtiThreadState_lock)` in the function
>> `JvmtiVTMSTransitionDisabler::VTMS_mount_end` which is on a
>> performance-critical path and looks like this:
>>
>> JvmtiThreadState* state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> MutexLocker mu(JvmtiThreadState_lock);
>> state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> JvmtiEventController::enter_interp_only_mode();
>> }
>> }
>>
>>
>> - Add extra check of `JvmtiExport::can_support_virtual_threads()` when
>> virtual thread mount and unmount are posted.
>> - Minor: Added a `ThreadsListHandle` to the
>> `JvmtiEventControllerPrivate::enter_interp_only_mode`. It is needed because
>> of the dynamic creation of compensating carrier threads which is racy for
>> JVMTI `SetEventNotificationMode` implementation.
>>
>> Performance mesurements:
>> - Without this fix the test provided by the bug submitter gives execution
>> numbers:
>> - no ClassLoad events enabled: 3251 ms
>> - ClassLoad events are enabled: 40534 ms
>>
>> - With the fix:
>> - no ClassLoad events enabled:3270 ms
>> - ClassLoad events are enabled: 3385 ms
>>
>> Testing:
>> - Ran mach5 tiers 1-6, no regressions are noticed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: remove newly added ThreadsListHandle from enter_interp_only_mode
Changes requested by dcubed (Reviewer).
src/hotspot/share/prims/jvmtiThreadState.cpp line 530:
> 528: assert(!thread->is_in_tmp_VTMS_transition(), "sanity check");
> 529:
> 530: // If interp_only_mode is enabled then we must eagerly create
> JvmtiThreadState
typo: s/is enabled/has been enabled/
src/hotspot/share/prims/jvmtiThreadState.cpp line 536:
> 534: JvmtiEventController::thread_started(thread);
> 535: }
> 536: if (JvmtiExport::should_post_vthread_start()) {
Why is this check: `if (JvmtiExport::can_support_virtual_threads()) {` removed?
src/hotspot/share/prims/jvmtiThreadState.cpp line 552:
> 550: JvmtiExport::post_vthread_unmount(vthread);
> 551: }
> 552: if (JvmtiExport::can_support_virtual_threads()) {
Why is this check: if (JvmtiExport::can_support_virtual_threads()) { removed?
src/hotspot/share/prims/jvmtiThreadState.hpp line 234:
> 232: inline void set_head_env_thread_state(JvmtiEnvThreadState* ets);
> 233:
> 234: static bool _seen_interp_only_mode; // interp_only_mode was requested
> once
perhaps: s/requested once/requested at least once/
-
PR Review: https://git.openjdk.org/jdk/pull/16686#pullrequestreview-1758088444
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1411066065
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1411051100
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1411051508
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1411059699
Re: RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v4]
On Thu, 30 Nov 2023 02:08:39 GMT, Serguei Spitsyn wrote:
>> This is a fix of a performance/scalability related issue. The
>> `JvmtiThreadState` objects for virtual thread filtered events enabled
>> globally are created eagerly because it is needed when the
>> `interp_only_mode` is enabled. Otherwise, some events which are generated in
>> `interp_only_mode` from the debugging version of interpreter chunks can be
>> missed.
>> However, it has to be okay to avoid eager creation of these object if no
>> `interp_only_mode` has ever been requested.
>> It seems to be an extremely important optimization to create
>> JvmtiThreadState objects lazily in such cases.
>> It is done by introducing the flag
>> `JvmtiThreadState::_seen_interp_only_mode` which indicates when the
>> `JvmtiThreadState` objects have to be created eagerly.
>>
>> Additionally, the fix includes the following related changes:
>> - Use condition double checking idiom for `MutexLocker
>> mu(JvmtiThreadState_lock)` in the function
>> `JvmtiVTMSTransitionDisabler::VTMS_mount_end` which is on a
>> performance-critical path and looks like this:
>>
>> JvmtiThreadState* state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> MutexLocker mu(JvmtiThreadState_lock);
>> state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> JvmtiEventController::enter_interp_only_mode();
>> }
>> }
>>
>>
>> - Add extra check of `JvmtiExport::can_support_virtual_threads()` when
>> virtual thread mount and unmount are posted.
>> - Minor: Added a `ThreadsListHandle` to the
>> `JvmtiEventControllerPrivate::enter_interp_only_mode`. It is needed because
>> of the dynamic creation of compensating carrier threads which is racy for
>> JVMTI `SetEventNotificationMode` implementation.
>>
>> Performance mesurements:
>> - Without this fix the test provided by the bug submitter gives execution
>> numbers:
>> - no ClassLoad events enabled: 3251 ms
>> - ClassLoad events are enabled: 40534 ms
>>
>> - With the fix:
>> - no ClassLoad events enabled:3270 ms
>> - ClassLoad events are enabled: 3385 ms
>>
>> Testing:
>> - Ran mach5 tiers 1-6, no regressions are noticed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: remove newly added ThreadsListHandle from enter_interp_only_mode
Removing the TLH is the right thing to do. If we get that failure mode again,
then we
can file a new bug and hopefully have an hs_err_pid file with it.
I don't think we should change the guarantee() in `Handshake::execute()`. When
the
three parameter version of `execute()` is called with `tlh == nullptr`, the
caller is
saying that there is supposed to be a ThreadsListHandle in the calling context.
Yes,
if the target thread is the calling thread, then a ThreadsListHandle is not
needed,
but that's why we have this code to prevent the call to `Handshake::execute()`:
if (target->is_handshake_safe_for(current)) {
hs.do_thread(target);
In other words, I think `Handshake::execute()` is working the way it is
supposed to
when `tlh == nullptr` is passed.
-
PR Comment: https://git.openjdk.org/jdk/pull/16686#issuecomment-1834244635
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v5]
On Wed, 22 Nov 2023 22:40:20 GMT, Jiangli Zhou wrote: >> Please review JvmtiThreadState::state_for_while_locked change to handle the >> state->get_thread_oop() null case. Please see >> https://bugs.openjdk.org/browse/JDK-8319935 for details. > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Address Serguei Spitsyn's comments/suggestions: > - Remove the redundant thread->is_Java_thread() check from > JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample(). > - Change the assert in JvmtiThreadState::state_for_while_locked to avoid > #ifdef ASSERT. A belated thumbs up. Sorry I didn't get back to this review before the fix was integrated. I found just a nit comment that could be more clear. src/hotspot/share/prims/jvmtiExport.cpp line 3143: > 3141: > 3142: // If the current thread is attaching from native and its Java thread > object > 3143: // is being allocated, things are not ready for allocation sampling. nit - typo: s/is being allocated/has not been allocated/ - PR Review: https://git.openjdk.org/jdk/pull/16642#pullrequestreview-1756369261 PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1409970009
Re: RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v3]
On Fri, 17 Nov 2023 20:29:11 GMT, Serguei Spitsyn wrote:
>> This is a fix of a performance/scalability related issue. The
>> `JvmtiThreadState` objects for virtual thread filtered events enabled
>> globally are created eagerly because it is needed when the
>> `interp_only_mode` is enabled. Otherwise, some events which are generated in
>> `interp_only_mode` from the debugging version of interpreter chunks can be
>> missed.
>> However, it has to be okay to avoid eager creation of these object if no
>> `interp_only_mode` has ever been requested.
>> It seems to be an extremely important optimization to create
>> JvmtiThreadState objects lazily in such cases.
>> It is done by introducing the flag
>> `JvmtiThreadState::_seen_interp_only_mode` which indicates when the
>> `JvmtiThreadState` objects have to be created eagerly.
>>
>> Additionally, the fix includes the following related changes:
>> - Use condition double checking idiom for `MutexLocker
>> mu(JvmtiThreadState_lock)` in the function
>> `JvmtiVTMSTransitionDisabler::VTMS_mount_end` which is on a
>> performance-critical path and looks like this:
>>
>> JvmtiThreadState* state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> MutexLocker mu(JvmtiThreadState_lock);
>> state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> JvmtiEventController::enter_interp_only_mode();
>> }
>> }
>>
>>
>> - Add extra check of `JvmtiExport::can_support_virtual_threads()` when
>> virtual thread mount and unmount are posted.
>> - Minor: Added a `ThreadsListHandle` to the
>> `JvmtiEventControllerPrivate::enter_interp_only_mode`. It is needed because
>> of the dynamic creation of compensating carrier threads which is racy for
>> JVMTI `SetEventNotificationMode` implementation.
>>
>> Performance mesurements:
>> - Without this fix the test provided by the bug submitter gives execution
>> numbers:
>> - no ClassLoad events enabled: 3251 ms
>> - ClassLoad events are enabled: 40534 ms
>>
>> - With the fix:
>> - no ClassLoad events enabled:3270 ms
>> - ClassLoad events are enabled: 3385 ms
>>
>> Testing:
>> - Ran mach5 tiers 1-6, no regressions are noticed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: add comment for new ThreadsListHandle use
I'm going to resurrect the failing guarantee() code and part of the stack trace
that was removed
and yack a bit about this code path.
Here's the location of the failing guarantee():
void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh,
JavaThread* target) {
. . .
guarantee(target != nullptr, "must be");
if (tlh == nullptr) {
guarantee(Thread::is_JavaThread_protected_by_TLH(target),
"missing ThreadsListHandle in calling context.");
and here's part of the stack trace that got us here:
V [libjvm.so+0x117937d]
JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState*)+0x45d
(jvmtiEventController.cpp:402)
V [libjvm.so+0x1179520]
JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState*) [clone
.part.0]+0x190 (jvmtiEventController.cpp:632)
V [libjvm.so+0x117a1e1]
JvmtiEventControllerPrivate::thread_started(JavaThread*)+0x351
(jvmtiEventController.cpp:1174)
V [libjvm.so+0x117e608] JvmtiExport::get_jvmti_thread_state(JavaThread*)+0x98
(jvmtiExport.cpp:424)
V [libjvm.so+0x118a86c] JvmtiExport::post_field_access(JavaThread*, Method*,
unsigned char*, Klass*, Handle, _jfieldID*)+0x6c (jvmtiExport.cpp:2214)
This must have been a stack trace from a build with some optimizations enabled
because
when I look at last night's code base, I see 8 frames from the
JvmtiExport::get_jvmti_thread_state()
call to Handshake::execute() with three params:
src/hotspot/share/prims/jvmtiExport.cpp:
JvmtiExport::get_jvmti_thread_state(JavaThread *thread) {
assert(thread == JavaThread::current(), "must be current thread");
if (thread->is_vthread_mounted() && thread->jvmti_thread_state() ==
nullptr) {
JvmtiEventController::thread_started(thread);
}
The above code asserts that the `thread` parameter is the current thread so
we know that the calling thread is operating on itself.
src/hotspot/share/prims/jvmtiEventController.cpp
JvmtiEventControllerPrivate::thread_started(JavaThread *thread) {
assert(thread == Thread::current(), "must be current thread");
// if we have any thread filtered events globally enabled, create/update
the thread state
if (is_any_thread_filtered_event_enabled_globally()) { // intentionally
racy
JvmtiThreadState::state_for(thread);
The above code asserts that the `thread` parameter is the current thread so
we know that the calling thread is operating on itself. Note that we're calling
the single parameter version of `JvmtiThr
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v9]
On Wed, 29 Nov 2023 06:38:51 GMT, Stefan Karlsson wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Thumbs up. Thanks for making the additional changes. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16783#pullrequestreview-1755779119
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Mon, 27 Nov 2023 20:20:01 GMT, Stefan Karlsson wrote: >> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation I'll re-review again once the last set of comments are addressed. - PR Comment: https://git.openjdk.org/jdk/pull/16783#issuecomment-1830463592
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Thu, 23 Nov 2023 11:21:12 GMT, Stefan Karlsson wrote: >> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java >> line 61: >> >>> 59: jniMonitorEnter(obj); >>> 60: obj = null; >>> 61: System.gc(); >> >> Again one gc() is generally not sufficient. >> >> How can this test tell that the object in the monitor was actually cleared? >> I think `monitorinflation` logging may be the only way to tell. > > Yes, probably. I've been looking at the `monitorinflation` logging to very > that it gets cleared. I think it would be messy to try to get this test to > also start to parse logs. There are other tests that try to make sure that some specific object is GC'ed. If the lack of the object being collected will cause the test to fail, then look else where for a more reliable mechanism. - PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406869425
Re: RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]
On Mon, 27 Nov 2023 20:20:01 GMT, Stefan Karlsson wrote:
>> In the rewrites made for:
>> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump
>> asserts in interleaved ObjectMonitor::deflate_monitor calls`
>>
>> I removed the filtering of *owned ObjectMonitors with dead objects*. The
>> reasoning was that you should never have an owned ObjectMonitor with a dead
>> object. I added an assert to check this assumption. It turns out that the
>> assumption was wrong *if* you use JNI to call MonitorEnter and then remove
>> all references to the locked object.
>>
>> The provided tests provoke this assert form:
>> * the JNI thread detach code
>> * thread dumping with locked monitors, and
>> * the JVMTI GetOwnedMonitorInfo API.
>>
>> While investigating this we've found that the thread detach code becomes
>> more correct when this filter was removed. Previously, the locked monitors
>> never got unlocked because the ObjectMonitor iterator never exposed these
>> monitors to the JNI detach code that unlocks the thread's monitors. That bug
>> caused an ObjectMonitor leak. So, for this case I'm leaving these
>> ObjectMonitors unfiltered so that we don't reintroduce the leak.
>>
>> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so
>> I'm filtering those in the closure that collects ObjectMonitor. Side note:
>> We have discussions about ways to completely rewrite this by letting each
>> thread have thread-local information about JNI held locks. If we have this
>> we could probably throw away the entire ObjectMonitorDump hashtable, and its
>> walk of the `_in_use_list.`.
>>
>> For GetOwnedMonitorInfo it is unclear if we should expose these weird
>> ObjectMonitor. If we do, then the users can detect that a thread holds a
>> lock with a dead object, and the code will return NULL as one of the "owned
>> monitors" returned. I don't think that's a good idea, so I'm filtering out
>> these ObjectMonitor for those calls.
>>
>> Test: the written tests with and without the fix. Tier1-Tier3, so far.
>
> Stefan Karlsson has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix indentation
I've made a first pass over this PR. I don't think have anything
that's a "must do". I'll make another pass tomorrow after I have
a chance to think about this fix.
test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights
> reserved.
nit: why include 2022 in the copyright header?
test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 2:
> 1: /*
> 2: * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights
> reserved.
nit: why include 2022 in the copyright header?
test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 28:
> 26: #include
> 27: #include
> 28: #include
Should these be in sort order?
test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 110:
> 108:
> 109: // Let the GC clear the weak reference to the object.
> 110: system_gc(env);
A single GC may not be enough...
test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 121:
> 119: create_monitor_with_dead_object(env);
> 120:
> 121: // DetachCurrenThread will try to unlock held monitors. This has been a
nit typo: s/DetachCurrenThread/DetachCurrentThread/
test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 131:
> 129: if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK)
> die("DetachCurrentThread");
> 130:
> 131: return NULL;
Why is this function return type "void*" when it only returns NULL?
test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 149:
> 147: if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK)
> die("DetachCurrentThread");
> 148:
> 149: return NULL;
Why is this function return type "void*" when it only returns NULL?
-
PR Review: https://git.openjdk.org/jdk/pull/16783#pullrequestreview-1751551279
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406858803
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406861430
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406862006
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406864625
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406864964
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406866330
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406866618
Re: RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v3]
On Sat, 18 Nov 2023 12:22:10 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/prims/jvmtiEventController.cpp line 374: >> >>> 372: // Protects any existing JavaThread's from being terminated while it >>> is set. >>> 373: // The FJP carrier thread compensating mechanism can create carrier >>> threads concurrently. >>> 374: ThreadsListHandle tlh(current); >> >> A ThreadsListHandle does not prevent a JavaThread from being terminated. It >> prevents a JavaThread from exiting and being freed. The JavaThread is able to >> set the terminated state on itself, but will not be able to complete exiting >> while >> it is on a ThreadsListHandle. There is a subtle difference. >> >> There's a `target` JavaThread that is fetched from a `JvmtiThreadState` >> object >> and that `target` JavaThread is only protected by this `tlh` if `target` is >> included >> in the ThreadsList that was captured by this `tlh`. In all likelihood, there >> should be >> a ThreadsListHandle farther up the stack that's protecting the JavaThread >> from >> which the `JvmtiThreadState` object was extracted and passed to this >> function. >> >> As for carrier threads, if they are created _after_ this `tlh` was created, >> then this >> `tlh` cannot protect them because they won't be on this `tlh`'s ThreadsList. > > Thank you for the comment, Dan! > Agreed, the comment needs to be corrected in two aspects. > I tried to simplify it but failed to do it correctly. > It is interesting that there is a `ThreadsListHandle` farther up the stack > but it does not help sometimes. > It is observed that a `JavaThread` (of a carrier thread) referenced from the > `JvmtiThreadState` object can be just created, so we need a > `ThreadsListHandle` to avoid possible asserts. With this fix in place I do > not see the asserts fired anymore. @sspitsyn - Please point me at the code where these JavaThreads are newly created? - PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1398317396
Re: RFR: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 [v3]
On Fri, 17 Nov 2023 20:29:11 GMT, Serguei Spitsyn wrote:
>> This is a fix of a performance/scalability related issue. The
>> `JvmtiThreadState` objects for virtual thread filtered events enabled
>> globally are created eagerly because it is needed when the
>> `interp_only_mode` is enabled. Otherwise, some events which are generated in
>> `interp_only_mode` from the debugging version of interpreter chunks can be
>> missed.
>> However, it has to be okay to avoid eager creation of these object if no
>> `interp_only_mode` has ever been requested.
>> It seems to be an extremely important optimization to create
>> JvmtiThreadState objects lazily in such cases.
>> It is done by introducing the flag
>> `JvmtiThreadState::_seen_interp_only_mode` which indicates when the
>> `JvmtiThreadState` objects have to be created eagerly.
>>
>> Additionally, the fix includes the following related changes:
>> - Use condition double checking idiom for `MutexLocker
>> mu(JvmtiThreadState_lock)` in the function
>> `JvmtiVTMSTransitionDisabler::VTMS_mount_end` which is on a
>> performance-critical path and looks like this:
>>
>> JvmtiThreadState* state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> MutexLocker mu(JvmtiThreadState_lock);
>> state = thread->jvmti_thread_state();
>> if (state != nullptr && state->is_pending_interp_only_mode()) {
>> JvmtiEventController::enter_interp_only_mode();
>> }
>> }
>>
>>
>> - Add extra check of `JvmtiExport::can_support_virtual_threads()` when
>> virtual thread mount and unmount are posted.
>> - Minor: Added a `ThreadsListHandle` to the
>> `JvmtiEventControllerPrivate::enter_interp_only_mode`. It is needed because
>> of the dynamic creation of compensating carrier threads which is racy for
>> JVMTI `SetEventNotificationMode` implementation.
>>
>> Performance mesurements:
>> - Without this fix the test provided by the bug submitter gives execution
>> numbers:
>> - no ClassLoad events enabled: 3251 ms
>> - ClassLoad events are enabled: 40534 ms
>>
>> - With the fix:
>> - no ClassLoad events enabled:3270 ms
>> - ClassLoad events are enabled: 3385 ms
>>
>> Testing:
>> - Ran mach5 tiers 1-6, no regressions are noticed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: add comment for new ThreadsListHandle use
src/hotspot/share/prims/jvmtiEventController.cpp line 374:
> 372: // Protects any existing JavaThread's from being terminated while it
> is set.
> 373: // The FJP carrier thread compensating mechanism can create carrier
> threads concurrently.
> 374: ThreadsListHandle tlh(current);
A ThreadsListHandle does not prevent a JavaThread from being terminated. It
prevents a JavaThread from exiting and being freed. The JavaThread is able to
set the terminated state on itself, but will not be able to complete exiting
while
it is on a ThreadsListHandle. There is a subtle difference.
There's a `target` JavaThread that is fetched from a `JvmtiThreadState` object
and that `target` JavaThread is only protected by this `tlh` if `target` is
included
in the ThreadsList that was captured by this `tlh`. In all likelihood, there
should be
a ThreadsListHandle farther up the stack that's protecting the JavaThread from
which the `JvmtiThreadState` object was extracted and passed to this function.
As for carrier threads, if they are created _after_ this `tlh` was created,
then this
`tlh` cannot protect them because they won't be on this `tlh`'s ThreadsList.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16686#discussion_r1397897633
Re: RFR: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks [v2]
On Tue, 14 Nov 2023 08:18:41 GMT, Roman Marchenko wrote: >> Zero'ing memory of extension event callbacks > > Roman Marchenko has updated the pull request incrementally with one > additional commit since the last revision: > > Fixing review comments Sure. Here's the relevant sub-section from the "OpenJDK Developers’ Guide": https://openjdk.org/guide/index.html#life-of-a-pr Get the required reviews At least one Reviewer knowledgeable in each area being changed must approve every change. Some areas (e.g. Client and HotSpot) require two reviewers in most cases, so be sure to read the relevant OpenJDK group pages for advice or ask your sponsor. Be open to comments and polite in replies. Remember that the reviewer wants to improve the world just as much as you do, only in a slightly different way. If you don’t understand some comment, ask the reviewer to clarify. Accept authority when applicable. If you’re making changes in an area where you’re not the area expert, acknowledge that your reviewers may be. Take their advice seriously, even if it is to not make the change. There are many reasons [why a change may get rejected](https://openjdk.org/guide/index.html#why-is-my-change-rejected). And you did read the section [Things to consider before changing OpenJDK code], right? - PR Comment: https://git.openjdk.org/jdk/pull/16647#issuecomment-1813094911
Re: RFR: 8320130: Problemlist 2 vmTestbase/nsk/jdi/StepRequest/addClassFilter_rt tests with Xcomp
On Wed, 15 Nov 2023 02:24:17 GMT, Alex Menkov wrote: > ProblemList 2 nsk jdi tests with Xcomp on all platforms Thumbs up. This is a trivial fix. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16667#pullrequestreview-1732508536
Re: RFR: 8319961: JvmtiEnvBase doesn't zero _ext_event_callbacks [v2]
On Tue, 14 Nov 2023 08:18:41 GMT, Roman Marchenko wrote: >> Zero'ing memory of extension event callbacks > > Roman Marchenko has updated the pull request incrementally with one > additional commit since the last revision: > > Fixing review comments Doing a post integration review. This is a trivial fix and does not need a second review nor wait 24 hours. Just a heads up that HotSpot code normally requires two reviews (1 from a (R)eviewer) and 24 hours unless it is called trivial AND agreed to be trivial by your reviewers. - PR Review: https://git.openjdk.org/jdk/pull/16647#pullrequestreview-1730761590 PR Comment: https://git.openjdk.org/jdk/pull/16647#issuecomment-1811258657
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread
On Mon, 13 Nov 2023 21:52:22 GMT, Jiangli Zhou wrote: > Please review JvmtiThreadState::state_for_while_locked change to handle the > state->get_thread_oop() null case. Please see > https://bugs.openjdk.org/browse/JDK-8319935 for details. @jianglizhou - I fixed a typo in the bug's synopsis line. Change this PR's title: s/is create/is created/ - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1809221878
