Re: RFR: 8339871: serviceability/sa/TestDebugInfoDecode.java should be driver [v2]
On Wed, 16 Oct 2024 10:30:48 GMT, Ramkumar Sunderbabu wrote: >> Passing "-Xmx1g -Xcomp" to the LingeredApp. >> Testing: tier1 > > Ramkumar Sunderbabu has updated the pull request incrementally with one > additional commit since the last revision: > > change othervm to driver, remove -Xmx1g Please check that test pass with options from CI. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21519#pullrequestreview-2373050348
Re: RFR: 8339871: serviceability/sa/TestDebugInfoDecode.java should be driver
On Tue, 15 Oct 2024 10:12:21 GMT, Ramkumar Sunderbabu wrote: > Passing "-Xmx1g -Xcomp" to the LingeredApp. > Testing: tier1 The only thing that is missed it change `othervm` to `driver`. The adding -Xcomp in LingeredApp.startApp(theApp, "-Xmx1g", "-Xcomp"); should works fine except when -Xint/-Xmixed is set explicitly. So generally test like needs to have @requires vm.compMode == "null" | vm.compMode == "Xcomp" However we don't set these modes, so it might be skipped. A lot of our test set Xcomp without any requires now. Also, the vm.flagless is not required, since test support various vm flags. Limiting tests to run only -Xcomp is set is also not needed for this test. The another thing to check (unrelated to the fix) is if test is executed in tier1 and should it be there. - PR Comment: https://git.openjdk.org/jdk/pull/21519#issuecomment-2415309978
Re: RFR: 8341588: Remove CollectionUsageThreshold.java from ProblemList-Xcomp for debugging [v3]
On Tue, 8 Oct 2024 04:17:31 GMT, Ramkumar Sunderbabu wrote: >> JDK-8318668 was not reproducible in repeated runs. Hence, I am pulling it >> out of problem listing. Additionally I have increased logging so that it is >> easier to debug when the issue happens again. >> >> Testing: >> tier1,tier2,tier3 >> tier6-comp,tier8-comp > > Ramkumar Sunderbabu has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains one commit: > > commit after resolving conflicts Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21392#pullrequestreview-2353346908
Re: RFR: 8341588: Remove CollectionUsageThreshold.java from ProblemList-Xcomp for debugging [v2]
On Tue, 8 Oct 2024 01:16:46 GMT, Ramkumar Sunderbabu wrote: >> JDK-8318668 was not reproducible in repeated runs. Hence, I am pulling it >> out of problem listing. Additionally I have increased logging so that it is >> easier to debug when the issue happens again. >> >> Testing: >> tier1,tier2,tier3 >> tier6-comp,tier8-comp > > Ramkumar Sunderbabu has updated the pull request incrementally with one > additional commit since the last revision: > > resolving merge conflict Changes requested by lmesnik (Reviewer). test/jdk/ProblemList-Xcomp.txt line 31: > 29: > 30: java/lang/invoke/MethodHandles/CatchExceptionTest.java 8146623 > generic-all > 31: java/foreign/TestUpcallStress.java 8341584 > generic-all This looks odd, this line shouldn't be added with your PR. It makes history of commits wrong. Please check if it was correctly merged. - PR Review: https://git.openjdk.org/jdk/pull/21392#pullrequestreview-2353223223 PR Review Comment: https://git.openjdk.org/jdk/pull/21392#discussion_r1791080538
Re: RFR: 8341273: JVMTI is not properly hiding some continuation related methods
On Mon, 7 Oct 2024 22:03:36 GMT, Serguei Spitsyn wrote:
> This fixes a problem in the VTMS (Virtual Thread Mount State) transition
> frames hiding mechanism.
> Please, see a fix description in the first comment.
>
> Testing:
> - Verified with new test `vthread/CheckHiddenFrames`
> - Mach5 tiers 1-6 are passed
Changes requested by lmesnik (Reviewer).
src/hotspot/share/prims/jvmtiEnvBase.cpp line 691:
> 689:
> 690: if (is_virtual || jt->is_in_VTMS_transition()) { // filter out pure
> continuations
> 691: jvf = check_and_skip_hidden_frames(jt->is_in_VTMS_transition(), jvf);
Wouldn't be easier to split method `check_and_skip_hidden_frames` to
skip_yeilds and skip_transition frames?
BTW Also it is unclear shouldn't the `@Hidden` methods be skipped from all
jvmti frame streams?
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2009:
> 2007: bool is_virtual = java_lang_VirtualThread::is_instance(thread_oop);
> 2008:
> 2009: // Target can be virtual or platform thread.
Can you please explain in comment why is it needed to disable all threads for
platform target thread.
test/hotspot/jtreg/serviceability/jvmti/vthread/CheckHiddenFrames/CheckHiddenFrames.java
line 25:
> 23:
> 24: /*
> 25: * @test id=virtual
Having 'id=virtual' not needed and might confuse people. They expect to have
other test variations for platform.
test/hotspot/jtreg/serviceability/jvmti/vthread/CheckHiddenFrames/CheckHiddenFrames.java
line 32:
> 30:
> 31: public class CheckHiddenFrames {
> 32: private static final String AGENT_LIB = "CheckHiddenFrames";
It is not used?
test/hotspot/jtreg/serviceability/jvmti/vthread/CheckHiddenFrames/CheckHiddenFrames.java
line 43:
> 41:
> 42: public static void main(String[] args) throws Exception {
> 43: Thread thread =
> Thread.ofVirtual().unstarted(CheckHiddenFrames::test);
You can use
startVirtualThread
to save one line.
-
PR Review: https://git.openjdk.org/jdk/pull/21397#pullrequestreview-2353060666
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1791023030
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1791008060
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1790967726
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1790968113
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1790966876
Re: RFR: 8334305: Remove all code for nsk.share.Log verbose mode
On Mon, 30 Sep 2024 14:55:33 GMT, Ramkumar Sunderbabu wrote: > Cleaning up nsk.share.Log code after the verbose mode was set always true. > > Tested all the vmTestbase/ tests. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21267#pullrequestreview-2337864195
Re: RFR: 8340276: Test java/lang/management/ThreadMXBean/Locks.java failed with NullPointerException
On Wed, 18 Sep 2024 11:04:32 GMT, Kevin Walls wrote: > One more place in this test where it iterates a list of ThreadInfo and should > be skipping nulls, representing threads that may have exited, or be virtual, > or not exist. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21056#pullrequestreview-2313780917
Re: RFR: 8340113: Remove JULONG as a Diagnostic Command argument type (jcmd JFR.view) [v4]
On Tue, 17 Sep 2024 14:32:22 GMT, Kevin Walls wrote: >> The few uses of the operation parameter type "JULONG" in Diagnostic Commands >> should be changed to INT. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > No comma in: INT followed by... Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21021#pullrequestreview-2311339024
Integrated: 8340213: jcmd VM.events ignores max argument
On Mon, 16 Sep 2024 19:14:01 GMT, Leonid Mesnik wrote: > The inner 'int max;' declaration hide previous max. This pull request has now been integrated. Changeset: 202fd421 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/202fd421f7e8b0f4a9c7393d1045e879acd13e64 Stats: 24 lines in 2 files changed: 22 ins; 1 del; 1 mod 8340213: jcmd VM.events ignores max argument Reviewed-by: szaldana, cjplummer, amenkov, mli - PR: https://git.openjdk.org/jdk/pull/21024
Re: RFR: 8340213: jcmd VM.events ignores max argument [v2]
On Mon, 16 Sep 2024 19:54:38 GMT, Leonid Mesnik wrote: >> The inner 'int max;' declaration hide previous max. > > Leonid Mesnik has updated the pull request incrementally with two additional > commits since the last revision: > > - typo fixed. > - check added. I filed separate https://bugs.openjdk.org/browse/JDK-8340334 for changing type. So any users of jcmd can track it, and to separate the backports. - PR Comment: https://git.openjdk.org/jdk/pull/21024#issuecomment-2357050757
Re: RFR: 8340213: jcmd VM.events ignores max argument [v2]
On Mon, 16 Sep 2024 19:34:40 GMT, Sonia Zaldana Calles
wrote:
>> Leonid Mesnik has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - typo fixed.
>> - check added.
>
> test/hotspot/jtreg/serviceability/dcmd/vm/EventsTest.java line 81:
>
>> 79: long lines = output.asLines().stream().filter(x ->
>> x.contains("Loading class")).count();
>> 80: Assert.assertTrue(lines == MAX, "There are should be " + MAX + "
>> lines");
>> 81: output.stdoutShouldNotMatch(buildHeaderPattern("Events"));
>
> Could also check the output contains the selected event category like in
> `run_selected` above.
Thanks! done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21024#discussion_r1761789660
Re: RFR: 8340213: jcmd VM.events ignores max argument [v2]
> The inner 'int max;' declaration hide previous max. Leonid Mesnik has updated the pull request incrementally with two additional commits since the last revision: - typo fixed. - check added. - Changes: - all: https://git.openjdk.org/jdk/pull/21024/files - new: https://git.openjdk.org/jdk/pull/21024/files/669efa83..b655cfb2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21024&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21024&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21024.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21024/head:pull/21024 PR: https://git.openjdk.org/jdk/pull/21024
RFR: 8340213: jcmd VM.events ignores max argument
The inner 'int max;' declaration hide previous max. - Commit messages: - fixed typo - 8340213: jcmd VM.events ignores max argument Changes: https://git.openjdk.org/jdk/pull/21024/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21024&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8340213 Stats: 23 lines in 2 files changed: 21 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21024.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21024/head:pull/21024 PR: https://git.openjdk.org/jdk/pull/21024
Re: RFR: 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java
On Mon, 16 Sep 2024 09:21:22 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which replaces the usage > of `-noclassgc` with `-Xnoclassgc` option when launching the test? > > As noted in https://bugs.openjdk.org/browse/JDK-8340176, the `-noclassgc` is > an undocumented option of the `java` launcher, which it converts to > `-Xnoclassgc` before passing it to the JVM. Instead of that option, the > documented `-Xnoclassgc` should be used. > > No new test has been introduced and the modified test continues to pass after > this change. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21012#pullrequestreview-2306989340
Re: RFR: 8310525: DynamicLauncher for JDP test needs to try harder to find a free port
On Fri, 13 Sep 2024 09:47:07 GMT, Kevin Walls wrote: > JDP tests only make 3 attempts to find a free port, using getFreePort(). > We know getFreePort() is racy and you need to make sure the port really is > fee when trying to use it. > > Make 10 attempts. Leave the logic unchanged. Also fix a typo in another > test. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20991#pullrequestreview-2306969278
Re: RFR: 8339801: Add better test failure diagnostics to vmTestbase/nsk/jdi/EventRequestManager/threadStartRequests/thrstartreq002
On Tue, 10 Sep 2024 00:27:23 GMT, Chris Plummer wrote: > This test fails periodically. It only prints the exception message when it > fails. It should print the entire stack trace so we have a better idea of why > it is failing. > > Testing: This test is not run until tier5 CI testing, so I'm running all > tier5 svc tests. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20927#pullrequestreview-2294129724
Integrated: 8339638: Update vmTestbase/nsk/jvmti/*Field*Watch tests to use virtual thread factory
On Fri, 6 Sep 2024 18:56:32 GMT, Leonid Mesnik wrote: > Field watch tests updated to be executed with virtual threads when virtual > test thread factory enabled.I added > jdk.test.lib.thread.TestThreadFactory > to created test threads so they support virtual thread factory jtreg plugin. > > It might makes sense to use it in different tests so virtual threads are > tests more extensively. > There is JDIThreadFactory in nsk/sharejdi which I copied. I am going to > update JDI tests to use jdk.test.lib.thread.TestThreadFactory > later. This pull request has now been integrated. Changeset: 9785e19f Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/9785e19f3f87306cabc26a862d35b89d41cfef93 Stats: 75 lines in 5 files changed: 63 ins; 0 del; 12 mod 8339638: Update vmTestbase/nsk/jvmti/*Field*Watch tests to use virtual thread factory Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/20897
Re: RFR: 8339638: Update vmTestbase/nsk/jvmti/*Field*Watch tests to use virtual thread factory
On Fri, 6 Sep 2024 18:56:32 GMT, Leonid Mesnik wrote: > Field watch tests updated to be executed with virtual threads when virtual > test thread factory enabled.I added > jdk.test.lib.thread.TestThreadFactory > to created test threads so they support virtual thread factory jtreg plugin. > > It might makes sense to use it in different tests so virtual threads are > tests more extensively. > There is JDIThreadFactory in nsk/sharejdi which I copied. I am going to > update JDI tests to use jdk.test.lib.thread.TestThreadFactory > later. Thank you for review. - PR Comment: https://git.openjdk.org/jdk/pull/20897#issuecomment-2342060595
Re: RFR: 8339638: Update vmTestbase/nsk/jvmti/*Field*Watch tests to use virtual thread factory
On Fri, 6 Sep 2024 19:52:19 GMT, Chris Plummer wrote: >> Field watch tests updated to be executed with virtual threads when virtual >> test thread factory enabled.I added >> jdk.test.lib.thread.TestThreadFactory >> to created test threads so they support virtual thread factory jtreg plugin. >> >> It might makes sense to use it in different tests so virtual threads are >> tests more extensively. >> There is JDIThreadFactory in nsk/sharejdi which I copied. I am going to >> update JDI tests to use jdk.test.lib.thread.TestThreadFactory >> later. > > test/lib/jdk/test/lib/thread/TestThreadFactory.java line 2: > >> 1: /* >> 2: * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights >> reserved. > > Should just be 2024. I copied file from JDIThreadFactory,java (I mention this in PR) and I left copyrights from original. I think it is the correct way to treat copyrights. Not sure, if I can find policy about this though. If you think that new file require current year only, I am fine to update it. - PR Review Comment: https://git.openjdk.org/jdk/pull/20897#discussion_r1747653242
RFR: 8339638: Update vmTestbase/nsk/jvmti/*Field*Watch tests to use virtual thread factory
Field watch tests updated to be executed with virtual threads when virtual test thread factory enabled.I added jdk.test.lib.thread.TestThreadFactory to created test threads so they support virtual thread factory jtreg plugin. It might makes sense to use it in different tests so virtual threads are tests more extensively. There is JDIThreadFactory in nsk/sharejdi which I copied. I am going to update JDI tests to use jdk.test.lib.thread.TestThreadFactory later. - Commit messages: - 8339638: Update vmTestbase/nsk/jvmti/*Field*Watch tests to use virtual thread factory Changes: https://git.openjdk.org/jdk/pull/20897/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20897&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8339638 Stats: 75 lines in 5 files changed: 63 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/20897.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20897/head:pull/20897 PR: https://git.openjdk.org/jdk/pull/20897
Integrated: 8338934: vmTestbase/nsk/jvmti/*Field*Watch/TestDescription.java tests timeout intermittently
On Thu, 29 Aug 2024 18:18:12 GMT, Leonid Mesnik wrote:
> The tests time out because of dedlock of of the thread that is in transition
> and thread changing field watches.
>
> They use JvmtiThreadState_lock and JvmtiVTMSTransitionDisabler.
>
> The change field watch require disabler, but attempt to use it only when
> already locked in
>
> void
> JvmtiEventController::change_field_watch(jvmtiEvent event_type, bool added) {
> MutexLocker mu(JvmtiThreadState_lock);
> JvmtiEventControllerPrivate::change_field_watch(event_type, added);
> }
>
>
> while it is needed to first disable transitions and then try to use
> JvmtiThreadState_lock.
> I quickly looked that most of jvmti methods do it already. Also moved
> disabler into jvmtiEmv.cpp to be more consistent with other methods.
>
>
> I was able to verify my fix in loom repo locally. and run tier1 + tier5-svc
> testing in jdk.
This pull request has now been integrated.
Changeset: 92aafb43
Author:Leonid Mesnik
URL:
https://git.openjdk.org/jdk/commit/92aafb43424321d8f2552aa34a9a3df291abf992
Stats: 8 lines in 3 files changed: 4 ins; 2 del; 2 mod
8338934: vmTestbase/nsk/jvmti/*Field*Watch/TestDescription.java tests timeout
intermittently
Reviewed-by: sspitsyn, amenkov
-
PR: https://git.openjdk.org/jdk/pull/20776
Re: RFR: 8338708: Don't create/destroy debug agent cmdQueueLock for each connection.
On Fri, 30 Aug 2024 20:18:15 GMT, Chris Plummer wrote: >> test/jdk/com/sun/jdi/ReattachStressTest.java line 76: >> >>> 74: // Read the first character of output to make sure we've >>> waited until the >>> 75: // debuggee is ready. This will be the debug agent's >>> "Listening..." message. >>> 76: InputStream is = p.getInputStream(); >> >> The stream is not closed. Not sure it can cause problems but probably could >> once we reuse agent for tests. > > The `InputStream` is already created when the `Process` is spawned. This API > is simply returning it. It doesn't seem appropriate to close it in this case. > It should be closed automatically when the Process is destroyed. I also see a > lot of other uses of `p.getInputStream()` that don't explicitly close, > including some which are obviously not closed: > > `p.getInputStream().read();` Thanks for explanation, sorry for noise. - PR Review Comment: https://git.openjdk.org/jdk/pull/20755#discussion_r1739415245
Re: RFR: 8338708: Don't create/destroy debug agent cmdQueueLock for each connection.
On Wed, 28 Aug 2024 21:40:57 GMT, Chris Plummer wrote:
> Only allocate the cmdQueueLock once rather than allocate each time there is a
> new connection (and destroy when the connection is terminated).
>
> Currently each time there is a new debug agent connection established, the
> cmdQueueLock is allocated. It is then destroyed when the connection is
> dropped, only to be recreated on the next connection. I looked closely at the
> use of this lock, and what happens when the connection is dropped, and I
> can't see any reason to create and then dispose of cmdQueueLock for each
> connection. Of the 18 debug agent raw monitors, this is the only one that
> gets destroyed and recreated. The other 17 stick around for the next
> connection. I'd like to get rid of this reallocation in order to support
> static initialization of all the debug agent raw monitors on startup. This
> will be done as part of the ranked monitor support
> ([JDK-8328866](https://bugs.openjdk.org/browse/JDK-8328866)).
>
> As part of the testing for this change, I put an assert in place when we try
> to create cmdQueueLock for the 2nd time. I ran all our debugger tests, and
> unfortunately none of them failed. This means we don't currently test
> attaching to the debug agent more than once in any of our tests, so I did two
> things to better test out this change. The first was to write a test that
> attaches numerous times in sequence, and the 2nd was to manually attach and
> reattach using jdb. At the same time I stepped through the debug agent code
> in gdb just to help better understand the setup and use of cmdQueueLock. I
> didn't see anything that gave me concern about making this change.
>
> Tested by running all jdi, jdwp, and jdb tests on all supported platforms.
src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c line 90:
> 88: /* We may be starting a new connection after an error */
> 89: cmdQueue = NULL;
> 90: if (cmdQueueLock == NULL) {
Doesn't it makes sense to check that lock is not locked here?
Is there possibility that lock is left locked for disconnected process or any
other similar timeout. Probably it is not covered by our tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20755#discussion_r1739149630
Re: RFR: 8338708: Don't create/destroy debug agent cmdQueueLock for each connection.
On Wed, 28 Aug 2024 21:40:57 GMT, Chris Plummer wrote: > Only allocate the cmdQueueLock once rather than allocate each time there is a > new connection (and destroy when the connection is terminated). > > Currently each time there is a new debug agent connection established, the > cmdQueueLock is allocated. It is then destroyed when the connection is > dropped, only to be recreated on the next connection. I looked closely at the > use of this lock, and what happens when the connection is dropped, and I > can't see any reason to create and then dispose of cmdQueueLock for each > connection. Of the 18 debug agent raw monitors, this is the only one that > gets destroyed and recreated. The other 17 stick around for the next > connection. I'd like to get rid of this reallocation in order to support > static initialization of all the debug agent raw monitors on startup. This > will be done as part of the ranked monitor support > ([JDK-8328866](https://bugs.openjdk.org/browse/JDK-8328866)). > > As part of the testing for this change, I put an assert in place when we try > to create cmdQueueLock for the 2nd time. I ran all our debugger tests, and > unfortunately none of them failed. This means we don't currently test > attaching to the debug agent more than once in any of our tests, so I did two > things to better test out this change. The first was to write a test that > attaches numerous times in sequence, and the 2nd was to manually attach and > reattach using jdb. At the same time I stepped through the debug agent code > in gdb just to help better understand the setup and use of cmdQueueLock. I > didn't see anything that gave me concern about making this change. > > Tested by running all jdi, jdwp, and jdb tests on all supported platforms. The changes looks good except one nit. test/jdk/com/sun/jdi/ReattachStressTest.java line 76: > 74: // Read the first character of output to make sure we've > waited until the > 75: // debuggee is ready. This will be the debug agent's > "Listening..." message. > 76: InputStream is = p.getInputStream(); The stream is not closed. Not sure it can cause problems but probably could once we reuse agent for tests. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20755#pullrequestreview-2273078247 PR Review Comment: https://git.openjdk.org/jdk/pull/20755#discussion_r1739136266
Re: RFR: 8338934: vmTestbase/nsk/jvmti/*Field*Watch/TestDescription.java tests timeout intermittently [v2]
> The tests time out because of dedlock of of the thread that is in transition
> and thread changing field watches.
>
> They use JvmtiThreadState_lock and JvmtiVTMSTransitionDisabler.
>
> The change field watch require disabler, but attempt to use it only when
> already locked in
>
> void
> JvmtiEventController::change_field_watch(jvmtiEvent event_type, bool added) {
> MutexLocker mu(JvmtiThreadState_lock);
> JvmtiEventControllerPrivate::change_field_watch(event_type, added);
> }
>
>
> while it is needed to first disable transitions and then try to use
> JvmtiThreadState_lock.
> I quickly looked that most of jvmti methods do it already. Also moved
> disabler into jvmtiEmv.cpp to be more consistent with other methods.
>
>
> I was able to verify my fix in loom repo locally. and run tier1 + tier5-svc
> testing in jdk.
Leonid Mesnik has updated the pull request incrementally with one additional
commit since the last revision:
fixed spaces
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20776/files
- new: https://git.openjdk.org/jdk/pull/20776/files/89e57b0d..cde6c486
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20776&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20776&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/20776.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20776/head:pull/20776
PR: https://git.openjdk.org/jdk/pull/20776
RFR: 8338934: vmTestbase/nsk/jvmti/*Field*Watch/TestDescription.java tests timeout intermittently
The tests time out because of dedlock of of the thread that is in transition
and thread changing field watches.
They use JvmtiThreadState_lock and JvmtiVTMSTransitionDisabler.
The change field watch require disabler, but attempt to use it only when
already locked in
void
JvmtiEventController::change_field_watch(jvmtiEvent event_type, bool added) {
MutexLocker mu(JvmtiThreadState_lock);
JvmtiEventControllerPrivate::change_field_watch(event_type, added);
}
while it is needed to first disable transitions and then try to use
JvmtiThreadState_lock.
I quickly looked that most of jvmti methods do it already. Also moved disabler
into jvmtiEmv.cpp to be more consistent with other methods.
I was able to verify my fix in loom repo locally. and run tier1 + tier5-svc
testing in jdk.
-
Commit messages:
- change lock
- moved disabler
Changes: https://git.openjdk.org/jdk/pull/20776/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20776&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8338934
Stats: 8 lines in 3 files changed: 4 ins; 2 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/20776.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20776/head:pull/20776
PR: https://git.openjdk.org/jdk/pull/20776
Re: RFR: 8337317: serviceability/jvmti tests failed with FATAL ERROR in native method: Failed during the GetClassSignature call [v4]
On Tue, 27 Aug 2024 23:12:45 GMT, Alex Menkov wrote: >> The fix adds guards against JVMTI_ERROR_WRONG_PHASE error in event handlers >> >> Testing: hotspot/jtreg/serviceability/jvmti on all platforms > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > replace printf with LOG Looks good, - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20699#pullrequestreview-2264630664
Re: RFR: 8337317: serviceability/jvmti tests failed with FATAL ERROR in native method: Failed during the GetClassSignature call [v3]
On Tue, 27 Aug 2024 23:02:57 GMT, Alex Menkov wrote:
>> test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
>> line 333:
>>
>>> 331: }
>>> 332:
>>> 333: err = jvmti->CreateRawMonitor("Event Monitor", &event_mon);
>>
>> Sorry, forgot to mention.
>> There is a function ' create_raw_monitor(jvmti, name)" in jvmti commo that
>> saves a few more lines:
>> use
>> event_mon = create_raw_monitor(jvmti, "Event Montori");
>
> I saw the function, but don't see much sense in it.
> On error it just return null (so need to check result anyway) and it's not
> possible to get jvmti error
Strange, this function should be improved to at least log the probleml.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20699#discussion_r1733632140
Re: RFR: 8337317: serviceability/jvmti tests failed with FATAL ERROR in native method: Failed during the GetClassSignature call [v3]
On Tue, 27 Aug 2024 01:18:46 GMT, Alex Menkov wrote:
>> The fix adds guards against JVMTI_ERROR_WRONG_PHASE error in event handlers
>>
>> Testing: hotspot/jtreg/serviceability/jvmti on all platforms
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> removed unneeded include
test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
line 333:
> 331: }
> 332:
> 333: err = jvmti->CreateRawMonitor("Event Monitor", &event_mon);
Sorry, forgot to mention.
There is a function ' create_raw_monitor(jvmti, name)" in jvmti commo that
saves a few more lines:
use
event_mon = create_raw_monitor(jvmti, "Event Montori");
test/hotspot/jtreg/serviceability/jvmti/VMObjectAlloc/libVMObjectAlloc.cpp line
66:
> 64: RawMonitorLocker locker(jvmti, jni, event_mon);
> 65:
> 66: printf("VMDeath\n");
We shouldn't use printf in tests. It doesn't flush stdout and output might be
missed. So it is always unclear if we had vmdeath or not. Please use LOG always.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20699#discussion_r1733423097
PR Review Comment: https://git.openjdk.org/jdk/pull/20699#discussion_r1733426692
Re: RFR: 8337317: serviceability/jvmti tests failed with FATAL ERROR in native method: Failed during the GetClassSignature call
On Sat, 24 Aug 2024 00:51:54 GMT, Alex Menkov wrote:
> The fix adds guards against JVMTI_ERROR_WRONG_PHASE error in event handlers
>
> Testing: hotspot/jtreg/serviceability/jvmti on all platforms
Changes requested by lmesnik (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
line 54:
> 52: }
> 53:
> 54: struct MonitorLock {
jvmti.common has RawMonitorLocker
(and LOG)
could they be used in test?
or even better to just use
std::atomic
if it is enough.
The atomic are not used in hotspot but no limitation about using them in test
code.
-
PR Review: https://git.openjdk.org/jdk/pull/20699#pullrequestreview-2258280922
PR Review Comment: https://git.openjdk.org/jdk/pull/20699#discussion_r1729646507
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v6]
On Thu, 22 Aug 2024 20:08:43 GMT, Roman Kennke wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> This PR also changes some code paths (mostly in GCs) to be more careful when
>> accessing Klass* (or mark-word or size) to be able to fetch it from the
>> forwardee in case the object is forwarded.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - Instances can now have their base-offset (the offset where the field
>> layouter starts to place fields) at offset 8 (instead of 12 or 16).
>> - Arrays will now store their length at offset 8.
>> - CDS can now write and read archives with the compressed header. However,
>> it is not possible to read an archive that has been written with an opposite
>> setting of UseCompactObjectHeaders. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix bit counts in GCForwarding
test/hotspot/jtreg/runtime/cds/appcds/TestZGCWithCDS.java line 59:
> 57: public static void main(String... args) throws Exception {
> 58: String zGenerational = args[0];
> 59: String compactHeaders = "-XX:" +
> (zGenerational.equals("-XX:+ZGenerational") ? "+" : "-") +
> "UseCompactObjectHeaders";
The test failing with
stdout: [[0.176s][info][cds] trying to map
/opt/mach5/mesos/work_dir/slaves/a20696e7-ae7d-4d37-8e9c-83f99ef002cb-S2261/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/f0801999-993f-4e08-b017-08b33a8ec44f/runs/34cc555e-ae8f-4a48-8175-e998194f204b/testoutput/test-support/jtreg_open_test_hotspot_jtreg_hotspot_cds_relocation/scratch/5/appcds-18h50m16s773.jsa
[0.176s][info][cds] Opened archive
/opt/mach5/mesos/work_dir/slaves/a20696e7-ae7d-4d37-8e9c-83f99ef002cb-S2261/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/f0801999-993f-4e08-b017-08b33a8ec44f/runs/34cc555e-ae8f-4a48-8175-e998194f204b/testoutput/test-support/jtreg_open_test_hotspot_jtreg_hotspot_cds_relocation/scratch/5/appcds-18h50m16s773.jsa.
[0.176s][info][cds] Archive was created with UseCompressedOops = 0,
UseCompressedClassPointers = 1
[0.176s][info][cds] The shared archive file's UseCompactObjectHeaders setting
(enabled) does not equal the current UseCompactObjectHeaders setting (disabled).
[0.176s][info][cds] Initialize static archive failed.
[0.176s][info][cds] Unable to map shared spaces
[0.176s][error][cds] An error has occurred while processing the shared archive
file.
[0.176s][error][cds] Unable to map shared spaces
Error occurred during initialization of VM
Unable to use shared archive.
];
stderr: []
exitValue = 1
java.lang.RuntimeException: 'Hello World' missing from stdout/stderr
at
jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:252)
at TestZGCWithCDS.main(TestZGCWithCDS.java:123)
at
java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:573)
at
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.r
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v6]
On Thu, 22 Aug 2024 20:08:43 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Fix bit counts in GCForwarding Changes requested by lmesnik (Reviewer). make/Images.gmk line 135: > 133: # > 134: # Param1 - VM variant (e.g., server, client, zero, ...) > 135: # Param2 - _nocoops, _coh, _nocoops_coh, or empty The -XX:+UseCompactObjectHeaders ssems to incompatible withe zero vm. The zero vm build start failing while generating shared archive with +UseCompactObjectHeaders. Generation should be disabled by default for zero to don't break the build. - PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2257621775 PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1729222671
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
On Tue, 20 Aug 2024 18:38:49 GMT, Serguei Spitsyn wrote: >> Leonid Mesnik has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed identation. > > Marked as reviewed by sspitsyn (Reviewer). @sspitsyn, @dean-long Thank you for review - PR Comment: https://git.openjdk.org/jdk/pull/20587#issuecomment-2299508676
Integrated: 8205957: setfldw001/TestDescription.java fails with bad field value
On Wed, 14 Aug 2024 19:34:26 GMT, Leonid Mesnik wrote: > The summary of the problem. Test is intermittently failing because can't get > expected field watch event. > The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. > I verified that frame with the method 'run' of setfmodw001b is compiled when > line > 118: setfmodw001.setWatch(4); > is executed, however the thread is in interp_only mode. The watch events are > supported by interpreter only and just ignored by compiled code. > > The reason of failure is race beteween setting interp_only mode in line > > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 > > and calling method call_helper while > the method run() > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 > > in newly created thread 'setfmodw001b' is invoked. > > The javaCalls:call are used to invoke methods from hotspot, so it might be > rare issues. But still, synchronization might be improved. > The > void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, > JavaCallArguments* args, TRAPS) > > checks if interp_only mode is set and use 'Method::from_interpreted_entry()' > if not. However the interp_only might be set later before compiled method is > called (or enter first safe point?). This might happens in safepoint during > transition via handshake. > So the running thread is in interp_only mode however the run() method is > compiled and executed already and never going to be deoptimized. > > The additional setWatch calls don't try to deptimize method that are already > in interp_only mode. > > BTW, the when JVMCI is enabled and verified adapter exists it is also will be > loaded even in interp_only mode set. There is not race here, just ignoring of > interp_only mode. > > I run failing test with Xcomp ~1000 times and tiers1-5. This pull request has now been integrated. Changeset: c646efc3 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/c646efc366342564baebd2f17133e14780abcaa8 Stats: 48 lines in 3 files changed: 15 ins; 22 del; 11 mod 8205957: setfldw001/TestDescription.java fails with bad field value Reviewed-by: sspitsyn, dlong - PR: https://git.openjdk.org/jdk/pull/20587
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
On Tue, 20 Aug 2024 17:28:12 GMT, Dean Long wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed identation.
>
> src/hotspot/share/runtime/javaCalls.cpp line 403:
>
>> 401: } else {
>> 402: // Since the call stub sets up like the interpreter we call
>> the from_interpreted_entry
>> 403: // so we can go compiled via a i2c.
>
> Do you think removed comment "Otherwise initial entry method will always run
> interpreted.", or it's understood and redundant?
Yes. I believe that expression in the 'if' clearly explain the reason of
using interpreter. So comment is redundant.
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001/TestDescription.java
> line 57:
>
>> 55: * /test/lib
>> 56: * @run main/othervm/native -agentlib:setfmodw001
>> -XX:TraceJVMTI=ec+,+ioe,+s -Xlog:jvmti=trace:file=vm.%p.log
>> nsk.jvmti.SetFieldModificationWatch.setfmodw001
>> 57: */
>
> What's the reason for removing this test run?
I was not able to reproduce failure locally at the beginning. So I added this
test run just to get more information about failure from CI runs for initial
analysis.
The information from logging clearly pointed that we just never got event while
set Thread-1 to interprter_only mode correctly.
However, it is not needed now so I am removing it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723701025
PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723704840
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value [v2]
> The summary of the problem. Test is intermittently failing because can't get > expected field watch event. > The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. > I verified that frame with the method 'run' of setfmodw001b is compiled when > line > 118: setfmodw001.setWatch(4); > is executed, however the thread is in interp_only mode. The watch events are > supported by interpreter only and just ignored by compiled code. > > The reason of failure is race beteween setting interp_only mode in line > > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 > > and calling method call_helper while > the method run() > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 > > in newly created thread 'setfmodw001b' is invoked. > > The javaCalls:call are used to invoke methods from hotspot, so it might be > rare issues. But still, synchronization might be improved. > The > void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, > JavaCallArguments* args, TRAPS) > > checks if interp_only mode is set and use 'Method::from_interpreted_entry()' > if not. However the interp_only might be set later before compiled method is > called (or enter first safe point?). This might happens in safepoint during > transition via handshake. > So the running thread is in interp_only mode however the run() method is > compiled and executed already and never going to be deoptimized. > > The additional setWatch calls don't try to deptimize method that are already > in interp_only mode. > > BTW, the when JVMCI is enabled and verified adapter exists it is also will be > loaded even in interp_only mode set. There is not race here, just ignoring of > interp_only mode. > > I run failing test with Xcomp ~1000 times and tiers1-5. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed identation. - Changes: - all: https://git.openjdk.org/jdk/pull/20587/files - new: https://git.openjdk.org/jdk/pull/20587/files/fdd9f91c..2a148a29 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20587&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20587&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20587.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20587/head:pull/20587 PR: https://git.openjdk.org/jdk/pull/20587
Re: RFR: 8205957: setfldw001/TestDescription.java fails with bad field value
On Wed, 14 Aug 2024 19:34:26 GMT, Leonid Mesnik wrote: > The summary of the problem. Test is intermittently failing because can't get > expected field watch event. > The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. > I verified that frame with the method 'run' of setfmodw001b is compiled when > line > 118: setfmodw001.setWatch(4); > is executed, however the thread is in interp_only mode. The watch events are > supported by interpreter only and just ignored by compiled code. > > The reason of failure is race beteween setting interp_only mode in line > > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 > > and calling method call_helper while > the method run() > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 > > in newly created thread 'setfmodw001b' is invoked. > > The javaCalls:call are used to invoke methods from hotspot, so it might be > rare issues. But still, synchronization might be improved. > The > void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, > JavaCallArguments* args, TRAPS) > > checks if interp_only mode is set and use 'Method::from_interpreted_entry()' > if not. However the interp_only might be set later before compiled method is > called (or enter first safe point?). This might happens in safepoint during > transition via handshake. > So the running thread is in interp_only mode however the run() method is > compiled and executed already and never going to be deoptimized. > > The additional setWatch calls don't try to deptimize method that are already > in interp_only mode. > > BTW, the when JVMCI is enabled and verified adapter exists it is also will be > loaded even in interp_only mode set. There is not race here, just ignoring of > interp_only mode. > > I run failing test with Xcomp ~1000 times and tiers1-5. I was able to reproduce failure in "-Xcomp +ZGC'' once in a couple of several hundred runs. Not reproduced anymore with thousands of executions. Run tier1-5 to ensure all svc tests passed. There is no easy way to develop regression test. It should provoke call_helper for compiled method with breakpoint and setting the only single setWatch. Not sure it can find anything in reasonable time. The call_helper is used only to call methods from hotspot directly, like , run() for Thread.start and similar not very common methods. I think to review any other possible cases and and thread stack verification that check that interp_only thread don't contain compiled frames on the stack. So we could find similar issues using our testing. Need to find the good place to inject this self-check. There are also a couple of places for improvements in this events handling. They are not directly rtelated to this bug. Will file them separately. - PR Comment: https://git.openjdk.org/jdk/pull/20587#issuecomment-2299317622
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 Tests looks good. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20628#pullrequestreview-2246396853
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 Changes requested by lmesnik (Reviewer). test/jdk/java/lang/management/MemoryMXBean/TestVerboseMemory.java line 30: > 28: * related unified logging is enabled. > 29: * > 30: * @run main/othervm -Xlog:gc=trace:file=vm.log TestVerboseMemory false Does it makes sense to add case where not only 'gc' logging is set from CLI with file output? It would be the exact copy of failed case. @run main/othervm -Xlog:all=trace:file=vm.log TestVerboseMemory false (Same for ClassLoading Bean) - PR Review: https://git.openjdk.org/jdk/pull/20628#pullrequestreview-2245982878 PR Review Comment: https://git.openjdk.org/jdk/pull/20628#discussion_r1722055680
RFR: 8205957: setfldw001/TestDescription.java fails with bad field value
The summary of the problem. Test is intermittently failing because can't get expected field watch event. The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. I verified that frame with the method 'run' of setfmodw001b is compiled when line 118: setfmodw001.setWatch(4); is executed, however the thread is in interp_only mode. The watch events are supported by interpreter only and just ignored by compiled code. The reason of failure is race beteween setting interp_only mode in line https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 and calling method call_helper while the method run() https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 in newly created thread 'setfmodw001b' is invoked. The javaCalls:call are used to invoke methods from hotspot, so it might be rare issues. But still, synchronization might be improved. The void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, JavaCallArguments* args, TRAPS) checks if interp_only mode is set and use 'Method::from_interpreted_entry()' if not. However the interp_only might be set later before compiled method is called (or enter first safe point?). This might happens in safepoint during transition via handshake. So the running thread is in interp_only mode however the run() method is compiled and executed already and never going to be deoptimized. The additional setWatch calls don't try to deptimize method that are already in interp_only mode. BTW, the when JVMCI is enabled and verified adapter exists it is also will be loaded even in interp_only mode set. There is not race here, just ignoring of interp_only mode. I run failing test with Xcomp ~1000 times and tiers1-5. - Commit messages: - fix - f - with jvmci - fix Changes: https://git.openjdk.org/jdk/pull/20587/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20587&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8205957 Stats: 49 lines in 3 files changed: 16 ins; 22 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/20587.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20587/head:pull/20587 PR: https://git.openjdk.org/jdk/pull/20587
Re: RFR: 8332488: Add JVMTI DataDumpRequest to the debug agent [v6]
On Tue, 13 Aug 2024 22:28:10 GMT, Chris Plummer wrote: >> JVMTI has a somewhat unique event called DataDumpRequest. One way it is >> triggered is via the JVMTI.data_dump jcmd, which causes JVMTI to send the >> DataDumpRequest event to all agents that have registered for the event >> callback. The agent is free to do pretty much what it wants during the >> callback, but the normal usage is to dump anything that might be useful for >> debugging the agent. In the case of the debug agent, it could dump internal >> data like the list of known threads and event handlers. After ranked monitor >> support is complete, it can also dump the state of all jvmti raw monitors >> that the debug agent uses. >> >> I decided to not enable this feature by default, and not make public the >> option to enable it. This should only be used by developers working on the >> debug agent, or by users when requested to do so (by debug agent developers) >> to help debug a debug agent problem. >> >> Most of the code executed during the data dump was only available for debug >> builds, so I've made it available for all builds. Their addition does not >> affect product builds except for adding a small footprint. >> >> TBD is directing the output to a file. This is useful for some of the >> debugger tests that don't include the debuggee output in the log. This seems >> to be the case for most com/sun/jdi tests. I decided not to include it for >> this first pass since it is rather disruptive and detracts from the main >> changes being made. >> >> testing tbd: run all tier1, tier2, and. tie5 svc tests. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Added newline Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20367#pullrequestreview-2238778317
Re: RFR: 8332488: Add JVMTI DataDumpRequest to the debug agent [v5]
On Mon, 12 Aug 2024 21:35:49 GMT, Chris Plummer wrote: >> JVMTI has a somewhat unique event called DataDumpRequest. One way it is >> triggered is via the JVMTI.data_dump jcmd, which causes JVMTI to send the >> DataDumpRequest event to all agents that have registered for the event >> callback. The agent is free to do pretty much what it wants during the >> callback, but the normal usage is to dump anything that might be useful for >> debugging the agent. In the case of the debug agent, it could dump internal >> data like the list of known threads and event handlers. After ranked monitor >> support is complete, it can also dump the state of all jvmti raw monitors >> that the debug agent uses. >> >> I decided to not enable this feature by default, and not make public the >> option to enable it. This should only be used by developers working on the >> debug agent, or by users when requested to do so (by debug agent developers) >> to help debug a debug agent problem. >> >> Most of the code executed during the data dump was only available for debug >> builds, so I've made it available for all builds. Their addition does not >> affect product builds except for adding a small footprint. >> >> TBD is directing the output to a file. This is useful for some of the >> debugger tests that don't include the debuggee output in the log. This seems >> to be the case for most com/sun/jdi tests. I decided not to include it for >> this first pass since it is rather disruptive and detracts from the main >> changes being made. >> >> testing tbd: run all tier1, tier2, and. tie5 svc tests. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > git rid of unnecessary @build Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20367#pullrequestreview-2236629903
Integrated: 8330535: Update nsk/jdb tests to use driver instead of othervm
On Fri, 9 Aug 2024 02:25:33 GMT, Leonid Mesnik wrote: > The nsk/jdb tests updated to use driver instead of othervm. The nsk/share/jdb > code updated to set correct classpath and throw Exceptions instead of exit. > > Testing by running tests with different flags and in different CI modes. > > The 'shouldPass()' might be changed to throw SkippedException but I am going > to do this separately. This pull request has now been integrated. Changeset: 4417c276 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/4417c276e484c1fe137ed7f4a7c28709d0c99af2 Stats: 483 lines in 69 files changed: 6 ins; 273 del; 204 mod 8330535: Update nsk/jdb tests to use driver instead of othervm Reviewed-by: cjplummer - PR: https://git.openjdk.org/jdk/pull/20518
Re: RFR: 8332488: Add JVMTI DataDumpRequest to the debug agent [v2]
On Thu, 8 Aug 2024 20:56:16 GMT, Chris Plummer wrote: >> test/jdk/com/sun/jdi/DataDumpTest.java line 51: >> >>> 49: * @library /test/lib >>> 50: * @modules jdk.jdi >>> 51: * @build DataDumpTest >> >> Is the build needed to build 'DataDumpTestTarg' because it is not explicitly >> used by the test? > > It's not needed because it is in the same file. I'm not even sure it would > work. ough, I mean 'is @build DataDumpTest' needed to build DataDumpTestTarg? The DataDumpTest should be built without explicit @build command. - PR Review Comment: https://git.openjdk.org/jdk/pull/20367#discussion_r1714265864
Re: RFR: 8330535: Update nsk/jdb tests to use driver instead of othervm [v2]
> The nsk/jdb tests updated to use driver instead of othervm. The nsk/share/jdb > code updated to set correct classpath and throw Exceptions instead of exit. > > Testing by running tests with different flags and in different CI modes. > > The 'shouldPass()' might be changed to throw SkippedException but I am going > to do this separately. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed exception - Changes: - all: https://git.openjdk.org/jdk/pull/20518/files - new: https://git.openjdk.org/jdk/pull/20518/files/23bbe47e..0d5b9494 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20518&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20518&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20518/head:pull/20518 PR: https://git.openjdk.org/jdk/pull/20518
RFR: 8330535: Update nsk/jdb tests to use driver instead of othervm
The nsk/jdb tests updated to use driver instead of othervm. The nsk/share/jdb code updated to set correct classpath and throw Exceptions instead of exit. Testing by running tests with different flags and in different CI modes. The 'shouldPass()' might be changed to throw SkippedException but I am going to do this separately. - Commit messages: - updated tests Changes: https://git.openjdk.org/jdk/pull/20518/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20518&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330535 Stats: 482 lines in 69 files changed: 6 ins; 272 del; 204 mod Patch: https://git.openjdk.org/jdk/pull/20518.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20518/head:pull/20518 PR: https://git.openjdk.org/jdk/pull/20518
Re: RFR: 8332488: Add JVMTI DataDumpRequest to the debug agent. [v3]
On Thu, 8 Aug 2024 20:59:24 GMT, Chris Plummer wrote: > FYI, this is what the (entire) debuggee output looks like for this test: > > ``` > Deuggee output: > Should we see the same report for CTRL+\ or after sending SIGQUIT signal? I see it should cause this request. - PR Comment: https://git.openjdk.org/jdk/pull/20367#issuecomment-2276649953
Re: RFR: 8332488: Add JVMTI DataDumpRequest to the debug agent. [v2]
On Thu, 8 Aug 2024 19:28:09 GMT, Chris Plummer wrote: >> JVMTI has a somewhat unique event called DataDumpRequest. One way it is >> triggered is via the JVMTI.data_dump jcmd, which causes JVMTI to send the >> DataDumpRequest event to all agents that have registered for the event >> callback. The agent is free to do pretty much what it wants during the >> callback, but the normal usage is to dump anything that might be useful for >> debugging the agent. In the case of the debug agent, it could dump internal >> data like the list of known threads and event handlers. After ranked monitor >> support is complete, it can also dump the state of all jvmti raw monitors >> that the debug agent uses. >> >> I decided to not enable this feature by default, and not make public the >> option to enable it. This should only be used by developers working on the >> debug agent, or by users when requested to do so (by debug agent developers) >> to help debug a debug agent problem. >> >> Most of the code executed during the data dump was only available for debug >> builds, so I've made it available for all builds. Their addition does not >> affect product builds except for adding a small footprint. >> >> TBD is directing the output to a file. This is useful for some of the >> debugger tests that don't include the debuggee output in the log. This seems >> to be the case for most com/sun/jdi tests. I decided not to include it for >> this first pass since it is rather disruptive and detracts from the main >> changes being made. >> >> testing tbd: run all tier1, tier2, and. tie5 svc tests. > > Chris Plummer has updated the pull request incrementally with two additional > commits since the last revision: > > - Minor improvment to datadump output > - Add test cast for new debug agent datadump support test/jdk/com/sun/jdi/DataDumpTest.java line 51: > 49: * @library /test/lib > 50: * @modules jdk.jdi > 51: * @build DataDumpTest Is the build needed to build 'DataDumpTestTarg' because it is not explicitly used by the test? test/jdk/com/sun/jdi/DataDumpTest.java line 52: > 50: * @modules jdk.jdi > 51: * @build DataDumpTest > 52: * @run main/othervm/timeout=15 DataDumpTest Is the othervm is really needed in this test? Also, it is unclear why the timeout=15 and not usual 120? - PR Review Comment: https://git.openjdk.org/jdk/pull/20367#discussion_r1710199916 PR Review Comment: https://git.openjdk.org/jdk/pull/20367#discussion_r1710203086
Re: RFR: 8332488: Add JVMTI DataDumpRequest to the debug agent. [v3]
On Thu, 8 Aug 2024 20:06:08 GMT, Chris Plummer wrote: >> JVMTI has a somewhat unique event called DataDumpRequest. One way it is >> triggered is via the JVMTI.data_dump jcmd, which causes JVMTI to send the >> DataDumpRequest event to all agents that have registered for the event >> callback. The agent is free to do pretty much what it wants during the >> callback, but the normal usage is to dump anything that might be useful for >> debugging the agent. In the case of the debug agent, it could dump internal >> data like the list of known threads and event handlers. After ranked monitor >> support is complete, it can also dump the state of all jvmti raw monitors >> that the debug agent uses. >> >> I decided to not enable this feature by default, and not make public the >> option to enable it. This should only be used by developers working on the >> debug agent, or by users when requested to do so (by debug agent developers) >> to help debug a debug agent problem. >> >> Most of the code executed during the data dump was only available for debug >> builds, so I've made it available for all builds. Their addition does not >> affect product builds except for adding a small footprint. >> >> TBD is directing the output to a file. This is useful for some of the >> debugger tests that don't include the debuggee output in the log. This seems >> to be the case for most com/sun/jdi tests. I decided not to include it for >> this first pass since it is rather disruptive and detracts from the main >> changes being made. >> >> testing tbd: run all tier1, tier2, and. tie5 svc tests. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Improved comments. In general changes looks good, there are only a couple small test questions. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20367#pullrequestreview-2228744867
Re: RFR: 8336846: assert(state->get_thread() == jt) failed: handshake unsafe conditions [v2]
On Thu, 1 Aug 2024 09:37:08 GMT, Serguei Spitsyn wrote: >> The JVMTI Watch Field functions do not disable VTMS transitions with the >> `JvmtiVTMSTransitionDisabler`: >> - `SetFieldAccessWatch()` >> - `ClearFieldAccessWatch()` >> - `SetFieldModificationWatch()` >> - `ClearFieldModificationWatch()` >> so in the `recompute_enabled()` we could see that a vthread is mounted, but >> in the `EnterInterpOnlyModeClosure` handshake the thread could have been >> unmounted already. This is a root cause of failures with this assert. >> >> The fix is to disable transitions in the >> `JvmtiEventControllerPrivate::change_field_watch()` function. >> >> Testing: >> - TBD: submit mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > rearranged to have one JvmtiVTMSTransitionDisabler instead of two Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20413#pullrequestreview-2216273600
Re: RFR: 8337667: sun/tools/jcmd/TestJcmdPIDSubstitution.java is failing on mac and windows
On Thu, 1 Aug 2024 14:25:47 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [8337667](https://bugs.openjdk.org/browse/JDK-8337667) . > > The `Compiler.perfmap` test case is failing on mac and windows as it is only > enabled in linux. I am removing this test case and noting that this use case > is already tested in > [test/hotspot/jtreg/serviceability/dcmd/compiler/PerfMapTest.java](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/serviceability/dcmd/compiler/PerfMapTest.java#L88) > which is linux specific. > > Thanks, > Sonia Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20421#pullrequestreview-2216272984
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v17]
On Mon, 29 Jul 2024 19:08:17 GMT, Sonia Zaldana Calles wrote: >> Hi all, >> >> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) >> enabling jcmd diagnostic commands that issue an output file to accept the >> `%p` pattern in the file name and substitute it for the PID. >> >> This PR addresses the following diagnostic commands: >> - [x] Compiler.perfmap >> - [x] GC.heap_dump >> - [x] System.dump_map >> - [x] Thread.dump_to_file >> - [x] VM.cds >> >> Note that some jcmd diagnostic commands already enable this functionality >> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). >> >> I propose opening a separate issue to track updating the man page similarly >> to how it’s done for the JFR diagnostic commands. For example, >> >> >> filename (Optional) Name of the file to which the flight recording >> data is >>written when the recording is stopped. If no filename is >> given, a >>filename is generated from the PID and the current date >> and is >>placed in the directory where the process was started. The >>filename may also be a directory in which case, the >> filename is >>generated from the PID and the current date in the >> specified >>directory. (STRING, no default value) >> >>Note: If a filename is given, '%p' in the filename will be >>replaced by the PID, and '%t' will be replaced by the >> time in >>'_MM_dd_HH_mm_ss' format. >> >> >> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), >> sources for the jcmd manpage remain in Oracle internal repos so this PR >> can’t address that. >> >> Testing: >> >> - [x] Added test case passes. >> - [x] Modified existing VM.cds tests to also check for `%p` filenames. >> >> Looking forward to your comments and addressing any diagnostic commands I >> might have missed (if any). >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > last lingering change Looks good also. The main goal of my request was to unify arguments paring. /using type FILE for 'char *' seems enough and no need to introduce new filename type for values. Thanks for fixing. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2205814072
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v14]
On Thu, 25 Jul 2024 15:31:05 GMT, Sonia Zaldana Calles
wrote:
>> Hi all,
>>
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492)
>> enabling jcmd diagnostic commands that issue an output file to accept the
>> `%p` pattern in the file name and substitute it for the PID.
>>
>> This PR addresses the following diagnostic commands:
>> - [x] Compiler.perfmap
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>>
>> Note that some jcmd diagnostic commands already enable this functionality
>> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`).
>>
>> I propose opening a separate issue to track updating the man page similarly
>> to how it’s done for the JFR diagnostic commands. For example,
>>
>>
>> filename (Optional) Name of the file to which the flight recording
>> data is
>>written when the recording is stopped. If no filename is
>> given, a
>>filename is generated from the PID and the current date
>> and is
>>placed in the directory where the process was started. The
>>filename may also be a directory in which case, the
>> filename is
>>generated from the PID and the current date in the
>> specified
>>directory. (STRING, no default value)
>>
>>Note: If a filename is given, '%p' in the filename will be
>>replaced by the PID, and '%t' will be replaced by the
>> time in
>>'_MM_dd_HH_mm_ss' format.
>>
>>
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265),
>> sources for the jcmd manpage remain in Oracle internal repos so this PR
>> can’t address that.
>>
>> Testing:
>>
>> - [x] Added test case passes.
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames.
>>
>> Looking forward to your comments and addressing any diagnostic commands I
>> might have missed (if any).
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Adding FILE descriptor for help output
Thank you for improving argument handling in jcmd. Please fix small identation
problem. Also I would recommend to get approval from svc team reviewer.
src/hotspot/share/prims/wbtestmethods/parserTests.cpp line 132:
> 130:} else if (strcmp(type, "FILE") == 0) {
> 131: DCmdArgument* argument =
> 132: new DCmdArgument(name, desc, "FILE", mandatory);
Please update identation.
-
Marked as reviewed by lmesnik (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2200510671
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1692165047
Re: RFR: 8327054: DiagnosticCommand Compiler.perfmap does not log on output() [v4]
On Mon, 22 Jul 2024 15:36:47 GMT, Sonia Zaldana Calles wrote: >> Hi all, >> >> This is a small patch to address >> [8327054](https://bugs.openjdk.org/browse/JDK-8327054) making >> `CodeCache::write_perf_map` aware of which output stream errors and warning >> message should be going to. >> >> Testing: >> - [x] Added test case passes. >> >> Thanks, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Updating warning message Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20257#pullrequestreview-2192606124
Re: RFR: 8248609: [Graal] vmTestbase/nsk/jdi/VoidValue/toString/tostring001/TestDescription.java failed with Unexpected com.sun.jdi.ObjectCollectedException
On Thu, 18 Jul 2024 18:50:48 GMT, Chris Plummer wrote:
> The test is failing with an ObjectCollectedException. The test hits a
> SUSPEND_ALL breakpoint. It then uses JDI to allocate an Object on the
> debuggee side:
>
> testedObject = testedClass.newInstance(thread, ctor, params, 0);
>
> Since we are under a SUSPEND_ALL, the object is not initially at risk of
> getting GC'd. However, the test then calls invokeMethod() in a loop:
>
> if (method.isStatic()) {
> voidValue = (VoidValue) testedClass.invokeMethod(thread,
> method, params, 0);
> } else {
> voidValue = (VoidValue) testedObject.invokeMethod(thread,
> method, params, 0);
> }
>
> On the first iteration of the loop, invokeMethod() will do a resumeAll() so
> it can execute the method on the specified thread. During this time a GC can
> happen, and that GC is likely to collect the object that testedObject is
> mirroring since it is only weakly kept alive. Then on a subsequent iteration
> of the loop, testedObject.invokeMethod() is called again, but this time you
> get the ObjectCollectedException because the object that testedObject is
> mirroring has been collected. The test needs to add a call to
> testedObject.disableCollection() to prevent it from being collected.
>
> I'm not able to reproduce this failure, but the bug is pretty clear. Testing
> is in progress. I'll run tier1 CI and also tier2 and tier5 test tasks that
> run this test.
Marked as reviewed by lmesnik (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/20242#pullrequestreview-2189211759
Re: RFR: 8333391: Test com/sun/jdi/InterruptHangTest.java failed: Thread was never interrupted during sleep
On Fri, 19 Jul 2024 18:41:10 GMT, Chris Plummer wrote: > The test sometimes fails because the InterruptException doesn't arrive during > the 100ms that the thread sleeps. My suspicion is that the interrupt is being > delayed for some external reason, like temporary load on the host. I'm > bumping the sleep period to 10s to hopefully avoid such an issue. This won't > make the test run any slower when it passes, but will make it slower (by > about 10 seconds) when it fails due to waiting longer for the > InterruptException to never arrive, assuming it still might fail for this > reason. > > I tested by running the InterruptHangTest locally. TBD I will run all of > con/sun/jdi on all supported platforms. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20263#pullrequestreview-2189118504
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v4]
On Fri, 19 Jul 2024 14:03:43 GMT, Sonia Zaldana Calles wrote: > * Regarding warnings, I noted we wanted to issue any warnings to the issuer > of the dcmd and not the JVM process. However, in `diagnosticArgument.cpp`, > they are issuing the warnings directly to the JVM process. I tried to stay > consistent with how things are done there, but let me know what you think. > It makes sense to file separate issue for this and keep current behavior in the fix. - PR Comment: https://git.openjdk.org/jdk/pull/20198#issuecomment-2240037485
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]
On Fri, 19 Jul 2024 14:07:12 GMT, Sonia Zaldana Calles
wrote:
>> Hi all,
>>
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492)
>> enabling jcmd diagnostic commands that issue an output file to accept the
>> `%p` pattern in the file name and substitute it for the PID.
>>
>> This PR addresses the following diagnostic commands:
>> - [x] Compiler.perfmap
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>>
>> Note that some jcmd diagnostic commands already enable this functionality
>> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`).
>>
>> I propose opening a separate issue to track updating the man page similarly
>> to how it’s done for the JFR diagnostic commands. For example,
>>
>>
>> filename (Optional) Name of the file to which the flight recording
>> data is
>>written when the recording is stopped. If no filename is
>> given, a
>>filename is generated from the PID and the current date
>> and is
>>placed in the directory where the process was started. The
>>filename may also be a directory in which case, the
>> filename is
>>generated from the PID and the current date in the
>> specified
>>directory. (STRING, no default value)
>>
>>Note: If a filename is given, '%p' in the filename will be
>>replaced by the PID, and '%t' will be replaced by the
>> time in
>>'_MM_dd_HH_mm_ss' format.
>>
>>
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265),
>> sources for the jcmd manpage remain in Oracle internal repos so this PR
>> can’t address that.
>>
>> Testing:
>>
>> - [x] Added test case passes.
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames.
>>
>> Looking forward to your comments and addressing any diagnostic commands I
>> might have missed (if any).
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Missing copyright header update
Thanks for updating the fix. The new version looks moistly good. I added a few
small comments.
src/hotspot/share/prims/wbtestmethods/parserTests.cpp line 132:
> 130:} else if (strcmp(type, "FILE") == 0) {
> 131: DCmdArgument *argument =
> 132: new DCmdArgument(name, desc, "FILE", mandatory);
Please check indentation.
src/hotspot/share/services/diagnosticArgument.cpp line 358:
> 356: template <> void DCmdArgument::destroy_value() { }
> 357:
> 358: template <>
The common style here is to place in the single line 'template<> and other part
of declaration.
src/hotspot/share/services/diagnosticArgument.cpp line 366:
> 364: _value._name = NEW_C_HEAP_ARRAY(char, JVM_MAXPATHLEN, mtInternal);
> 365: if (!Arguments::copy_expand_pid(str, len, _value._name,
> JVM_MAXPATHLEN)) {
> 366: fatal("Invalid file path: %s", str);
As I understand the 'copy_expand_pid' might fail if very long line is used.
This cause jvm crash.,
So there is possibility that user might crash jvm accidentally invoking jcmd
command.
It doesn't look safe, I believe it would be better to throw Exception like for
any other invalid command, see
" THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),"
The 'fatal" owuld make sense only if failing of 'copy_expand_pid' means some
unrecoverable jvm bug.
-
Changes requested by lmesnik (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2189044201
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1684887604
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1684892964
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1684923626
Re: RFR: 8313235: TestClhsdbJstackLock.java failed with '^\s+- waiting to lock <0x[0-9a-f]+> \(a java\.lang\.Class for LingeredAppWithLock\)$' missing from stdout/stderr
On Fri, 28 Jun 2024 22:30:52 GMT, Chris Plummer wrote: > Once the main thread has detected that the spawned thread is in the BLOCKED > state, the spawned thread's LingeredAppWithLock.lockMethod() should be > visible on the top of the stack, but it is not, so the "waiting to lock" > message is missing from the stack trace. > > I think the explanations mentioned in > [JDK-8335124](https://bugs.openjdk.org/browse/JDK-8335124) and > [JDK-8269881](https://bugs.openjdk.org/browse/JDK-8269881) apply here also. > The state of the thread has moved to BLOCKED, but the thread still needs to > finish making an OS call to actually become blocked and the thread become > idle. During that time we may not be able to get an accurate stack trace. In > this case probably SP has not been flushed, so we are not seeing the > lockMethod() frame, which should appear at the top of the stack. > > A short delay has been added after all threads have entered the desired state > to make sure they have fully reached the desired state and are now idle. > > Tested with Tier1 CI and all svc test tasks for tier2 and tier5. Although I don't like using Thread.sleep(), seems there are no any other way to fir the problem. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19953#pullrequestreview-2184368963
Re: RFR: 8269881: SA stack dump fails to include stack trace for SteadyStateThread
On Fri, 28 Jun 2024 20:34:48 GMT, Chris Plummer wrote: > The completely unrelated fix to > [JDK-8335124](https://bugs.openjdk.org/browse/JDK-8335124) led me to believe > that the issue with sometimes not being able to get the stack trace of the > SteadyStateThread might be due to the thread being active for a short period > after being reported as in the Thread.State.BLOCKED state. Once set to that > state, the thread still needs to call a native OS API to block the thread so > it is truly idle. During this time the thread stack might be inconsistent and > not walk-able. The fix is to add a short sleep after the thread has moved to > the Thread.State.BLOCKED state to give it a chance to finish blocking. > > Tested with Tier1 CI and all svc test tasks for tier2 and tier5. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19951#pullrequestreview-2184371771
Re: RFR: 8334771: [TESTBUG] Run TestDockerMemoryMetrics.java with -Xcomp fails exitValue = 137
On Mon, 24 Jun 2024 16:16:29 GMT, SendaoYan wrote: > Hi all, > After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the > footprint memory usage increased significantly when run the testcase with > -Xcomp jvm options, then cause the testcase was killed by docker by OOM. > Maybe the footprint memory usage increased was inevitable, so I think we > should increase the smallest memory limite for this testcase. > Only change the testcase, the change has been verified, no risk. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19864#pullrequestreview-2184371193
Re: RFR: 8332551: Test vmTestbase/nsk/monitoring/MemoryNotificationInfo/from/from001/TestDescription.java timed out [v2]
On Fri, 12 Jul 2024 09:16:05 GMT, Kevin Walls wrote: >> Short version: >> Stop testing this test with -Xcomp by adding requires vm.compMode != "Xcomp" >> >> Make additional typo fixes and tidyups while here, it's just shocking. >> >> TestDescription.java contains the test definition, so the "requires" goes >> there, with a comment. >> >> Updates to from001.java are typos and clarifications, and a changed loop >> with a poll on a queue rather than block forever. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > formatting Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20146#pullrequestreview-2184355125
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v3]
On Wed, 17 Jul 2024 19:59:10 GMT, Sonia Zaldana Calles
wrote:
>> Hi all,
>>
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492)
>> enabling jcmd diagnostic commands that issue an output file to accept the
>> `%p` pattern in the file name and substitute it for the PID.
>>
>> This PR addresses the following diagnostic commands:
>> - [x] Compiler.perfmap
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>>
>> Note that some jcmd diagnostic commands already enable this functionality
>> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`).
>>
>> I propose opening a separate issue to track updating the man page similarly
>> to how it’s done for the JFR diagnostic commands. For example,
>>
>>
>> filename (Optional) Name of the file to which the flight recording
>> data is
>>written when the recording is stopped. If no filename is
>> given, a
>>filename is generated from the PID and the current date
>> and is
>>placed in the directory where the process was started. The
>>filename may also be a directory in which case, the
>> filename is
>>generated from the PID and the current date in the
>> specified
>>directory. (STRING, no default value)
>>
>>Note: If a filename is given, '%p' in the filename will be
>>replaced by the PID, and '%t' will be replaced by the
>> time in
>>'_MM_dd_HH_mm_ss' format.
>>
>>
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265),
>> sources for the jcmd manpage remain in Oracle internal repos so this PR
>> can’t address that.
>>
>> Testing:
>>
>> - [x] Added test case passes.
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames.
>>
>> Looking forward to your comments and addressing any diagnostic commands I
>> might have missed (if any).
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Updates based on feedback
Changes requested by lmesnik (Reviewer).
src/hotspot/share/code/codeCache.cpp line 1791:
> 1789:
> 1790: #ifdef LINUX
> 1791: void CodeCache::write_perf_map(const char* filename, outputStream* out)
> {
I don't think that it is a right place ti expand arguments here. I think it is
more consistent to do it in diagnosticCommand.cpp for any jcmd command. So
this logic might be the same for any _filename processing. Moreover it would be
better be add 'FileArgument' like 'MemorySizeArgument' that correctly parse
patterns like %p for all file arguments, used be all commands and be extensible
for new macroses if needed.
test/jdk/sun/tools/jcmd/TestJcmdPIDSubstitution.java line 32:
> 30: * @modules java.base/jdk.internal.misc
> 31: * java.management
> 32: * @run main/othervm TestJcmdPIDSubstitution
Why othervm is needed here? I suggest to add driver mode instead.
test/jdk/sun/tools/jcmd/TestJcmdPIDSubstitution.java line 59:
> 57: do {
> 58: path = Paths.get("myfile%d".formatted(pid));
> 59: } while(Files.exists(path));
Why this do/while loop is needed? Each test should have it's own empty scratch
directory.
-
PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2184333180
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681931084
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681921063
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681920781
Integrated: 8332252: Clean up vmTestbase/vm/share
On Fri, 14 Jun 2024 16:37:15 GMT, Leonid Mesnik wrote: > The vmTestbase/vm/share is a shared test library for vmTestbase tests. This > library contains a lot of code that is used by only by small number of tests > or not used at all. There are no plans to actively develop new tests in > vmTestsbase and improve this shared library. > The final goal of this and the following PRs is to reduce the maintenance > cost of vmTestbase by eliminating this library. > > Also, this PR moves test-specific code into corresponding test directories to > increase code locality. This allows later easier move tests from vmTestbase. > > The few remaining classes include > InMemoryJavaCompiler.java > that is very similar to same class from the standard testlibrary and could be > merge with it and > ProcessUtils.java > which is used by > test/hotspot/jtreg/runtime/Thread/TestBreakSignalThreadDump.java > and thus should be moved into the standard testlibrary. > The stack and options might be merged in nsk/share test library. This pull request has now been integrated. Changeset: a81e1bf1 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/a81e1bf1e1a6f00280b9be987c03fe20915fd52c Stats: 1669 lines in 45 files changed: 27 ins; 1595 del; 47 mod 8332252: Clean up vmTestbase/vm/share Reviewed-by: cjplummer - PR: https://git.openjdk.org/jdk/pull/19727
Integrated: 8333117: Remove support of remote and manual debuggee launchers
On Fri, 14 Jun 2024 22:08:17 GMT, Leonid Mesnik wrote: > The nsk jdi,jdb,jdwp test suites support remote and manual launchers that are > not used. > These launchers might be configured by test options, however no tests use > these options. > The remote launchers allow to run debugee on another host which is not > supported by jtreg and not seems useful for testing. The manual debuggee > launcher might be used to connect launch debuggee manually and also not used. > > These modes have never been used last 15 years as I know. > > So just removed a bunch of useless code. > > Also, I moved implementation of the single Debugee realization into Debugee > itself for jdi/jdwp/jdb. This pull request has now been integrated. Changeset: 99e4d77a Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/99e4d77aac72cdddb4973805d28c225f17ea965f Stats: 1575 lines in 10 files changed: 41 ins; 1470 del; 64 mod 8333117: Remove support of remote and manual debuggee launchers Reviewed-by: cjplummer - PR: https://git.openjdk.org/jdk/pull/19729
Re: RFR: 8332252: Clean up vmTestbase/vm/share [v4]
> The vmTestbase/vm/share is a shared test library for vmTestbase tests. This > library contains a lot of code that is used by only by small number of tests > or not used at all. There are no plans to actively develop new tests in > vmTestsbase and improve this shared library. > The final goal of this and the following PRs is to reduce the maintenance > cost of vmTestbase by eliminating this library. > > Also, this PR moves test-specific code into corresponding test directories to > increase code locality. This allows later easier move tests from vmTestbase. > > The few remaining classes include > InMemoryJavaCompiler.java > that is very similar to same class from the standard testlibrary and could be > merge with it and > ProcessUtils.java > which is used by > test/hotspot/jtreg/runtime/Thread/TestBreakSignalThreadDump.java > and thus should be moved into the standard testlibrary. > The stack and options might be merged in nsk/share test library. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed imports. - Changes: - all: https://git.openjdk.org/jdk/pull/19727/files - new: https://git.openjdk.org/jdk/pull/19727/files/53757d6f..12f63cc6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19727&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19727&range=02-03 Stats: 25 lines in 6 files changed: 11 ins; 9 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19727.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19727/head:pull/19727 PR: https://git.openjdk.org/jdk/pull/19727
Re: RFR: 8332252: Clean up vmTestbase/vm/share [v3]
> The vmTestbase/vm/share is a shared test library for vmTestbase tests. This > library contains a lot of code that is used by only by small number of tests > or not used at all. There are no plans to actively develop new tests in > vmTestsbase and improve this shared library. > The final goal of this and the following PRs is to reduce the maintenance > cost of vmTestbase by eliminating this library. > > Also, this PR moves test-specific code into corresponding test directories to > increase code locality. This allows later easier move tests from vmTestbase. > > The few remaining classes include > InMemoryJavaCompiler.java > that is very similar to same class from the standard testlibrary and could be > merge with it and > ProcessUtils.java > which is used by > test/hotspot/jtreg/runtime/Thread/TestBreakSignalThreadDump.java > and thus should be moved into the standard testlibrary. > The stack and options might be merged in nsk/share test library. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: removed unused import - Changes: - all: https://git.openjdk.org/jdk/pull/19727/files - new: https://git.openjdk.org/jdk/pull/19727/files/f8a637dc..53757d6f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19727&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19727&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19727.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19727/head:pull/19727 PR: https://git.openjdk.org/jdk/pull/19727
Re: RFR: 8333117: Remove support of remote and manual debuggee launchers [v3]
On Fri, 14 Jun 2024 23:04:05 GMT, Chris Plummer wrote: >> Leonid Mesnik has updated the pull request incrementally with one additional >> commit since the last revision: >> >> removed empty lines > > test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 78: > >> 76: >> 77: /** Need or not to check debuggee process termination. */ >> 78: private boolean checkTermination = true; > > What is the impact of this change to our current testing? The 'checkTermination' is set to true by and jdi/jdwp LocalDebugee implementation. So it should be always set to true initially. It is used to check process status and complain and kill debugee if the debugee process hasn't been finished by itself. I think it could be remove later, but don't want to change any logic now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19729#discussion_r1640477950
Re: RFR: 8333117: Remove support of remote and manual debuggee launchers [v3]
On Fri, 14 Jun 2024 22:38:33 GMT, Chris Plummer wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> removed empty lines
>
> test/hotspot/jtreg/vmTestbase/nsk/share/jdb/Launcher.java line 124:
>
>> 122: } else if (argumentHandler.isListeningConnector()) {
>> 123:
>> 124: localLaunchAndListen(jdbCmdArgs, classToExecute);
>
> I'd suggest getting rid of all the empty lines. Not sure why they were there
> in the first place.
done
> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeArgumentHandler.java line
> 699:
>
>> 697: || option.equals("debugee.host")
>> 698: || option.equals("test.host")) {
>> 699: throw new RuntimeException("debugee.host option is not
>> supported.");
>
> Suggestion:
>
> throw new RuntimeException(""" + option + "" option is not
> supported.");
fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19729#discussion_r1640457700
PR Review Comment: https://git.openjdk.org/jdk/pull/19729#discussion_r1640457884
Re: RFR: 8333117: Remove support of remote and manual debuggee launchers [v3]
> The nsk jdi,jdb,jdwp test suites support remote and manual launchers that are > not used. > These launchers might be configured by test options, however no tests use > these options. > The remote launchers allow to run debugee on another host which is not > supported by jtreg and not seems useful for testing. The manual debuggee > launcher might be used to connect launch debuggee manually and also not used. > > These modes have never been used last 15 years as I know. > > So just removed a bunch of useless code. > > Also, I moved implementation of the single Debugee realization into Debugee > itself for jdi/jdwp/jdb. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: removed empty lines - Changes: - all: https://git.openjdk.org/jdk/pull/19729/files - new: https://git.openjdk.org/jdk/pull/19729/files/f0e3c6ab..6f753be3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19729&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19729&range=01-02 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19729.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19729/head:pull/19729 PR: https://git.openjdk.org/jdk/pull/19729
Re: RFR: 8333117: Remove support of remote and manual debuggee launchers [v2]
> The nsk jdi,jdb,jdwp test suites support remote and manual launchers that are > not used. > These launchers might be configured by test options, however no tests use > these options. > The remote launchers allow to run debugee on another host which is not > supported by jtreg and not seems useful for testing. The manual debuggee > launcher might be used to connect launch debuggee manually and also not used. > > These modes have never been used last 15 years as I know. > > So just removed a bunch of useless code. > > Also, I moved implementation of the single Debugee realization into Debugee > itself for jdi/jdwp/jdb. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed error message - Changes: - all: https://git.openjdk.org/jdk/pull/19729/files - new: https://git.openjdk.org/jdk/pull/19729/files/4c0142ed..f0e3c6ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19729&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19729&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19729.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19729/head:pull/19729 PR: https://git.openjdk.org/jdk/pull/19729
RFR: 8333117: Remove support of remote and manual debuggee launchers
The nsk jdi,jdb,jdwp test suites support remote and manual launchers that are not used. These launchers might be configured by test options, however no tests use these options. The remote launchers allow to run debugee on another host which is not supported by jtreg and not seems useful for testing. The manual debuggee launcher might be used to connect launch debuggee manually and also not used. These modes have never been used last 15 years as I know. So just removed a bunch of useless code. - Commit messages: - 8333117: Remove support of remote and manual debuggee launchers Changes: https://git.openjdk.org/jdk/pull/19729/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19729&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333117 Stats: 1565 lines in 10 files changed: 41 ins; 1460 del; 64 mod Patch: https://git.openjdk.org/jdk/pull/19729.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19729/head:pull/19729 PR: https://git.openjdk.org/jdk/pull/19729
Re: RFR: 8332252: Clean up vmTestbase/vm/share [v2]
On Fri, 14 Jun 2024 19:42:21 GMT, Coleen Phillimore wrote: >> Leonid Mesnik has updated the pull request incrementally with one additional >> commit since the last revision: >> >> removed toHandle() > > test/hotspot/jtreg/vmTestbase/metaspace/share/TriggerUnloadingWithWhiteBox.java > line 23: > >> 21: * questions. >> 22: */ >> 23: package metaspace.share; > > There's a triggerUnloading call here: > > test/lib/jdk/test/lib/classloader/ClassUnloadCommon.java > > You might be able to also remove this file (and maybe the others) and use the > ClassUnloadCommon version. Thanks. I filed https://bugs.openjdk.org/browse/JDK-8334320 The functionality is little different so more testing might be required for changed tests. - PR Review Comment: https://git.openjdk.org/jdk/pull/19727#discussion_r1640315974
Re: RFR: 8332252: Clean up vmTestbase/vm/share [v2]
> The vmTestbase/vm/share is a shared test library for vmTestbase tests. This > library contains a lot of code that is used by only by small number of tests > or not used at all. There are no plans to actively develop new tests in > vmTestsbase and improve this shared library. > The final goal of this and the following PRs is to reduce the maintenance > cost of vmTestbase by eliminating this library. > > Also, this PR moves test-specific code into corresponding test directories to > increase code locality. This allows later easier move tests from vmTestbase. > > The few remaining classes include > InMemoryJavaCompiler.java > that is very similar to same class from the standard testlibrary and could be > merge with it and > ProcessUtils.java > which is used by > test/hotspot/jtreg/runtime/Thread/TestBreakSignalThreadDump.java > and thus should be moved into the standard testlibrary. > The stack and options might be merged in nsk/share test library. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: removed toHandle() - Changes: - all: https://git.openjdk.org/jdk/pull/19727/files - new: https://git.openjdk.org/jdk/pull/19727/files/275c9a00..f8a637dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19727&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19727&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19727.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19727/head:pull/19727 PR: https://git.openjdk.org/jdk/pull/19727
Re: RFR: 8332252: Clean up vmTestbase/vm/share
On Fri, 14 Jun 2024 19:51:29 GMT, Chris Plummer wrote: >> The vmTestbase/vm/share is a shared test library for vmTestbase tests. This >> library contains a lot of code that is used by only by small number of tests >> or not used at all. There are no plans to actively develop new tests in >> vmTestsbase and improve this shared library. >> The final goal of this and the following PRs is to reduce the maintenance >> cost of vmTestbase by eliminating this library. >> >> Also, this PR moves test-specific code into corresponding test directories >> to increase code locality. This allows later easier move tests from >> vmTestbase. >> >> The few remaining classes include >> InMemoryJavaCompiler.java >> that is very similar to same class from the standard testlibrary and could >> be merge with it and >> ProcessUtils.java >> which is used by >> test/hotspot/jtreg/runtime/Thread/TestBreakSignalThreadDump.java >> and thus should be moved into the standard testlibrary. >> The stack and options might be merged in nsk/share test library. > > test/hotspot/jtreg/vmTestbase/vm/compiler/complog/share/LogCompilationTest.java > line 32: > >> 30: import vm.share.options.Option; >> 31: import vm.share.options.OptionSupport; >> 32: import vm.share.process.ProcessExecutor; > > You got rid of this import, but ProcessExecutor is still referenced below. Is > this file even referenced during test execution? The ProcessExecutor has been moved into this package, so it is local package now. Double checked that it is used and tests jtreg:open/test/hotspot/jtreg/vmTestbase/vm/compiler/complog still pass. - PR Review Comment: https://git.openjdk.org/jdk/pull/19727#discussion_r1640299885
RFR: 8332252: Clean up vmTestbase/vm/share
The vmTestbase/vm/share is a shared test library for vmTestbase tests. This library contains a lot of code that is used by only by small number of tests or not used at all. There are no plans to actively develop new tests in vmTestsbase and improve this shared library. The final goal of this and the following PRs is to reduce the maintenance cost of vmTestbase by eliminating this library. Also, this PR moves test-specific code into corresponding test directories to increase code locality. This allows later easier move tests from vmTestbase. The few remaining classes include InMemoryJavaCompiler.java that is very similar to same class from the standard testlibrary and could be merge with it and ProcessUtils.java which is used by test/hotspot/jtreg/runtime/Thread/TestBreakSignalThreadDump.java and thus should be moved into the standard testlibrary. The stack and options might be merged in nsk/share test library. - Commit messages: - 8332252: Clean up vmTestbase/vm/share Changes: https://git.openjdk.org/jdk/pull/19727/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19727&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8332252 Stats: 1647 lines in 44 files changed: 17 ins; 1586 del; 44 mod Patch: https://git.openjdk.org/jdk/pull/19727.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19727/head:pull/19727 PR: https://git.openjdk.org/jdk/pull/19727
Integrated: 8332113: Update nsk.share.Log to be always verbose
On Sat, 8 Jun 2024 18:05:36 GMT, Leonid Mesnik wrote: > The nsk.share.Log has 3 parameters that might be configured by tests or > using command-line: > - verbose, traceLevel and timestamp > > The main purpose of these modes was to minimize output and use command-line > arguments to enable them during bug reproducing/debugging for vmTestbase when > it was compiled separately. > > However, such an approach has several disadvantages: > -- For intermittent issues, there is no all data in the logs > -- The enabling log might affect test behavior > -- No easy way to set these command-line options for jtreg tests > -- When verbose mode is disabled the messages are saved in some buffer and > printed only test complains. The mode causes issues if the test fails without > complaining (exception, crash, etc). The messages are just never printed. > -- the solution is over-complicated. > > The fix enabled verbose mode and printing time stamps always, setting the > debugging log level. > > The plan is to remove all these options and simplify logging as much as > possible in the future. This pull request has now been integrated. Changeset: 8464ce6d Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/8464ce6db5cbd5d50ac2a2bcba905b7255f510f5 Stats: 10 lines in 1 file changed: 0 ins; 4 del; 6 mod 8332113: Update nsk.share.Log to be always verbose Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/19613
Re: RFR: 8332113: Update nsk.share.Log to be always verbose [v4]
> The nsk.share.Log has 3 parameters that might be configured by tests or > using command-line: > - verbose, traceLevel and timestamp > > The main purpose of these modes was to minimize output and use command-line > arguments to enable them during bug reproducing/debugging for vmTestbase when > it was compiled separately. > > However, such an approach has several disadvantages: > -- For intermittent issues, there is no all data in the logs > -- The enabling log might affect test behavior > -- No easy way to set these command-line options for jtreg tests > -- When verbose mode is disabled the messages are saved in some buffer and > printed only test complains. The mode causes issues if the test fails without > complaining (exception, crash, etc). The messages are just never printed. > -- the solution is over-complicated. > > The fix enabled verbose mode and printing time stamps always, setting the > debugging log level. > > The plan is to remove all these options and simplify logging as much as > possible in the future. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: the unused code removed - Changes: - all: https://git.openjdk.org/jdk/pull/19613/files - new: https://git.openjdk.org/jdk/pull/19613/files/0434e815..8a966242 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19613&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19613&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19613.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19613/head:pull/19613 PR: https://git.openjdk.org/jdk/pull/19613
Re: RFR: 8332113: Update nsk.share.Log to be always verbose [v3]
On Fri, 14 Jun 2024 03:09:26 GMT, Chris Plummer wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> reverted traceLevel
>
> test/hotspot/jtreg/vmTestbase/nsk/share/Log.java line 226:
>
>> 224: if (!verbose) {
>> 225: flushLogBuffer();
>> 226: }
>
> flushLogBuffer() is never executed. I think you can remove it.
fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19613#discussion_r1639188317
Re: RFR: 8332113: Update nsk.share.Log to be always verbose [v3]
On Fri, 14 Jun 2024 00:31:41 GMT, Chris Plummer wrote: >> The intention is to print always all logged messages. So we should set the >> highest tracing level for the logger to work in debug mode. > > I'm not so sure I agree with this. Do we have examples of debug logging? I > think in general verbose logging has actually been used to produced a log of > useful info, so always logging in verbose mode seems like a good idea, but I > see debug logging as something beyond that, and may get too noisy. That's quite an ineteresting thing. I haven't find any trace usage in tests and thought that it is never used. However, it is used actually by Binder and SocketConnection. And with completely different levels. The log has class TraceLevel: public static final int TRACE_NONE = 0; public static final int TRACE_IMPORTANT = 1; public static final int TRACE_NORMAL = 2; public static final int TRACE_VERBOSE = 3; public static final int TRACE_DEBUG = 4; while Binder and SocketConnection use their own trace levels private static int TRACE_LEVEL_PACKETS = 10; private static int TRACE_LEVEL_THREADS = 20; private static int TRACE_LEVEL_ACTIONS = 30; private static int TRACE_LEVEL_SOCKETS = 40; private static int TRACE_LEVEL_IO = 50; So even with my fix I haven't seen any changes. I'll left traceLevel changes for separate PR, need to move them out of Log into Binder/Connections. They might be useful to debug packets/connection but not needed to be always enabled. - PR Review Comment: https://git.openjdk.org/jdk/pull/19613#discussion_r1639083715
Re: RFR: 8332113: Update nsk.share.Log to be always verbose [v3]
> The nsk.share.Log has 3 parameters that might be configured by tests or > using command-line: > - verbose, traceLevel and timestamp > > The main purpose of these modes was to minimize output and use command-line > arguments to enable them during bug reproducing/debugging for vmTestbase when > it was compiled separately. > > However, such an approach has several disadvantages: > -- For intermittent issues, there is no all data in the logs > -- The enabling log might affect test behavior > -- No easy way to set these command-line options for jtreg tests > -- When verbose mode is disabled the messages are saved in some buffer and > printed only test complains. The mode causes issues if the test fails without > complaining (exception, crash, etc). The messages are just never printed. > -- the solution is over-complicated. > > The fix enabled verbose mode and printing time stamps always, setting the > debugging log level. > > The plan is to remove all these options and simplify logging as much as > possible in the future. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: reverted traceLevel - Changes: - all: https://git.openjdk.org/jdk/pull/19613/files - new: https://git.openjdk.org/jdk/pull/19613/files/ee7790ba..0434e815 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19613&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19613&range=01-02 Stats: 10 lines in 1 file changed: 9 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19613.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19613/head:pull/19613 PR: https://git.openjdk.org/jdk/pull/19613
Integrated: 8330534: Update nsk/jdwp tests to use driver instead of othervm
On Wed, 17 Apr 2024 20:19:49 GMT, Leonid Mesnik wrote: > The jdwp tests use debugger and debugee. There is no goal to execute debugger > part with all VM flags, they are needed to be used with debugee VM only. > The change is all tests is to don't use System.exit() and use 'driver' > instead of othervm. > test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java > is updated to correctly set classpath for debugee This pull request has now been integrated. Changeset: 56e8e607 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/56e8e60792b23bc101f46b497dcc9d3c76855384 Stats: 824 lines in 219 files changed: 342 ins; 0 del; 482 mod 8330534: Update nsk/jdwp tests to use driver instead of othervm Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/18826
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm [v4]
> The jdwp tests use debugger and debugee. There is no goal to execute debugger > part with all VM flags, they are needed to be used with debugee VM only. > The change is all tests is to don't use System.exit() and use 'driver' > instead of othervm. > test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java > is updated to correctly set classpath for debugee Leonid Mesnik 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 five additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into 8330534 - reverted jdi - jdi cleanup. - identation updated. - 8330534: Update nsk/jdwp tests to use driver instead of othervm - Changes: - all: https://git.openjdk.org/jdk/pull/18826/files - new: https://git.openjdk.org/jdk/pull/18826/files/842ce512..c38eb004 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18826&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18826&range=02-03 Stats: 211213 lines in 4196 files changed: 123140 ins; 67888 del; 20185 mod Patch: https://git.openjdk.org/jdk/pull/18826.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18826/head:pull/18826 PR: https://git.openjdk.org/jdk/pull/18826
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm [v3]
On Tue, 11 Jun 2024 18:38:31 GMT, Leonid Mesnik wrote: >> The jdwp tests use debugger and debugee. There is no goal to execute >> debugger part with all VM flags, they are needed to be used with debugee VM >> only. >> The change is all tests is to don't use System.exit() and use 'driver' >> instead of othervm. >> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java >> is updated to correctly set classpath for debugee > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > jdi cleanup. Thanks! I've updated also 'jdi' tests by this space deletion. Reverted the changes, the PR is clean now. I'll update jdi tests separately. - PR Comment: https://git.openjdk.org/jdk/pull/18826#issuecomment-2161397922
Integrated: 8333841: Add more logging into setfldw001 tests
On Sat, 8 Jun 2024 17:00:04 GMT, Leonid Mesnik wrote: > Tests > SetFieldAccessWatch/setfldw001 > SetFieldModificationWatch/setfmodw001 > intermittently fail with Xcomp. I was unable to reproduce the problem. > The fix adds more checks and variants with jvmti logging. > > The goal is to understand why the test fails. > 1. Confirm that the event is not sent. Currently, the test doesn't differ > between sending "NULL" event and not sending an event at all. > 2. Check if the interpreter-only mode is switched too late in Thread-1. The > jvmti logging shows when field events are enabled and when each thread > actually switches to interpreter-only and enables event handling. > > The plan is to try to reproduce the failure and remove '@test id=logging' > after https://bugs.openjdk.org/browse/JDK-8205957 is fixed. This pull request has now been integrated. Changeset: 7ed8a5c4 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/7ed8a5c431e1cba34167896f8d331caf594852ef Stats: 35 lines in 5 files changed: 31 ins; 2 del; 2 mod 8333841: Add more logging into setfldw001 tests Reviewed-by: cjplummer, amenkov, sspitsyn - PR: https://git.openjdk.org/jdk/pull/19612
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm [v3]
> The jdwp tests use debugger and debugee. There is no goal to execute debugger > part with all VM flags, they are needed to be used with debugee VM only. > The change is all tests is to don't use System.exit() and use 'driver' > instead of othervm. > test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java > is updated to correctly set classpath for debugee Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: jdi cleanup. - Changes: - all: https://git.openjdk.org/jdk/pull/18826/files - new: https://git.openjdk.org/jdk/pull/18826/files/efd5a7db..842ce512 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18826&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18826&range=01-02 Stats: 635 lines in 634 files changed: 0 ins; 0 del; 635 mod Patch: https://git.openjdk.org/jdk/pull/18826.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18826/head:pull/18826 PR: https://git.openjdk.org/jdk/pull/18826
RFR: 8333841: Add more logging into setfldw001 tests.
Tests SetFieldAccessWatch/setfldw001 SetFieldModificationWatch/setfmodw001 intermittently fail with Xcomp. I was unable to reproduce the problem. The fix adds more checks and variants with jvmti logging. The goal is to understand why the test fails. 1. Confirm that the event is not sent. Currently, the test doesn't differ between sending "NULL" event and not sending an event at all. 2. Check if the interpreter-only mode is switched too late in Thread-1. The jvmti logging shows when field events are enabled and when each thread actually switches to interpreter-only and enables event handling. The plan is to try to reproduce the failure and remove '@test id=logging' after https://bugs.openjdk.org/browse/JDK-8205957 is fixed. - Commit messages: - 8333841: Add more logging into setfldw001 tests. Changes: https://git.openjdk.org/jdk/pull/19612/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19612&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333841 Stats: 35 lines in 5 files changed: 31 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19612.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19612/head:pull/19612 PR: https://git.openjdk.org/jdk/pull/19612
Re: RFR: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
On Thu, 6 Jun 2024 02:12:11 GMT, Alex Menkov wrote: > The fix updates com/sun/tools/attach/BasicTests.java to listen and connect > using loopback addresses > > Testing: run the test on all Oracle-supported platforms Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19571#pullrequestreview-2105081330
Integrated: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
On Tue, 4 Jun 2024 17:25:04 GMT, Leonid Mesnik wrote: > The kill sends async exception that is thrown somewhere during > log.display(...) method. > The log shows that it is thrown while PrintStream locking moving it into an > inconsistent state. > So the fix is to use some method that could be safely interrupted by async > exception. This pull request has now been integrated. Changeset: f73922b2 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/f73922b27d126314fc3127ee25aa40b6258c8a6b Stats: 14 lines in 1 file changed: 11 ins; 0 del; 3 mod 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1 Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/19547
Re: RFR: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
On Tue, 4 Jun 2024 21:16:08 GMT, Serguei Spitsyn wrote:
>> The empty method is removed. So test failing with '-Xcomp /C2' and exception
>> happens after try block.
>>
>> The log shows:
>> reply[2]: main[1]
>> Sending command: cont
>> reply[0]: >
>> reply[1]: Exception occurred: java.lang.NullPointerException (to be caught
>> at: nsk.jdb.kill.kill001.MyThread.run(), line=165
>> bci=107)"thread=MyThread-1", nsk.jdb.kill.kill001.MyThread.run(), line=164
>> bci=100
>> reply[2]: 164methodForException();
>> reply[3]:
>> reply[4]: MyThread-1[1]
>> Sending command: cont
>> reply[0]: >
>> reply[1]: Exception occurred: nsk.jdb.kill.kill001.MyException (uncaught)
>> reply[2]: Exception occurred: nsk.jdb.kill.kill001.MyException
>> (uncaught)"thread=MyThread-4", nsk.jdb.kill.kill001.MyThread.run(), line=178
>> bci=187
>> reply[3]: 178kill001a.log.display(ThreadFinished);
>> reply[4]:
>> reply[5]: MyThread-4[1]
>> Sending command: cont
>> reply[0]: >
>> reply[1]: Exception occurred: com.sun.jdi.IncompatibleThreadStateException
>> (uncaught)
>> reply[2]: Exception occurred: com.sun.jdi.IncompatibleThreadStateException
>> (uncaught)"thread=MyThread-3", nsk.share.Log.display(), line=327 bci=9
>> reply[3]: 327doPrint(message.toString());
>> reply[4]:
>> reply[5]: MyThread-3[1]
>> Sending command: cont
>> reply[0]: > Thread MyThread-1 caught expected async exception:
>> java.lang.NullPointerException: kill001a's Exception
>> reply[1]: Thread finished: MyThread-1
>> reply[2]:
>> reply[3]: Exception occurred: java.lang.ThreadDeath (uncaught)
>> reply[4]: Exception occurred: java.lang.ThreadDeath
>> (uncaught)"thread=MyThread-0", nsk.share.Log.doPrint(), line=495 bci=1
>> reply[5]: 495PrintStream stream = findOutStream();
>> reply[6]:
>> reply[7]: MyThread-0[1]
>> Sending command: cont
>> reply[0]: >
>> reply[1]: Exception occurred: java.lang.SecurityException (uncaught)
>> reply[2]: Exception occurred: java.lang.SecurityException
>> (uncaught)"thread=MyThread-2", nsk.share.Log.doPrint(), line=495 bci=1
>> reply[3]: 495PrintStream stream = findOutStream();
>
> Empty method could work but the JIT compiler can optimize it out with
> inlining.
> But the loop is not needed. Something like this could work:
>
>static public int trash;
> void methodForException() {
> trash = 10;
> }
>
> But I'm not sure if the static variable needs to be used outside of this
> method. I'm afraid, that Escape Analysis (EA) can spoil this. I wonder if
> `Thread.yield()`, Thread.sleep(), or something alike can be used here instead
> of `methodForException()`.
I'm afraid it could be just inlined into try block. The public static variables
might be read globally and should be optimized. However, really safer is jut to
use them explicitly.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19547#discussion_r1626732823
Re: RFR: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
On Tue, 4 Jun 2024 19:06:19 GMT, Chris Plummer wrote: >> The kill sends async exception that is thrown somewhere during >> log.display(...) method. >> The log shows that it is thrown while PrintStream locking moving it into an >> inconsistent state. >> So the fix is to use some method that could be safely interrupted by async >> exception. > > test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001a.java line 153: > >> 151: } >> 152: } >> 153: > > Have you tried an empty method? I don't think it's a matter of how much time > you spend in the method, but just whether or not a JVM async exception > polling point is reached. I suspect the method call or return will be a > polling point, but I'm unsure what happens if the method call is elided by > the JIT because it does nothing. The empty method is removed. So test failing with '-Xcomp /C2' and exception happens after try block. The log shows: reply[2]: main[1] Sending command: cont reply[0]: > reply[1]: Exception occurred: java.lang.NullPointerException (to be caught at: nsk.jdb.kill.kill001.MyThread.run(), line=165 bci=107)"thread=MyThread-1", nsk.jdb.kill.kill001.MyThread.run(), line=164 bci=100 reply[2]: 164methodForException(); reply[3]: reply[4]: MyThread-1[1] Sending command: cont reply[0]: > reply[1]: Exception occurred: nsk.jdb.kill.kill001.MyException (uncaught) reply[2]: Exception occurred: nsk.jdb.kill.kill001.MyException (uncaught)"thread=MyThread-4", nsk.jdb.kill.kill001.MyThread.run(), line=178 bci=187 reply[3]: 178kill001a.log.display(ThreadFinished); reply[4]: reply[5]: MyThread-4[1] Sending command: cont reply[0]: > reply[1]: Exception occurred: com.sun.jdi.IncompatibleThreadStateException (uncaught) reply[2]: Exception occurred: com.sun.jdi.IncompatibleThreadStateException (uncaught)"thread=MyThread-3", nsk.share.Log.display(), line=327 bci=9 reply[3]: 327doPrint(message.toString()); reply[4]: reply[5]: MyThread-3[1] Sending command: cont reply[0]: > Thread MyThread-1 caught expected async exception: java.lang.NullPointerException: kill001a's Exception reply[1]: Thread finished: MyThread-1 reply[2]: reply[3]: Exception occurred: java.lang.ThreadDeath (uncaught) reply[4]: Exception occurred: java.lang.ThreadDeath (uncaught)"thread=MyThread-0", nsk.share.Log.doPrint(), line=495 bci=1 reply[5]: 495PrintStream stream = findOutStream(); reply[6]: reply[7]: MyThread-0[1] Sending command: cont reply[0]: > reply[1]: Exception occurred: java.lang.SecurityException (uncaught) reply[2]: Exception occurred: java.lang.SecurityException (uncaught)"thread=MyThread-2", nsk.share.Log.doPrint(), line=495 bci=1 reply[3]: 495PrintStream stream = findOutStream(); - PR Review Comment: https://git.openjdk.org/jdk/pull/19547#discussion_r1626603001
Integrated: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share
On Fri, 31 May 2024 18:22:47 GMT, Leonid Mesnik wrote: > The fix removes finalization cleanup from vmTestbase. > The last to classes that use it are: DebugeeBinder and SocketIOPipe. > The DebugeeBinder is used in jdi and jdwp tests and is always linked with > debuggee process. So the DebugeeProcess.waitFor() is the good place to close > binder and free all it's resources. > The SocketIOPipe is used directly in AOD tests where it should be closed > after test execution. > > The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it is > connected directly in tests. However is also connected with debuggee and > could be closed in DebugeeProcess.waitFor(). > > The VMOutOfMemoryException001 test is fixed to release some memory after > throwing OOME so Sytem.exit() could complete successfully. Previously some > memory freed during VM shutdown hook. > > I verified that cleanup printed that corresponding 'close' method has been > already called before VM shutdown phase for debugger process. > Additionally, run all vmTestbase tests to verify there are no failures, This pull request has now been integrated. Changeset: 244f6ac2 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/244f6ac222fa98fba4fb99bf5bccd36e3e6c5de1 Stats: 314 lines in 10 files changed: 24 ins; 271 del; 19 mod 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/19505
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v4]
> The fix removes finalization cleanup from vmTestbase. > The last to classes that use it are: DebugeeBinder and SocketIOPipe. > The DebugeeBinder is used in jdi and jdwp tests and is always linked with > debuggee process. So the DebugeeProcess.waitFor() is the good place to close > binder and free all it's resources. > The SocketIOPipe is used directly in AOD tests where it should be closed > after test execution. > > The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it is > connected directly in tests. However is also connected with debuggee and > could be closed in DebugeeProcess.waitFor(). > > The VMOutOfMemoryException001 test is fixed to release some memory after > throwing OOME so Sytem.exit() could complete successfully. Previously some > memory freed during VM shutdown hook. > > I verified that cleanup printed that corresponding 'close' method has been > already called before VM shutdown phase for debugger process. > Additionally, run all vmTestbase tests to verify there are no failures, Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: added message - Changes: - all: https://git.openjdk.org/jdk/pull/19505/files - new: https://git.openjdk.org/jdk/pull/19505/files/6b051e41..2b877797 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19505&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19505&range=02-03 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19505/head:pull/19505 PR: https://git.openjdk.org/jdk/pull/19505
RFR: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
The kill sends async exception that is thrown somewhere during log.display(...) method. The log shows that it is thrown while PrintStream locking moving it into an inconsistent state. So the fix is to use some method that could be safely interrupted by async exception. - Commit messages: - dot added. - 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1 Changes: https://git.openjdk.org/jdk/pull/19547/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19547&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333235 Stats: 14 lines in 1 file changed: 11 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19547.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19547/head:pull/19547 PR: https://git.openjdk.org/jdk/pull/19547
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v2]
On Mon, 3 Jun 2024 22:08:46 GMT, Chris Plummer wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed try/finally
>
> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 201:
>
>> 199: try {
>> 200: int exitCode = waitForDebugee();
>> 201: return exitCode;
>
> I don't think I've ever run across a try block with a return statement
> before, especially when there is also a finally block. The reader is likely
> to miss the fact that before the return is done the finally block is
> executed. It's also odd because now there is no return statement at the end
> of the method. Although the compiler is smart enough to recognize that this
> is ok, it is another point of confusion for the reader. Any reason not to
> just instead do the return at the end of the method?
Yes, the code become too unreadable. Moved return out of try/catch.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1625141478
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v3]
On Fri, 31 May 2024 19:58:34 GMT, Chris Plummer wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> moved return out of try/catch
>
> test/hotspot/jtreg/vmTestbase/nsk/share/aod/DummyTargetApplication.java line
> 68:
>
>> 66: if ((signal == null) ||
>> !signal.equals(AODTestRunner.SIGNAL_FINISH))
>> 67: throw new TestBug("Unexpected signal: '" + signal + "'");
>> 68: pipe.close();
>
> Exceptions can be thrown before you get here.
Thanks, updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1623281376
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v3]
> The fix removes finalization cleanup from vmTestbase. > The last to classes that use it are: DebugeeBinder and SocketIOPipe. > The DebugeeBinder is used in jdi and jdwp tests and is always linked with > debuggee process. So the DebugeeProcess.waitFor() is the good place to close > binder and free all it's resources. > The SocketIOPipe is used directly in AOD tests where it should be closed > after test execution. > > The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it is > connected directly in tests. However is also connected with debuggee and > could be closed in DebugeeProcess.waitFor(). > > The VMOutOfMemoryException001 test is fixed to release some memory after > throwing OOME so Sytem.exit() could complete successfully. Previously some > memory freed during VM shutdown hook. > > I verified that cleanup printed that corresponding 'close' method has been > already called before VM shutdown phase for debugger process. > Additionally, run all vmTestbase tests to verify there are no failures, Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: moved return out of try/catch - Changes: - all: https://git.openjdk.org/jdk/pull/19505/files - new: https://git.openjdk.org/jdk/pull/19505/files/0ce8772f..6b051e41 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19505&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19505&range=01-02 Stats: 4 lines in 1 file changed: 2 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19505/head:pull/19505 PR: https://git.openjdk.org/jdk/pull/19505
Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v5]
On Wed, 29 May 2024 01:18:57 GMT, Serguei Spitsyn wrote:
>> Leonid Mesnik has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - fixed space.
>> - The result is updated.
>
> src/hotspot/share/prims/jvmtiTrace.cpp line 284:
>
>> 282: JavaThreadState current_state =
>> JavaThread::cast(Thread::current())->thread_state();
>> 283: if (current_state == _thread_in_native || current_state ==
>> _thread_blocked) {
>> 284: return "not readable";
>
> Nit: I'd suggest to make it more detailed, something like like this:
> "" or ""
@sspitsyn, @dholmes-ora Thanks for the naming suggestion, looks to long in the
report. Let me try to use logging and see if it makes sense to make more
improvements.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19275#discussion_r1623691234
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v2]
> The fix removes finalization cleanup from vmTestbase. > The last to classes that use it are: DebugeeBinder and SocketIOPipe. > The DebugeeBinder is used in jdi and jdwp tests and is always linked with > debuggee process. So the DebugeeProcess.waitFor() is the good place to close > binder and free all it's resources. > The SocketIOPipe is used directly in AOD tests where it should be closed > after test execution. > > The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it is > connected directly in tests. However is also connected with debuggee and > could be closed in DebugeeProcess.waitFor(). > > The VMOutOfMemoryException001 test is fixed to release some memory after > throwing OOME so Sytem.exit() could complete successfully. Previously some > memory freed during VM shutdown hook. > > I verified that cleanup printed that corresponding 'close' method has been > already called before VM shutdown phase for debugger process. > Additionally, run all vmTestbase tests to verify there are no failures, Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed try/finally - Changes: - all: https://git.openjdk.org/jdk/pull/19505/files - new: https://git.openjdk.org/jdk/pull/19505/files/151a36d5..0ce8772f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19505&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19505&range=00-01 Stats: 42 lines in 2 files changed: 20 ins; 12 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/19505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19505/head:pull/19505 PR: https://git.openjdk.org/jdk/pull/19505
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share
On Fri, 31 May 2024 19:55:56 GMT, Chris Plummer wrote:
>> The fix removes finalization cleanup from vmTestbase.
>> The last to classes that use it are: DebugeeBinder and SocketIOPipe.
>> The DebugeeBinder is used in jdi and jdwp tests and is always linked with
>> debuggee process. So the DebugeeProcess.waitFor() is the good place to close
>> binder and free all it's resources.
>> The SocketIOPipe is used directly in AOD tests where it should be closed
>> after test execution.
>>
>> The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it
>> is connected directly in tests. However is also connected with debuggee and
>> could be closed in DebugeeProcess.waitFor().
>>
>> The VMOutOfMemoryException001 test is fixed to release some memory after
>> throwing OOME so Sytem.exit() could complete successfully. Previously some
>> memory freed during VM shutdown hook.
>>
>> I verified that cleanup printed that corresponding 'close' method has been
>> already called before VM shutdown phase for debugger process.
>> Additionally, run all vmTestbase tests to verify there are no failures,
>
> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 215:
>
>> 213: if (binder != null) {
>> 214: binder.close();
>> 215: }
>
> Won't this be skipped if there is an exception during `waitForDebugee` or
> `waitForRedirectors`?
Really, this all code is not going to work if any exception thrown.
The adding
The process is not destroyed, and
the 'waitForRedirectors' is skipped in exception in waitForDebugee happens.
The 'waitForRedirectors' tries to close 3 redirectors and fail to close all of
them if exception happens.
So all failure handling logic should be updated.
I changed it to try/finally so it close pipe, binder and destroy process.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1623281071
