Re: RFR: 8341692: Implement JEP 490: ZGC: Remove the Non-Generational Mode [v2]
On Wed, 9 Oct 2024 12:57:36 GMT, Axel Boldt-Christmas wrote: >> This is the implementation task for `JEP 490: ZGC: Remove the >> Non-Generational Mode`. See the JEP for details. >> [JDK-8335850](https://bugs.openjdk.org/browse/JDK-8335850) > > Axel Boldt-Christmas has updated the pull request incrementally with six > additional commits since the last revision: > > - LargeWindowPaintTest.java fix id typo > - Fix problem-listed @requires typo > - Fix @requires !vm.gc.Z, must use vm.gc != "Z" > - Reorder z_globals options: product > diagnostic product > develop > - Consistent albite special code style > - Consistent order between ZArguments and GCArguments Looks good to me! - PR Comment: https://git.openjdk.org/jdk/pull/21401#issuecomment-2402271738
Re: RFR: 8341692: Implement JEP 490: ZGC: Remove the Non-Generational Mode [v2]
On Wed, 9 Oct 2024 12:57:36 GMT, Axel Boldt-Christmas
wrote:
>> This is the implementation task for `JEP 490: ZGC: Remove the
>> Non-Generational Mode`. See the JEP for details.
>> [JDK-8335850](https://bugs.openjdk.org/browse/JDK-8335850)
>
> Axel Boldt-Christmas has updated the pull request incrementally with six
> additional commits since the last revision:
>
> - LargeWindowPaintTest.java fix id typo
> - Fix problem-listed @requires typo
> - Fix @requires !vm.gc.Z, must use vm.gc != "Z"
> - Reorder z_globals options: product > diagnostic product > develop
> - Consistent albite special code style
> - Consistent order between ZArguments and GCArguments
src/hotspot/share/runtime/arguments.cpp line 523:
> 521:
> 522: { "MetaspaceReclaimPolicy", JDK_Version::undefined(),
> JDK_Version::jdk(21), JDK_Version::undefined() },
> 523: { "ZGenerational",JDK_Version::jdk(23),
> JDK_Version::jdk(24), JDK_Version::undefined() },
FTR: This line depends on what version the JEP gets targeted to.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21401#discussion_r1793510168
RFR: 8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
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 - Commit messages: - 8338139: The MXBean isVerbose() is always on when Xlog is enabled Changes: https://git.openjdk.org/jdk/pull/20628/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20628&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8338139 Stats: 195 lines in 6 files changed: 189 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/20628.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20628/head:pull/20628 PR: https://git.openjdk.org/jdk/pull/20628
Re: RFR: 8332494: java/util/zip/EntryCount64k.java failing with java.lang.RuntimeException: '\\A\\Z' missing from stderr [v2]
On Mon, 20 May 2024 07:24:16 GMT, Axel Boldt-Christmas wrote: >> Improve `java/util/zip/EntryCount64k.java` stderr parsing by ignoring >> deprecated warnings. Testing non-generational ZGC requires the use of a >> deprecated option. > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19297#pullrequestreview-2065538663
Re: RFR: 8332495: java/util/logging/LoggingDeadlock2.java fails with AssertionError: Some tests failed [v2]
On Mon, 20 May 2024 07:22:49 GMT, Axel Boldt-Christmas wrote: >> Improve ` java/util/logging/LoggingDeadlock2.java` stderr parsing by >> ignoring deprecated warnings. Testing non-generational ZGC requires the use >> of a deprecated option. > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19298#pullrequestreview-2065533348
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v3]
On Wed, 13 Mar 2024 20:10:25 GMT, Roger Riggs wrote: >> The intermittent failure of ObjectStreamClassCaching is due to an incorrect >> assumption about GC behavior and a race condition. >> >> Removed test based on incorrect assumptions about simultaneous clearing of >> WeakReferences. >> Added a run of the test using ZGC, (previously omitted) > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Use the correct ZGenerational collector Looks good. Make sure that the test has run after the -XX:+ZGenerational flag was added. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1935146818
Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v2]
On Wed, 13 Mar 2024 19:41:25 GMT, Roger Riggs wrote: >> The intermittent failure of ObjectStreamClassCaching is due to an incorrect >> assumption about GC behavior and a race condition. >> >> Removed test based on incorrect assumptions about simultaneous clearing of >> WeakReferences. >> Added a run of the test using ZGC, (previously omitted) > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove obsolete note; no longer disabled Changes requested by stefank (Reviewer). test/jdk/java/io/ObjectStreamClass/ObjectStreamClassCaching.java line 57: > 55: * @library /test/lib/ > 56: * @summary ObjectStreamClass caches keep ClassLoaders alive (ZGC) > 57: * @run testng/othervm -Xmx64m -XX:+UseZGC ObjectStreamClassCaching This test is meant to run with Generational ZGC (according to the requires line). It needs to run with `-XX:+UseZGC -XX:+ZGenerational` to enable Generational ZGC. If you run with `-XX:+UseZGC` you get the older, non-generational ZGC. - PR Review: https://git.openjdk.org/jdk/pull/18284#pullrequestreview-1935079550 PR Review Comment: https://git.openjdk.org/jdk/pull/18284#discussion_r1523833917
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v4]
> Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Update copyright years - Changes: - all: https://git.openjdk.org/jdk/pull/16807/files - new: https://git.openjdk.org/jdk/pull/16807/files/9a43b2c9..4fb2fc21 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v3]
On Fri, 12 Jan 2024 01:10:49 GMT, Leonid Mesnik wrote: > Needed to update copyrights now. Even when the code was written in 2023? - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1888699636
Re: RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/17344#issuecomment-1884845496
Integrated: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. This pull request has now been integrated. Changeset: ec385057 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/ec38505720251ceefc8e838bd68b740d166c83c1 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups Reviewed-by: dholmes, shade, tschatzl - PR: https://git.openjdk.org/jdk/pull/17344
RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups file still has a reference to it. This causes problems in our CI pipeline. - Commit messages: - Revert unrelated changes - 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups Changes: https://git.openjdk.org/jdk/pull/17344/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17344&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323508 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17344.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17344/head:pull/17344 PR: https://git.openjdk.org/jdk/pull/17344
Re: RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. Thanks. The unrelated change has been reverted. - PR Comment: https://git.openjdk.org/jdk/pull/17344#issuecomment-1884749086
Re: RFR: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable [v2]
On Fri, 5 Jan 2024 08:22:41 GMT, Stefan Karlsson wrote: >> Most functions in ProcessTools are declared to throw Exceptions, or one of >> its subclasses. However, there are a small number of functions that are >> unnecessarily declared to throw Throwable instead of Exception. I propose >> that we change them to also be declared to throw Exception. >> >> This is a trivial patch to make it easier to refactor tests to use the >> updated functions. >> >> Tested manually, but will wait for GHA to verify that the change is OK. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Copyright year Thanks for the reviews. Testing with Tier1-3 passes. - PR Comment: https://git.openjdk.org/jdk/pull/17240#issuecomment-1878341513
Integrated: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable
On Wed, 3 Jan 2024 09:51:24 GMT, Stefan Karlsson wrote: > Most functions in ProcessTools are declared to throw Exceptions, or one of > its subclasses. However, there are a small number of functions that are > unnecessarily declared to throw Throwable instead of Exception. I propose > that we change them to also be declared to throw Exception. > > This is a trivial patch to make it easier to refactor tests to use the > updated functions. > > Tested manually, but will wait for GHA to verify that the change is OK. This pull request has now been integrated. Changeset: 868f8745 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/868f8745faf70c915d8294ae8f85b2d6aa096900 Stats: 6 lines in 1 file changed: 0 ins; 2 del; 4 mod 8322920: Some ProcessTools.execute* functions are declared to throw Throwable Reviewed-by: dholmes, lmesnik - PR: https://git.openjdk.org/jdk/pull/17240
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v3]
> Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. Stefan Karlsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge remote-tracking branch 'upstream/master' into 8320699_OutputAnalyzer_progress_logging - Update OutputBuffer.java copyright years - 8320699: Add parameter to skip progress logging of OutputAnalyzer - Changes: https://git.openjdk.org/jdk/pull/16807/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=02 Stats: 24 lines in 2 files changed: 21 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
Re: RFR: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable [v2]
> Most functions in ProcessTools are declared to throw Exceptions, or one of > its subclasses. However, there are a small number of functions that are > unnecessarily declared to throw Throwable instead of Exception. I propose > that we change them to also be declared to throw Exception. > > This is a trivial patch to make it easier to refactor tests to use the > updated functions. > > Tested manually, but will wait for GHA to verify that the change is OK. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/17240/files - new: https://git.openjdk.org/jdk/pull/17240/files/910a863c..402b6727 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17240&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17240&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17240.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17240/head:pull/17240 PR: https://git.openjdk.org/jdk/pull/17240
RFR: 8322920: Some executeProcess overloads are declared to throw Throwable
Most functions in ProcessTools are declared to throw Exceptions, or one of its subclasses. However, there are a small number of functions that are unnecessarily declared to throw Throwable instead of Exception. I propose that we change them to also be declared to throw Exception. This is a trivial patch to make it easier to refactor tests to use the updated functions. Tested manually, but will wait for GHA to verify that the change is OK. - Commit messages: - 8322920: Some executeProcess overloads are declared to throw Throwable Changes: https://git.openjdk.org/jdk/pull/17240/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17240&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322920 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17240.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17240/head:pull/17240 PR: https://git.openjdk.org/jdk/pull/17240
Integrated: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder
On Mon, 11 Dec 2023 09:15:50 GMT, Stefan Karlsson wrote: > [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. This pull request has now been integrated. Changeset: cbe329b9 Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/cbe329b90ac1488836d4852fead79aa26c082114 Stats: 262 lines in 89 files changed: 73 ins; 1 del; 188 mod 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder Reviewed-by: lkorinth, lmesnik - PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v5]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson 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 remote-tracking branch 'upstream/master' into rename_executeTestJvm - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm - Test cleanup - Fix impl and add test - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/486dc6d5..755d925d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=03-04 Stats: 875 lines in 70 files changed: 577 ins; 58 del; 240 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Integrated: 8321718: ProcessTools.executeProcess calls waitFor before logging
On Mon, 11 Dec 2023 11:16:05 GMT, Stefan Karlsson wrote: > There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... This pull request has now been integrated. Changeset: 9ab29f8d Author:Stefan Karlsson URL: https://git.openjdk.org/jdk/commit/9ab29f8dcd1c0092e4251f996bd53c704e87a74a Stats: 61 lines in 3 files changed: 47 ins; 11 del; 3 mod 8321718: ProcessTools.executeProcess calls waitFor before logging Reviewed-by: dholmes, jpai - PR: https://git.openjdk.org/jdk/pull/17052
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v3]
On Mon, 11 Dec 2023 14:06:43 GMT, Stefan Karlsson wrote: >> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename >> createJavaProcessBuilder' changed the name of the ProcessTools helper >> functions used to create `ProcessBuilder`s used to spawn new java test >> processes. >> >> We now have `createTestJavaProcessBuilder` and >> `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, >> while the latter doesn't. >> >> With these functions it is common to see the following pattern in tests: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = executeProcess(pb); >> >> >> We have a couple of thin wrapper in `ProcessTools` that does exactly this, >> so that the code can be written as a one-liner: >> >> OutputAnalyzer output = ProcessTools.executeTestJvm(); >> >> >> I propose that we name this functions using the same naming scheme we used >> for `createTestJavaProcessBuilder` and >> `createLimitedTestJavaProcessBuilder`. That is, we change `executeTestJvm` >> to `executeTestJava` and add a new `executeLimitedTestJava` function. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Test cleanup Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/17049#issuecomment-1874176578
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v4]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson 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 four additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm - Test cleanup - Fix impl and add test - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/5d488f42..486dc6d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=02-03 Stats: 5249 lines in 348 files changed: 3069 ins; 973 del; 1207 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]
On Tue, 12 Dec 2023 09:01:08 GMT, Stefan Karlsson wrote: >> There is some logging printed when tests spawns processes. This logging is >> triggered from calls to `OutputAnalyzer`, when it delegates calls to >> `LazyOutputBuffer`. >> >> If we write code like this: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = new OutputAnalyzer(pb.start()); >> int exitValue = output.getExitValue(); >> >> >> We get the following logging: >> >> [timestamp0] "Gathering output for process >> [timestamp1] Waiting for completion for process >> [timestamp2] Waiting for completion finished for process >> >> >> The first line comes from the `OutputAnalyzer` constructor and the two other >> lines comes from the `getExitValue` call, which calls logs the above >> messages around the call to `waitFor`. >> >> If instead write the code above as: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = ProcessTools.executeProcess(pb); >> int exitValue = output.getExitValue(); >> >> >> We get the same logging, but timestamp1 and timestamp2 are almost the same. >> This happens because there's an extra call to `waitFor` inside >> `executeProcess`, but that `waitFor` does not have the "wait for" logging. >> So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. >> >> I would like to propose a small workaround to solve this. The workaround is >> that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` >> via `OutputAnalyzer`. This is a small, limited workaround for this issue. >> Ideally I would like to extract the entire Process handling code out of >> LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all >> usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the >> test directories, so I'm starting out with this suggestion. >> >> We can see of this patch by looking at the timestamps generated from the >> included test. Without the proposed workaround: >> >> Without >> >> testExecuteProcessExit >> [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 >> [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 >> [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process >> 2547719 >> >> testExecuteProcessStdout >> [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 >> [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 >> [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process >> 2547741 >> >> >> testNewOutp... > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Typo Thanks for reviewing! I'll remove the test, merge, and will do some sanity checks before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/17052#issuecomment-1874162036
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v3]
> There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... Stefan Karlsson 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 remote-tracking branch 'upstream/master' into executeProcessLogging - Remove temporary test - Typo - Whitespace cleanups - 8321718: ProcessTools.executeProcess calls waitFor before logging - Changes: - all: https://git.openjdk.org/jdk/pull/17052/files - new: https://git.openjdk.org/jdk/pull/17052/files/3275136c..fcba9dbd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17052&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17052&range=01-02 Stats: 5347 lines in 349 files changed: 3069 ins; 1071 del; 1207 mod Patch: https://git.openjdk.org/jdk/pull/17052.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17052/head:pull/17052 PR: https://git.openjdk.org/jdk/pull/17052
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]
> There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Typo - Changes: - all: https://git.openjdk.org/jdk/pull/17052/files - new: https://git.openjdk.org/jdk/pull/17052/files/145e1eaa..3275136c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17052&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17052&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17052.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17052/head:pull/17052 PR: https://git.openjdk.org/jdk/pull/17052
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]
On Tue, 12 Dec 2023 07:43:31 GMT, David Holmes wrote: >> Stefan Karlsson has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Typo > > test/lib/jdk/test/lib/process/OutputAnalyzer.java line 111: > >> 109: /** >> 110: * Delegate waitFor to the OutputBuffer. This ensures that >> 111: * the progress and timestmaps are logged correctly. > > Typo: timestmaps Thanks. Updated - PR Review Comment: https://git.openjdk.org/jdk/pull/17052#discussion_r1423664193
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging
On Mon, 11 Dec 2023 11:16:05 GMT, Stefan Karlsson wrote: > There is some logging printed when tests spawns processes. This logging is > triggered from calls to `OutputAnalyzer`, when it delegates calls to > `LazyOutputBuffer`. > > If we write code like this: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = new OutputAnalyzer(pb.start()); > int exitValue = output.getExitValue(); > > > We get the following logging: > > [timestamp0] "Gathering output for process > [timestamp1] Waiting for completion for process > [timestamp2] Waiting for completion finished for process > > > The first line comes from the `OutputAnalyzer` constructor and the two other > lines comes from the `getExitValue` call, which calls logs the above messages > around the call to `waitFor`. > > If instead write the code above as: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = ProcessTools.executeProcess(pb); > int exitValue = output.getExitValue(); > > > We get the same logging, but timestamp1 and timestamp2 are almost the same. > This happens because there's an extra call to `waitFor` inside > `executeProcess`, but that `waitFor` does not have the "wait for" logging. > So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. > > I would like to propose a small workaround to solve this. The workaround is > that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` > via `OutputAnalyzer`. This is a small, limited workaround for this issue. > Ideally I would like to extract the entire Process handling code out of > LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all > usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the > test directories, so I'm starting out with this suggestion. > > We can see of this patch by looking at the timestamps generated from the > included test. Without the proposed workaround: > > Without > > testExecuteProcessExit > [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 > [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 > [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process > 2547719 > > testExecuteProcessStdout > [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 > [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 > [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process > 2547741 > > > testNewOutputAnalyzerExit > [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 > [2023... Thanks for reviewing! I'm considering removing the test before integrating, as it currently takes >8s to run and it was mainly used to show the difference between the implementations. Do you agree that it would be OK to remove the test? - PR Comment: https://git.openjdk.org/jdk/pull/17052#issuecomment-1851564295
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v3]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Test cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/ad072e06..5d488f42 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=01-02 Stats: 10 lines in 1 file changed: 1 ins; 8 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v2]
> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename > createJavaProcessBuilder' changed the name of the ProcessTools helper > functions used to create `ProcessBuilder`s used to spawn new java test > processes. > > We now have `createTestJavaProcessBuilder` and > `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, > while the latter doesn't. > > With these functions it is common to see the following pattern in tests: > > ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); > OutputAnalyzer output = executeProcess(pb); > > > We have a couple of thin wrapper in `ProcessTools` that does exactly this, so > that the code can be written as a one-liner: > > OutputAnalyzer output = ProcessTools.executeTestJvm(); > > > I propose that we name this functions using the same naming scheme we used > for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. > That is, we change `executeTestJvm` to `executeTestJava` and add a new > `executeLimitedTestJava` function. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Fix impl and add test - Changes: - all: https://git.openjdk.org/jdk/pull/17049/files - new: https://git.openjdk.org/jdk/pull/17049/files/080caef5..ad072e06 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=00-01 Stats: 54 lines in 2 files changed: 52 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging
There is some logging printed when tests spawns processes. This logging is triggered from calls to `OutputAnalyzer`, when it delegates calls to `LazyOutputBuffer`. If we write code like this: ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); OutputAnalyzer output = new OutputAnalyzer(pb.start()); int exitValue = output.getExitValue(); We get the following logging: [timestamp0] "Gathering output for process [timestamp1] Waiting for completion for process [timestamp2] Waiting for completion finished for process The first line comes from the `OutputAnalyzer` constructor and the two other lines comes from the `getExitValue` call, which calls logs the above messages around the call to `waitFor`. If instead write the code above as: ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); OutputAnalyzer output = ProcessTools.executeProcess(pb); int exitValue = output.getExitValue(); We get the same logging, but timestamp1 and timestamp2 are almost the same. This happens because there's an extra call to `waitFor` inside `executeProcess`, but that `waitFor` does not have the "wait for" logging. So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. I would like to propose a small workaround to solve this. The workaround is that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` via `OutputAnalyzer`. This is a small, limited workaround for this issue. Ideally I would like to extract the entire Process handling code out of LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the test directories, so I'm starting out with this suggestion. We can see of this patch by looking at the timestamps generated from the included test. Without the proposed workaround: Without testExecuteProcessExit [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process 2547719 testExecuteProcessStdout [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process 2547741 testNewOutputAnalyzerExit [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782 [2023-12-11T11:05:46.231972095Z] Waiting for completion for process 2547782 [2023-12-11T11:05:48.421324123Z] Waiting for completion finished for process 2547782 testNewOutputAnalyzerStdout [2023-12-11T11:05:48.424801269Z] Gathering output for process 2547929 With the proposed workaround: testExecuteProcessExit [2023-12-11T11:06:12.203633363Z] Gathering output for process 2550961 [2023-12-11T11:06:12.285826294Z] Waiting for completion for process 2550961 [2023-12-11T11:06:14.361912745Z] Waiting for completion finished for process 2550961 testExecuteProcessStdout [2023-12-11T11:06:14.398685794Z] Gathering output for process 2550982 [2023-12-11T11:06:14.399518617Z] Waiting for completion for process 2550982 [2023-12-11T11:06:16.595986889Z] Waiting for completion finished for process 2550982 testNewOutputAnalyzerExit [2023-12-11T11:06:16.602218373Z] Gathering output for process 2551067 [2023-12-11T11:06:16.603176801Z] Waiting for completion for process 2551067 [2023-12-11T11:06:18.793886942Z] Waiting for completion finished for process 2551067 testNewOutputAnalyzerStdout [2023-12-11T11:06:18.796332176Z] Gathering output for process 2551172 See how the timestamp for "Waiting for completion for process" becomes "earlier" in the "executeProcess" cases. - Commit messages: - Whitespace cleanups - 8321718: ProcessTools.executeProcess calls waitFor before logging Changes: https://git.openjdk.org/jdk/pull/17052/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17052&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321718 Stats: 159 lines in 4 files changed: 145 ins; 11 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17052.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17052/head:pull/17052 PR: https://git.openjdk.org/jdk/pull/17052
RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder
[JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename createJavaProcessBuilder' changed the name of the ProcessTools helper functions used to create `ProcessBuilder`s used to spawn new java test processes. We now have `createTestJavaProcessBuilder` and `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, while the latter doesn't. With these functions it is common to see the following pattern in tests: ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); OutputAnalyzer output = executeProcess(pb); We have a couple of thin wrapper in `ProcessTools` that does exactly this, so that the code can be written as a one-liner: OutputAnalyzer output = ProcessTools.executeTestJvm(); I propose that we name this functions using the same naming scheme we used for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. That is, we change `executeTestJvm` to `executeTestJava` and add a new `executeLimitedTestJava` function. - Commit messages: - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder Changes: https://git.openjdk.org/jdk/pull/17049/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17049&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321713 Stats: 217 lines in 88 files changed: 28 ins; 1 del; 188 mod Patch: https://git.openjdk.org/jdk/pull/17049.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049 PR: https://git.openjdk.org/jdk/pull/17049
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Wed, 6 Dec 2023 01:56:46 GMT, David Holmes wrote: > FWIW exitCode out numbers exitValue in source code 3:1 (and > 5:1 in test > code). That might be the case, but both the called function returning the value and the function we are changing uses the name exitValue. It seems irrelevant that other places in the code uses exitCode. - PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1842489162
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]
On Fri, 1 Dec 2023 12:44:17 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change to the test library's >> `OutputAnalyzer` class, which proposes to remove some unnecessary logging >> from the `getExitValue()` call? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this >> method logs: >> >> >> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909 >> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process >> 24909 >> >> >> even when the process has already completed and the exit value already >> known. The change in this PR makes it such that if the exit value is >> available then we no longer log this (nor call `process.waitFor()`). >> >> No new tests have been added given the nature of this change. tier1, tier2 >> and tier3 tests continue to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > remove micro optimization Looks good! - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1759744755
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v2]
On Fri, 1 Dec 2023 11:14:04 GMT, Jaikiran Pai wrote:
>> test/lib/jdk/test/lib/process/OutputBuffer.java line 150:
>>
>>> 148: @Override
>>> 149: public int getExitValue() {
>>> 150: Integer exitCode = this.processExitCode;
>>
>> Do we really need the local `exitCode` variable? Even if another multiple
>> threads write to processExitCode, I expect them to all write a non-null
>> Integer.
>
> Hello Stefan, this is just for a tiny optimization to prevent multiple reads
> (in the same thread) on the `volatile` field `processExitCode` in this method.
I don't think such an optimization is needed here. This is not
performance-critical code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1412028528
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed
On Fri, 1 Dec 2023 11:09:30 GMT, Stefan Karlsson wrote:
>> Can I please get a review for this change to the test library's
>> `OutputAnalyzer` class, which proposes to remove some unnecessary logging
>> from the `getExitValue()` call?
>>
>> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this
>> method logs:
>>
>>
>> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909
>> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process
>> 24909
>>
>>
>> even when the process has already completed and the exit value already
>> known. The change in this PR makes it such that if the exit value is
>> available then we no longer log this (nor call `process.waitFor()`).
>>
>> No new tests have been added given the nature of this change. tier1, tier2
>> and tier3 tests continue to pass with this change.
>
> test/lib/jdk/test/lib/process/OutputBuffer.java line 158:
>
>> 156: boolean aborted = true;
>> 157: try {
>> 158: this.processExitCode = exitCode = p.waitFor();
>
> According to the `waitFor` javadocs it returns the "exit value" and this
> function is named `getExitValue()`. I propose that we rename
> `processExitCode` to `exitValue` (alt. `processExitValue`).
With the earlier suggestion, this would become:
exitValue = p.waitFor();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411949002
Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed
On Fri, 1 Dec 2023 09:48:23 GMT, Jaikiran Pai wrote:
> Can I please get a review for this change to the test library's
> `OutputAnalyzer` class, which proposes to remove some unnecessary logging
> from the `getExitValue()` call?
>
> As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this
> method logs:
>
>
> [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909
> [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process
> 24909
>
>
> even when the process has already completed and the exit value already known.
> The change in this PR makes it such that if the exit value is available then
> we no longer log this (nor call `process.waitFor()`).
>
> No new tests have been added given the nature of this change. tier1, tier2
> and tier3 tests continue to pass with this change.
Great find and nice to get rid of the extra logging!
test/lib/jdk/test/lib/process/OutputBuffer.java line 150:
> 148: @Override
> 149: public int getExitValue() {
> 150: Integer exitCode = this.processExitCode;
Do we really need the local `exitCode` variable? Even if another multiple
threads write to processExitCode, I expect them to all write a non-null Integer.
test/lib/jdk/test/lib/process/OutputBuffer.java line 158:
> 156: boolean aborted = true;
> 157: try {
> 158: this.processExitCode = exitCode = p.waitFor();
According to the `waitFor` javadocs it returns the "exit value" and this
function is named `getExitValue()`. I propose that we rename `processExitCode`
to `exitValue` (alt. `processExitValue`).
-
PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1759560718
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411946205
PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411948365
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer
On Thu, 30 Nov 2023 12:19:33 GMT, Jaikiran Pai wrote: > Hello Stefan, > > > The test were I want to use this is a long-running stress test that is > > known to be good at shaking out JVM hangs. It has been written to provide > > its own output about spawned processes and what they are doing, and have > > that info written to appropriate files. For me, the OutputAnalyzer logging > > just adds redundant output to the streams where I don't want the > > information. > > Thank you for that context. What you are then proposing is for a way to reuse > this utility class in specific tests, like the one you are working on, > without needing the progress logs. So the idea isn't really to use this new > API in all newly developed tests which use this `ProcessTools` library. Plus > the implementation in this PR, retains the existing behaviour of logging the > progress by default, which is a good thing. Yes, you summarize this well. > > Given that context, I don't have any objection in introducing this > enhancemnet. During future reviews, I think, it wouldn't be too difficult to > catch misuses of this new API in newer tests where the progress logs > shouldn't be disabled. Yes, I agree. It would be quite obvious if someone tries to turn this off. > > As for whether this "config" should be externalized - I think it shouldn't > be, since whether or not the progress logs will be excessive and too > distracting (and thus need to be disabled) would be known at test development > time itself. I agree. > The copyright year on OutputBuffer.java will need an update. Fixed. Thanks for taking a look at this! - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1834103958
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v2]
> Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision: Update OutputBuffer.java copyright years - Changes: - all: https://git.openjdk.org/jdk/pull/16807/files - new: https://git.openjdk.org/jdk/pull/16807/files/d5b13dce..d0091eb6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer
On Fri, 24 Nov 2023 17:01:29 GMT, Jaikiran Pai wrote: > Hello Stefan, I agree with David that I have found these logs to be useful - > it shows that process launched by the test hasn't yet finished and is being > waited upon. The log message when written will have a timestamp and can then > also help to quickly understand how long it could have taken for this process > to complete. The test were I want to use this is a long-running stress test that is known to be good at shaking out JVM hangs. It has been written to provide its own output about spawned processes and what they are doing, and have that info written to appropriate files. For me, the OutputAnalyzer logging just adds redundant output to the streams where I don't want the information. When we do have JVM hangs in these larger tests it's usually is easier to look at the output generated from [test/failure_handler](https://github.com/openjdk/jdk/tree/master/test/failure_handler). - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1827410018
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer
On Fri, 24 Nov 2023 17:13:30 GMT, Jaikiran Pai wrote: > Other than a quick local test run, I haven't tested it for anything else. It > did improve the situation in one of my test and it now only logs it once. In > the setup that you are seeing this large amount of logging, would you be able > to see if this change helps there too? The place where I would like to use it we repeatedly spawn a number of different processes to inspect the JVM and the OS. So, I think you have found a separate issue. - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1827352788
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer
On Fri, 24 Nov 2023 13:06:05 GMT, David Holmes wrote: > I can't help but feel that if you disable this output then you are going to > be missing it in the cases that you need it! Something will go wrong and > there won't be enough info in the log to know what it was. My experience is that I won't need it. I've been able to identify numerous hangs, deadlocks, timeouts in the stress tests where I want to use this. Right now I much more annoyed by the log files filling up with thousands upon thousands lines of noise, making it harder to actually find the status of the test. An alternative to this could be to redirect the output to another file. - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1825888462
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer
On Fri, 24 Nov 2023 10:34:02 GMT, Stefan Karlsson wrote: > Tests using ProcessTools.executeProcess gets the following output written to > stdout: > [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 > [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 > [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process > 2517117 > > This might be good for some use cases and debugging, but some tests spawns a > large number of processes and for those this output fills up the log files. > > I propose that we add a way to turn of this output for tests where we find > this output to be too noisy. This has been tested with an Oracle-internal stress tests. - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1825466825
RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer
Tests using ProcessTools.executeProcess gets the following output written to stdout: [2023-11-24T09:58:16.797540608Z] Gathering output for process 2517117 [2023-11-24T09:58:23.070781345Z] Waiting for completion for process 2517117 [2023-11-24T09:58:23.071045055Z] Waiting for completion finished for process 2517117 This might be good for some use cases and debugging, but some tests spawns a large number of processes and for those this output fills up the log files. I propose that we add a way to turn of this output for tests where we find this output to be too noisy. - Commit messages: - 8320699: Add parameter to skip progress logging of OutputAnalyzer Changes: https://git.openjdk.org/jdk/pull/16807/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16807&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320699 Stats: 24 lines in 2 files changed: 21 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16807.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16807/head:pull/16807 PR: https://git.openjdk.org/jdk/pull/16807
Re: RFR: 8319613: Complier error in benchmark TestLoadSegmentVarious
On Tue, 7 Nov 2023 10:36:00 GMT, Per Minborg wrote: > This PR proposes to fix a compilation error in the TestLoadSegmentVarious > class Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16537#pullrequestreview-1717370876
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder()
On Wed, 1 Nov 2023 00:06:35 GMT, Leonid Mesnik wrote: > Test thread factory is a mode similar to VM flags and should not be used in > ProcessTools.createLimitedTestJavaProcessBuilder(). Only > createTestJavaProcessBuilder() should use it like jtreg VM options. > > Adding the test thread factory requires the injection of arguments in the > middle of the list. I don't think it makes sense to modify arguments in > several places so I replaced it with the flag isLimited and moved all > modifications in createJavaProcessBuilder(). > > Testing tier1-5. It seems like the code would be cleaner if you moved the threadFactory injection to `createTestJavaProcessBuilder`. - Changes requested by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16442#pullrequestreview-1707671922
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Tue, 5 Sep 2023 18:05:34 GMT, Roger Riggs wrote: >> I have created an alternative that uses enums to force the user to make a >> decision: >> https://github.com/openjdk/jdk/compare/master...lkorinth:jdk:+process_tools >> . Another alternative is to do the same but instead using an enum (I think >> it is not as good). A third alternative is to use the current pull request >> with a better name. >> >> What do you prefer? Do you have a better alternative? Do someone still think >> the current code is good? I think what we have today is inferior to all >> these improvements, and I would like to make it harder to develop bad test >> cases. > >> What do you prefer? Do you have a better alternative? Do someone still think >> the current code is good? I think what we have today is inferior to all >> these improvements, and I would like to make it harder to develop bad test ca > > The current API (name) is fine and fit for purpose; it does not promise or > hide extra functionality under a simple name. > > There needs to be an explicit intention in the test(s) to support after the > fact that arbitrary flags can be added. > @AlanBateman's proposal for naming > [above](https://github.com/openjdk/jdk/pull/15452#issuecomment-1700459277) > (or similar) would capture more clearly that test options are propagated to > the child process. > Every test writer should be aware that additional command line options may be > mixed in. > > There are many cases in which the ProcessTools APIs are not used to create > child processes and do not need to be used in writing tests. They provide > some convenience but also add a dependency and another API layer to work > through in the case of failures. > > As far as I'm aware, there is no general guidance or design pattern outside > of hotspot tests to propagate flags or use ProcessTools. Adding that as a > requirement will need a different level of communication and change. @RogerRiggs You seem to know what you want w.r.t. the extra java doc comments. Could you help write those? Could we also do that as a separate RFE? I think that would make it easier to get this PR and the javadoc update through the door. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1778669353
Re: RFR: 8315097: Rename createJavaProcessBuilder [v6]
On Tue, 24 Oct 2023 07:49:30 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with six additional > commits since the last revision: > > - static import > - copyright > - whitespace > - whitespace > - sed > - fix test/lib/jdk/test/lib/process/ProcessTools.java Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1694437335
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix static import >From talking to other HotSpot devs it is quite clear that the different names >lead to mistakes when writing (copy-n-pasting) tests, so I'm happy that we try >to figure out some way to make it more obvious when we prepend the extra test >options and when we don't. I agree with @msheppar that `createTestJvm` isn't a good name and I wouldn't be opposed to changing that name instead of `createJavaProcessBuilder`. However, if we do rename that function I strongly urge us to also rename the corresponding `executeTestJvm` function. I also think it is too obscure that functions with *Test* in the name prepend the extra test options, and those without *Test* don't, so I'd like to get rid of that convention. I wouldn't be opposed to a change that: * Keeps the `createJavaProcessBuilder` name * Renames `createTestJvm` to `createJavaProcessBuilderPrependTestOpts` * Renames `executeTestJvm` to `executeJavaPrependTestOpts` * Removes `createTestJava` * Removes `executeTestJava` - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1699588853
Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v2]
On Fri, 16 Jun 2023 11:41:14 GMT, Axel Boldt-Christmas
wrote:
>> The current implementation for testing generational ZGC with jtreg is
>> implemented with a filter on the mode flag `ZGenerational`. Because of this
>> only environments which set this flag explicitly will run most of the tests.
>> So they get missed in Github Actions and for developers running jtreg
>> locally without supplying the `ZGenerational` flag.
>>
>> The proposed change here is to introduce two new jtreg requirement
>> properties, `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`. These flags will
>> effectively behave the same as the existing `vm.gc.` flags but also take
>> the specific ZGC mode in account.
>>
>> If no gc flags are supplied to jtreg and the `vm.gc.Z` is true (the build
>> includes ZGC) both `vm.gc.ZGenerational` and `vm.gc.ZSingelgen` will be true.
>>
>> If `-XX:+UseZGC` is supplied then both `vm.gc.ZGenerational` and
>> `vm.gc.ZSingelgen` will also be true.
>>
>> If `-XX:{+,-}ZGenerational` is supplied then either `vm.gc.ZGenerational`
>> or `vm.gc.ZSingelgen` be true depending on the flags value.
>>
>> And if `vm.gc.Z` is false both `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`
>> will be false.
>>
>> This change also splits the relevant tests into two distinct runs for the
>> two modes. And the respective test ids are set to `ZGenerational` or
>> `ZSinglegen` to make it easier to distinguish the runs.
>>
>> This also solves the issue that some compiler tests will never run with
>> generational ZGC unless the `TEST_VM_FLAGLESS` is set. This is because the
>> current filter `vm.opt.final.ZGenerational` requires the flag to be
>> explicit, but these compiler tests uses `vm.flagless`.
>>
>> The introduction of `vm.gc.ZGenerational` and `vm.gc.ZSingelgen` harmonizes
>> the way you specify generational / single gen ZGC test with the way it is
>> done for other gcs with `vm.gc.`
>>
>> To support this feature the Whitebox API is extended with `isDefaultVMFlag`
>> to enable checking if `ZGenerational` is default or not.
>>
>> `vm.opt.final.ZGenerational` is still kept because we still have some
>> reasons to filter based on the supplied flags.
>> - `test/hotspot/jtreg/gc/cslocker/TestCSLocker.java` is disabled when
>> running with ZGenerational
>> - `test/jdk/java/lang/ProcessBuilder/CloseRace.java` is ran with a different
>> max heap size for ZGenerational, but it is not the intent to dispatch the
>> test to both G1 and generational ZGC if Generational ZGC is not specified.
>>
>> `test/jdk/java/lang/management/MemoryMXBean/MemoryTest.java` was also
>> changed to not filter but instead dispatch. However u...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix wrong ZGenerational flag in VectorRebracket128Test.java
Looks good.
-
Marked as reviewed by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14509#pullrequestreview-1483282594
Re: RFR: JDK-8308047 java/util/concurrent/ScheduledThreadPoolExecutor/BasicCancelTest.java timed out and also had jcmd pipe errors [v2]
On Fri, 9 Jun 2023 10:07:31 GMT, Viktor Klang wrote: >> The test used a too-small heap for generic testing, and coupled with looking >> at `freeMemory()` it led to a less-than-ideal determinism of execution of >> the test. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Decresing the number of iterations proportional to the increase in the max > memory size I'm happy with the changes, but make sure to get a Review from one of the maintainers of this code area. Thanks. - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14389#pullrequestreview-1471817200
Re: RFR: JDK-8308047 java/util/concurrent/ScheduledThreadPoolExecutor/BasicCancelTest.java timed out and also had jcmd pipe errors
On Fri, 9 Jun 2023 09:13:08 GMT, Viktor Klang wrote: > The test used a too-small heap for generic testing, and coupled with looking > at `freeMemory()` it led to a less-than-ideal determinism of execution of the > test. Did you test if this significantly changed the execution time of the test? I'm wondering if it would make sense to change this to something smaller, say `maxMemory / 32`? - PR Review: https://git.openjdk.org/jdk/pull/14389#pullrequestreview-1471626631
Re: RFR: 8309408: Thread.sleep cleanup
On Sun, 4 Jun 2023 11:28:33 GMT, Alan Bateman wrote: > Thread.sleep has had quite a bit of churn recently to support virtual > threads, add sleep(Duration), a JFR event, and the change the underlying > implementation to support sub-millis precision. I think the changes have > settled down now so we can do some small cleanups that came up in PR > discussions. The cleanups were kicked down the road as it requires tracking > down faraway tests that depend on the stack depth and the names of internal > methods. The two cleanups proposed here are: > > 1. Add a private sleepNanos method that creates/commits the JFR event around > the sleep, this avoids duplicate code in the 3 sleep methods. > 2. Rename JVM_Sleep to JVM_SleepNanos to make it clear that it takes the > sleep time in nanoseconds, esp. when Thread.sleep's parameter is milliseconds. I welcome this change. We were looking at this code on Friday and was thinking that it would have been good to rename sleep0 and JVM_Sleep. :) - Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14303#pullrequestreview-1461982066
Re: RFR: 8308223: failure handler missed jcmd.vm.info command
On Tue, 16 May 2023 18:07:16 GMT, Leonid Mesnik wrote: > Trivial fix that added missed useful command. > Tested by running make of failure handler and verifying results. Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14018#pullrequestreview-1429289984
Re: RFR: 8308223: failure handler missed jcmd.vm.info command
On Tue, 16 May 2023 18:07:16 GMT, Leonid Mesnik wrote: > Trivial fix that added missed useful command. > Tested by running make of failure handler and verifying results. This looks good and "trivial". Thanks for fixing this. - PR Comment: https://git.openjdk.org/jdk/pull/14018#issuecomment-1550170733
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v22]
On Wed, 10 May 2023 13:43:41 GMT, Martin Doerr wrote: >> You are reasoning about implementation details. By using the provided >> abstraction you and other maintainers (who might be unfamiliar with them) >> would not have to do that. Also the assumptions you make introduce a hidden >> dependency. > > I just figured it out. It was introduced by > https://bugs.openjdk.org/browse/JDK-8203172 (on aarch64) which mentions > Shenandoah and future GCs. However, the Shenandoah comment says > "non-reference load, no additional barrier is needed" and it doesn't use > barriers in such a case. So, for the time being, I'll keep the normal load > (because `access_load_at` is not ready for non-oop types). But I should add > the `NONZERO` check. FWIW, since Shenandoah changed their load barriers we have been cleaning away the usages of the Access API for loads and stores to primitive values. There's no such support in the C++ Runtime code. - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1189948988
