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
Integrated: 8330702: Update failure handler to don't generate Error message if cores actions are empty
On Thu, 30 May 2024 02:28:56 GMT, Leonid Mesnik wrote: > The message is generated if cores (or any other tools) section doesn't exist > or is empty. However, there is no any tool for cores processing now defined. > So ERROR message is generating, confusing users. > The fix is to don't print error for empty toolset which is the valid case. > The message is still generate is tool is not defined to get error message in > the case of miswriting. This pull request has now been integrated. Changeset: 548e95a6 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/548e95a689d63e97ddbdfe7dd7df3a2e3377046c Stats: 9 lines in 2 files changed: 5 ins; 0 del; 4 mod 8330702: Update failure handler to don't generate Error message if cores actions are empty Reviewed-by: sspitsyn - PR: https://git.openjdk.org/jdk/pull/19470
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed.
On Fri, 24 Feb 2023 22:15:18 GMT, Leonid Mesnik wrote: > The solution proposed by Stefan K > > The startProcess() might wait forever for the expected line if the process > exits (failed to start). It makes sense to just fail earlier in such cases. > > The fix also move > 'output = new OutputAnalyzer(this.process);' > in method xrun() to be able to try to print them in waitFor is > failed/interrupted. Thanks - PR Comment: https://git.openjdk.org/jdk/pull/12751#issuecomment-1444608371
RFR: 8330702: Update failure handler to don't generate Error message if cores actions are empty.
The message is generated if cores (or any other tools) section doesn't exist or is empty. However, there is no any tool for cores processing now defined. So ERROR message is generating, confusing users. The fix is to don't print error for empty toolset which is the valid case. The message is still generate is tool is not defined to get error message in the case of miswriting. - Commit messages: - 8330702 Changes: https://git.openjdk.org/jdk/pull/19470/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19470=00 Issue: https://bugs.openjdk.org/browse/JDK-8330702 Stats: 9 lines in 2 files changed: 5 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19470.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19470/head:pull/19470 PR: https://git.openjdk.org/jdk/pull/19470
Re: RFR: 8332898: failure_handler: log directory of commands
On Fri, 24 May 2024 14:34:39 GMT, Ludvig Janiuk wrote: > Also log the directory in which the command used by failure_handler was > executed. While often the same, it isn't always, and so it this should > simplify troubleshooting for someone looking at this at a glance without > being a failure_handler expert. > > Example output after this change: > > > [2024-05-24 14:26:46] [/usr/bin/pmap, -p, 2233017] timeout=2 in > //JTwork/scratch > > > before: > > > [2024-05-24 14:26:46] [/usr/bin/pmap, -p, 2233017] timeout=2 > Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19396#pullrequestreview-2077492691
Re: RFR: 8332885: Clarify failure_handler self-tests
On Fri, 24 May 2024 12:16:25 GMT, Ludvig Janiuk wrote: > Adding commetns to clarify that the failure_handler selftests are intended to > be run manually. Ideally this would include a more thorough description of > the exepcted outputs, but this is what I have time to add right now. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19393#pullrequestreview-2077481241
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 01:18:50 GMT, Kim Barrett wrote: > Please review this change that renames some test .h files to .hpp. These > files contain C++ code and should be named accordingly. Some of them contain > uses of NULL, which we change to nullptr. > > The renamed files are: > > test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h > test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h > > test/lib/jdk/test/lib/jvmti/jvmti_thread.h > test/lib/jdk/test/lib/jvmti/jvmti_common.h > test/lib/native/testlib_threads.h > > The #include updates were performed mostly mechanically, and builds would fail > if there were mistakes. The exception is test > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, > which was added after I'd done the mechanical update, so was updated by hand. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. > > Thus, the only "interesting" changes are to the renamed files. > > Testing: mach5 tier1 Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18035#pullrequestreview-1907166501
RFR: 8316451: 6 java/lang/instrument/PremainClass tests ignore VM flags
Tests updated to use jtreg vm flags. Tested by running tests with different flags and tier1. - Commit messages: - 8316451 Changes: https://git.openjdk.org/jdk/pull/17781/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17781=00 Issue: https://bugs.openjdk.org/browse/JDK-8316451 Stats: 10 lines in 2 files changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17781.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17781/head:pull/17781 PR: https://git.openjdk.org/jdk/pull/17781
RFR: 8319578: Few java/lang/instrument ignore test.java.opts and accept test.vm.opts only
There are several .sh tests which use ${TESTVMOPTS} only. Updated them to use
${TESTJAVAOPTS} also.
Tested by running them with different options and tier1.
-
Commit messages:
- 8319578: Few java/lang/instrument ignore test.java.opts and accept
test.vm.opts only
Changes: https://git.openjdk.org/jdk/pull/17780/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=17780=00
Issue: https://bugs.openjdk.org/browse/JDK-8319578
Stats: 35 lines in 14 files changed: 0 ins; 0 del; 35 mod
Patch: https://git.openjdk.org/jdk/pull/17780.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17780/head:pull/17780
PR: https://git.openjdk.org/jdk/pull/17780
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Wed, 24 Jan 2024 21:28:29 GMT, Leonid Mesnik wrote: >> Some jtreg tests require resolvable external dependencies. This resolution >> is delegated to JIB, which is not used in vanilla OpenJDK testing. It would >> be convenient to add a keyword that marks tests that require these external >> dependencies, so that we could exclude those tests from runs. This would >> allow us to: a) run all tests in hotspot:tier4, which now excludes >> `applications/` specifically; b) make all tests runs (#17422) cleaner on >> many environments. >> >> I provisionally call this flag `external-dep`, but I am open for other >> suggestions. >> >> Note that some tests that pull `@Artifact`-s provide special paths that do >> limited testing anyway. However, there are tests which cannot run without >> external dependencies at all. These include at least `applications/jcstress` >> and `applications/scimark` tests. >> >> Ironically, I cannot run the jcstress test generator because the >> dependencies are lacking here. I regenerated those test using a self-built >> jcstress 0.16 bundle. >> >> Additional testing: >> - [x] `make test TEST=applications/` fails >> - [x] `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, >> skipping most of the tests > > Marked as reviewed by lmesnik (Reviewer). > @lmesnik, you good with the keyword name? Yes, I'm fine. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1910613276
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev wrote: > Some jtreg tests require resolvable external dependencies. This resolution is > delegated to JIB, which is not used in vanilla OpenJDK testing. It would be > convenient to add a keyword that marks tests that require these external > dependencies, so that we could exclude those tests from runs. This would > allow us to: a) run all tests in hotspot:tier4, which now excludes > `applications/` specifically; b) make all tests runs (#17422) cleaner on many > environments. > > I provisionally call this flag `external-dep`, but I am open for other > suggestions. > > Note that some tests that pull `@Artifact`-s provide special paths that do > limited testing anyway. However, there are tests which cannot run without > external dependencies at all. These include at least `applications/jcstress` > and `applications/scimark` tests. > > Ironically, I cannot run the jcstress test generator because the dependencies > are lacking here. I regenerated those test using a self-built jcstress 0.16 > bundle. > > Additional testing: > - [x] `make test TEST=applications/` fails > - [x] `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, > skipping most of the tests Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17421#pullrequestreview-1842419609
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Marked as reviewed by lmesnik (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/17422#pullrequestreview-1839361504
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v3]
On Fri, 5 Jan 2024 09:07:57 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. > > 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 I think that commit is going to have date in 2024. So any check is going to compare this date and copyright year. It shouldn't cause fail i CI and files are going to be updated later with verification scripts. - PR Comment: https://git.openjdk.org/jdk/pull/16807#issuecomment-1889709770
Re: RFR: 8320699: Add parameter to skip progress logging of OutputAnalyzer [v3]
On Fri, 5 Jan 2024 09:07:57 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. > > 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 Needed to update copyrights now. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16807#pullrequestreview-1817096686
Re: RFR: 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. You need to update copyrights. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17240#pullrequestreview-1805436752
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v7]
On Fri, 15 Dec 2023 10:49:56 GMT, Serguei Spitsyn wrote: >> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 >> time frame. >> It is fixing a deadlock issue between `VirtualThread` class critical >> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, >> `getAndClearInterrupt()`, `threadState()`, `toString()`), >> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. >> The deadlocking scenario is well described by Patricio in a bug report >> comment. >> In simple words, a virtual thread should not be suspended during >> 'interruptLock' critical sections. >> >> The fix is to record that a virtual thread is in a critical section >> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about >> begin/end of critical section. >> This bit is used in `HandshakeState::get_op_for_self()` to filter out any >> `HandshakeOperation` if a target `JavaThread` is in a critical section. >> >> Some of new notifications with `notifyJvmtiSync()` method is on a >> performance critical path. It is why this method has been intrincified. >> >> New test was developed by Patricio: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> The test is very nice as it reliably in 100% reproduces the deadlock without >> the fix. >> The test is never failing with this fix. >> >> Testing: >> - tested with newly added test: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: improve an assert message Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17011#pullrequestreview-1785514646
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Fri, 8 Dec 2023 11:54:40 GMT, Serguei Spitsyn wrote:
>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`,
>> `getAndClearInterrupt()`, `threadState()`, `toString()`),
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report
>> comment.
>> In simple words, a virtual thread should not be suspended during
>> 'interruptLock' critical sections.
>>
>> The fix is to record that a virtual thread is in a critical section
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>>
>> Some of new notifications with `notifyJvmtiSync()` method is on a
>> performance critical path. It is why this method has been intrincified.
>>
>> New test was developed by Patricio:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without
>> the fix.
>> The test is never failing with this fix.
>>
>> Testing:
>> - tested with newly added test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: (1) rename notifyJvmti method; (2) add try-final statements to
> VirtualThread methods
Changes requested by lmesnik (Reviewer).
src/hotspot/share/prims/jvm.cpp line 4013:
> 4011: // Notification from VirtualThread about entering/exiting sync critical
> section.
> 4012: // Needed to avoid deadlocks with JVMTI suspend mechanism.
> 4013: JVM_ENTRY(void, JVM_VirtualThreadCriticalLock(JNIEnv* env, jobject
> vthread, jboolean enter))
the jobject vthread is not used. Can't be the method made static to reduce the
number of arguments?
It is the performance-critical code, I don't know if it is optimized by C2.
src/hotspot/share/runtime/javaThread.hpp line 320:
> 318: bool _is_in_VTMS_transition; // thread is in
> virtual thread mount state transition
> 319: bool _is_in_tmp_VTMS_transition; // thread is in
> temporary virtual thread mount state transition
> 320: bool _is_in_critical_section; // thread is in
> a locking critical section
might make sense to add a comment, that his variable Is changed/read only by
current thread and no sync is needed.
src/java.base/share/classes/java/lang/VirtualThread.java line 1164:
> 1162:
> 1163: @IntrinsicCandidate
> 1164: private native void notifyJvmtiCriticalLock(boolean enter);
The name is confusing to me, the CriticalLock looks like it is the section is
critical and might be taken by a single thread only. Or it's just unclear what
is critical here.
However, the purpose is to disable suspend
Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name
here?
or comment what critical means here.
test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
line 30:
> 28: * @requires vm.continuations
> 29: * @library /testlibrary
> 30: * @run main/othervm -Xint SuspendWithInterruptLock
Doesn't it make sense to add a testcase without -Xint also? Just to give stress
testing with compilation.
test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
line 36:
> 34:
> 35: public class SuspendWithInterruptLock {
> 36: static boolean done;
done is accessed from different threads, should be volatile.
test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
line 54:
> 52: Thread.yield();
> 53: }
> 54: done = true;
I think it is better to use done to stop all threads and set it to true in the
main thread after some time. So you could be sure that the yielder hadn't been
completed before the suspender started. But it is just proposal.
-
PR Review: https://git.openjdk.org/jdk/pull/17011#pullrequestreview-1778571090
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424694672
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424697179
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424687810
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424662055
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424663078
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424683585
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 Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17049#pullrequestreview-1778566038
Re: RFR: 8319647: Update or mark as vm.flagless tests that ignore external VM flags
On Mon, 4 Dec 2023 10:39:05 GMT, Darragh Clarke wrote: > Updated tests to use vm.flagless as they already ignore Vm flags Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16946#pullrequestreview-1763304819
Integrated: 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX
On Fri, 1 Dec 2023 00:58:57 GMT, Leonid Mesnik wrote: > Description of problem and propsed fix from @plummercj > ' > In test/failure_handler/src/share/conf/mac.properties we have: > > process.top.app=top > process.top.args=-l 1 > > So top is run with the "-l 1" args, causing it to do one iteration and then > exit. Unfortunately the first iteration always shows all 0's for CPU usage. > If you do at least 2 iterations, you do see the proper CPU usage on the 2nd > iteration. The user just needs to know to scroll down to see it. I suggest we > start using "-l 2" unless someone has a better idea. > > BTW, for proper CPU usage you can instead look at the failure handler "ps" > output, which seems to be correct. But it is nice to have "top" produce the > correct output so all the CPU hogs are at the top of the list. > ' > > I verified that top report contains now 2 samples. This pull request has now been integrated. Changeset: 8be3e392 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/8be3e39220cd64521f4e370011958e17e5fdeaf3 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX Reviewed-by: cjplummer, jpai - PR: https://git.openjdk.org/jdk/pull/16915
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 Technically, the volatile is not enough to avoid racy writing into exitValue. However, it doesn't affect logic. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1760491166
Re: RFR: 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX
On Fri, 1 Dec 2023 00:58:57 GMT, Leonid Mesnik wrote: > Description of problem and propsed fix from @plummercj > ' > In test/failure_handler/src/share/conf/mac.properties we have: > > process.top.app=top > process.top.args=-l 1 > > So top is run with the "-l 1" args, causing it to do one iteration and then > exit. Unfortunately the first iteration always shows all 0's for CPU usage. > If you do at least 2 iterations, you do see the proper CPU usage on the 2nd > iteration. The user just needs to know to scroll down to see it. I suggest we > start using "-l 2" unless someone has a better idea. > > BTW, for proper CPU usage you can instead look at the failure handler "ps" > output, which seems to be correct. But it is nice to have "top" produce the > correct output so all the CPU hogs are at the top of the list. > ' > > I verified that top report contains now 2 samples. The top problem is macos specific issue and no need to fix other OS. - PR Comment: https://git.openjdk.org/jdk/pull/16915#issuecomment-1836465281
RFR: 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX
Description of problem and propsed fix from @plummercj ' In test/failure_handler/src/share/conf/mac.properties we have: process.top.app=top process.top.args=-l 1 So top is run with the "-l 1" args, causing it to do one iteration and then exit. Unfortunately the first iteration always shows all 0's for CPU usage. If you do at least 2 iterations, you do see the proper CPU usage on the 2nd iteration. The user just needs to know to scroll down to see it. I suggest we start using "-l 2" unless someone has a better idea. BTW, for proper CPU usage you can instead look at the failure handler "ps" output, which seems to be correct. But it is nice to have "top" produce the correct output so all the CPU hogs are at the top of the list. ' I verified that top report contains now 2 samples. - Commit messages: - 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX Changes: https://git.openjdk.org/jdk/pull/16915/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16915=00 Issue: https://bugs.openjdk.org/browse/JDK-8320129 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16915/head:pull/16915 PR: https://git.openjdk.org/jdk/pull/16915
Re: RFR: 8319567: Update java/lang/invoke tests to support vm flags [v3]
On Thu, 16 Nov 2023 18:20:04 GMT, Mandy Chung wrote: >> This PR includes test fixes for the following issues: >> >> 8319567: Update java/lang/invoke tests to support vm flags >> 8319568: Update java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java >> to accept vm flags >> 8319672: Several classloader tests ignore VM flags >> 8319676: A couple of jdk/modules/incubator/ tests ignore VM flags >> 8319677: Test jdk/internal/misc/VM/RuntimeArguments.java should be marked as >> flagless >> >> It converts the test to use `ProcessTools::createTestJavaProcessBuilder` or >> `createNativeTestJavaProcessBuilder` so that the test will support VM flags >> passed to jtreg. A couple tests that ignore VM flags should use >> `ProcessTools::createLimtiedTestJavaProcessBuilder` and marks the test with >> `@requires vm.flagless`. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > mark DefaultImage test vm.flagless Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/1#pullrequestreview-1735156738
Re: RFR: 8319567: Update java/lang/invoke tests to support vm flags [v2]
On Wed, 15 Nov 2023 02:39:56 GMT, Mandy Chung wrote: >> This PR includes test fixes for the following issues: >> >> 8319567: Update java/lang/invoke tests to support vm flags >> 8319568: Update java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java >> to accept vm flags >> 8319672: Several classloader tests ignore VM flags >> 8319676: A couple of jdk/modules/incubator/ tests ignore VM flags >> 8319677: Test jdk/internal/misc/VM/RuntimeArguments.java should be marked as >> flagless >> >> It converts the test to use `ProcessTools::createTestJavaProcessBuilder` or >> `createNativeTestJavaProcessBuilder` so that the test will support VM flags >> passed to jtreg. A couple tests that ignore VM flags should use >> `ProcessTools::createLimtiedTestJavaProcessBuilder` and marks the test with >> `@requires vm.flagless`. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Changes requested by lmesnik (Reviewer). test/jdk/jdk/modules/incubator/DefaultImage.java line 113: > 111: PrintStream ps = new PrintStream(baos); > 112: > 113: ProcessBuilder pb = > ProcessTools.createLimitedTestJavaProcessBuilder(opts); Shouldn't the test be marked as flagless? - PR Review: https://git.openjdk.org/jdk/pull/1#pullrequestreview-1735130788 PR Review Comment: https://git.openjdk.org/jdk/pull/1#discussion_r1396146084
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v5]
On Fri, 10 Nov 2023 01:49:17 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. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > variable was renamed. It is expected from `createLimitedTestJavaProcessBuilder` to execute the process exactly as specified in the test, without any additional changes from jtreg. It is very likely that the test might fail if the process is executed in other modes. I agree that usage of this method is always a potential loss of coverage for vm flags being tested and for virtual threads. So this method should be used only when it is necessary. As well as we should minimize the number of flagless testing. - PR Comment: https://git.openjdk.org/jdk/pull/16442#issuecomment-1813899682
Re: RFR: 8319572: Test jdk/incubator/vector/LoadJsvmlTest.java ignores VM flags
On Thu, 9 Nov 2023 22:08:06 GMT, Sandhya Viswanathan wrote: > Test jdk/incubator/vector/LoadJsvmlTest.java ignores VM flags and thus marked > as flagless through @requires vm.flagless per > [JDK-8319566](https://bugs.openjdk.org/browse/JDK-8319566). Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16589#pullrequestreview-1731069907
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v5]
On Fri, 10 Nov 2023 01:49:17 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. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > variable was renamed. The createLimitedTestJavaProcessBuilder is used in 410 tests (hotspot and jdk). 233 of them are flagless and not supposed to be executed with any additional VM flags. (They should be reviewed by it is a separate issue). 177 are not marked as flagless and should be updated to be flagless or use createTestJavaProcessBuilder. The 'createLimitedTestJavaProcessBuilder' method should be used only when the process not accept any flags and it is logical to assume that thread factory shouldn't be used either. I think it just makes consistent the createLimitedTestJavaProcessBuilder and createTestJavaProcessBuilder methods. It was not my original goal to add test thread factory part of createLimitedTestJavaProcessBuilder, I just missed that original createJavaProcessBuilder is applied to all processes. - PR Comment: https://git.openjdk.org/jdk/pull/16442#issuecomment-1804972301
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v5]
> 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. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: variable was renamed. - Changes: - all: https://git.openjdk.org/jdk/pull/16442/files - new: https://git.openjdk.org/jdk/pull/16442/files/aa93f71a..bc165dd6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16442=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16442=03-04 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/16442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16442/head:pull/16442 PR: https://git.openjdk.org/jdk/pull/16442
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v3]
On Wed, 8 Nov 2023 19:02:36 GMT, Mark Sheppard wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> converted list to array.
>
> test/lib/jdk/test/lib/process/ProcessTools.java line 387:
>
>> 385: */
>> 386:
>> 387: private static String[] addTestThreadFactoryArgs(String
>> testThreadFactoryName, String[] command) {
>
> would it be appropriate, at this juncture, to rename the method parameter
> "command" here, and throughout the associated code, to commandArgs, as the
> actual command i.e. java is added in createJavaProcessBuilder, and the
> parameter references the java command's args? or is that too much hassle.
done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16442#discussion_r1387085932
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v4]
> 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. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: renamed arguments - Changes: - all: https://git.openjdk.org/jdk/pull/16442/files - new: https://git.openjdk.org/jdk/pull/16442/files/4ba1e85e..aa93f71a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16442=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16442=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16442/head:pull/16442 PR: https://git.openjdk.org/jdk/pull/16442
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v3]
On Wed, 8 Nov 2023 02:33:29 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. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > converted list to array. Moved the threadFactory injection to createTestJavaProcessBuilder. I think that it is more logical to don't set it for limited version of process builder. - PR Comment: https://git.openjdk.org/jdk/pull/16442#issuecomment-1801121241
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v3]
> 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. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: converted list to array. - Changes: - all: https://git.openjdk.org/jdk/pull/16442/files - new: https://git.openjdk.org/jdk/pull/16442/files/db069ace..4ba1e85e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16442=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16442=01-02 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16442/head:pull/16442 PR: https://git.openjdk.org/jdk/pull/16442
Re: RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder() [v2]
> 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. 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 three additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into 8319200 - param removed - Moved ttf from limited builder. - Changes: - all: https://git.openjdk.org/jdk/pull/16442/files - new: https://git.openjdk.org/jdk/pull/16442/files/0dcf3673..db069ace Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16442=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16442=00-01 Stats: 31452 lines in 728 files changed: 16977 ins; 6081 del; 8394 mod Patch: https://git.openjdk.org/jdk/pull/16442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16442/head:pull/16442 PR: https://git.openjdk.org/jdk/pull/16442
Withdrawn: 8318839: Update test thread factory to catch all exceptions
On Wed, 25 Oct 2023 21:08:01 GMT, Leonid Mesnik wrote: > The jtreg starts the main thread in a separate ThreadGroup and checks > unhandled exceptions for this group. However, it doesn't catch all unhandled > exceptions. There is a jtreg issue for this > https://bugs.openjdk.org/browse/CODETOOLS-7903526. > Catching such issues for virtual threads is important because they are not > included in any groups. So this fix implements the handler for the test > thread factory. > > A few tests start failing. > > The test > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java > has testcases for platform and virtual threads. So, there is there's no need > to run it with the thread factory. > > The test > java/lang/Thread/virtual/ThreadAPI.java > tests UncaughtExceptionHandler and virtual threads. No need to run it with a > thread factory. > > Test > test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check the > default empty handler. > > Probably, we need some common approach about dealing with the > UncaughtExceptionHandler in jtreg. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/16369
Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
On Fri, 3 Nov 2023 03:44:31 GMT, Leonid Mesnik wrote: >> The jtreg starts the main thread in a separate ThreadGroup and checks >> unhandled exceptions for this group. However, it doesn't catch all unhandled >> exceptions. There is a jtreg issue for this >> https://bugs.openjdk.org/browse/CODETOOLS-7903526. >> Catching such issues for virtual threads is important because they are not >> included in any groups. So this fix implements the handler for the test >> thread factory. >> >> A few tests start failing. >> >> The test >> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java >> has testcases for platform and virtual threads. So, there is there's no need >> to run it with the thread factory. >> >> The test >> java/lang/Thread/virtual/ThreadAPI.java >> tests UncaughtExceptionHandler and virtual threads. No need to run it with a >> thread factory. >> >> Test >> test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check >> the default empty handler. >> >> Probably, we need some common approach about dealing with the >> UncaughtExceptionHandler in jtreg. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > Replaced System.exit() with exception. ok, seems there is no good way to process exceptions here. - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1792774893
Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
On Fri, 3 Nov 2023 03:44:31 GMT, Leonid Mesnik wrote: >> The jtreg starts the main thread in a separate ThreadGroup and checks >> unhandled exceptions for this group. However, it doesn't catch all unhandled >> exceptions. There is a jtreg issue for this >> https://bugs.openjdk.org/browse/CODETOOLS-7903526. >> Catching such issues for virtual threads is important because they are not >> included in any groups. So this fix implements the handler for the test >> thread factory. >> >> A few tests start failing. >> >> The test >> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java >> has testcases for platform and virtual threads. So, there is there's no need >> to run it with the thread factory. >> >> The test >> java/lang/Thread/virtual/ThreadAPI.java >> tests UncaughtExceptionHandler and virtual threads. No need to run it with a >> thread factory. >> >> Test >> test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check >> the default empty handler. >> >> Probably, we need some common approach about dealing with the >> UncaughtExceptionHandler in jtreg. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > Replaced System.exit() with exception. I updated the test thread factory to check if there are unhandled exceptions after the test is completed. It now works similarly to what I plan to fix in jtreg. So the purpose of this fix is to catch all exceptions with factory enabled and also to "pre-test" jtreg fix in some cases. - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1791859353
Re: RFR: 8318839: Update test thread factory to catch all exceptions [v2]
> The jtreg starts the main thread in a separate ThreadGroup and checks > unhandled exceptions for this group. However, it doesn't catch all unhandled > exceptions. There is a jtreg issue for this > https://bugs.openjdk.org/browse/CODETOOLS-7903526. > Catching such issues for virtual threads is important because they are not > included in any groups. So this fix implements the handler for the test > thread factory. > > A few tests start failing. > > The test > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java > has testcases for platform and virtual threads. So, there is there's no need > to run it with the thread factory. > > The test > java/lang/Thread/virtual/ThreadAPI.java > tests UncaughtExceptionHandler and virtual threads. No need to run it with a > thread factory. > > Test > test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check the > default empty handler. > > Probably, we need some common approach about dealing with the > UncaughtExceptionHandler in jtreg. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: Replaced System.exit() with exception. - Changes: - all: https://git.openjdk.org/jdk/pull/16369/files - new: https://git.openjdk.org/jdk/pull/16369/files/8fbb2798..b0878f35 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16369=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16369=00-01 Stats: 48 lines in 1 file changed: 37 ins; 10 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16369.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16369/head:pull/16369 PR: https://git.openjdk.org/jdk/pull/16369
RFR: 8319200: Don't use test thread factory in ProcessTools.createLimitedTestJavaProcessBuilder()
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. - Commit messages: - Moved ttf from limited builder. Changes: https://git.openjdk.org/jdk/pull/16442/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16442=00 Issue: https://bugs.openjdk.org/browse/JDK-8319200 Stats: 15 lines in 1 file changed: 8 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16442/head:pull/16442 PR: https://git.openjdk.org/jdk/pull/16442
Re: RFR: 8319153: Fix: Class is a raw type in ProcessTools
On Tue, 31 Oct 2023 07:43:43 GMT, Leo Korinth wrote: > Changing from `Class c` to `Class c` removes two warnings. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16431#pullrequestreview-1707376126
Re: RFR: 8318839: Update test thread factory to catch all exceptions
On Sun, 29 Oct 2023 14:10:32 GMT, Jaikiran Pai wrote: >> The jtreg starts the main thread in a separate ThreadGroup and checks >> unhandled exceptions for this group. However, it doesn't catch all unhandled >> exceptions. There is a jtreg issue for this >> https://bugs.openjdk.org/browse/CODETOOLS-7903526. >> Catching such issues for virtual threads is important because they are not >> included in any groups. So this fix implements the handler for the test >> thread factory. >> >> A few tests start failing. >> >> The test >> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java >> has testcases for platform and virtual threads. So, there is there's no need >> to run it with the thread factory. >> >> The test >> java/lang/Thread/virtual/ThreadAPI.java >> tests UncaughtExceptionHandler and virtual threads. No need to run it with a >> thread factory. >> >> Test >> test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check >> the default empty handler. >> >> Probably, we need some common approach about dealing with the >> UncaughtExceptionHandler in jtreg. > > Hello Leonid, in order to understand what exactly we are trying to solve > here, I ran a few tests to see how things work without the changes being > proposed in this PR. Here's my findings. > > A bit of background first. When jtreg runs, either in agent mode or othervm > mode, it creates a specific thread within which the actual test code runs. In > either of these modes, it uses a custom jtreg specific `ThreadGroup` instance > for this thread which is running the test code. This instance of > `ThreadGroup` has specific overridden implementation of the `public void > uncaughtException(Thread t, Throwable e)` API which keeps track of uncaught > exception that might have been thrown by any threads that could have been > spawned by the test. After `start()`ing the thread which runs the test code, > the jtreg framework then waits for that thread to complete and once completed > (either exceptionally or normally), jtreg framework then queries a state on > the custom `ThreadGroup` instance to see if any uncaught exception(s) were > received during the lifetime of this thread which ran that test. If it finds > any, then it marks the test as failed and reports such a failure > appropriately in the test re port. As noted, this applies for both the agent mode and other vm mode. Some important aspects of this implementation is that: > > - The custom `ThreadGroup` which has the overridden implementation of the > `uncaughtException(Thread t, Throwable e)` method doesn't interfere with the > JVM level default exception handler. > > - After the thread which ran the test code completes, the decision on whether > to fail or pass a test is taken by checking the custom `ThreadGroup`'s state. > Once this decision is done, the decision is immediately reported in relevant > ways and the test status is marked (i.e. finalized) at this point. > > - If this was running in agent vm mode, the agent vm mode continues to > operate and isn't terminated and thus is available for subsequent tests. This > point I think is important to remember for reasons that will be noted later > in this comment. > > Now coming to the part where in https://bugs.openjdk.org/browse/JDK-8303703 > we introduced a way where jtreg instead of creating a platform thread (backed > by its custom implementation of the `ThreadGroup`) to run the test code, now > checks the presence of a `ThreadFactory` and if present, lets it create a > `Thread` which runs this test code. In the JDK repo, we have an > implementation of a `ThreadFactory` which creates a virtual thread instead of > platform... @jaikiran, your analysis is correct. @jaikiran , @dholmes-ora The jtreg is going to be fixed to handle all uncaught exceptions. See PR: https://github.com/openjdk/jtreg/pull/172 The problem might happens not only with test thread factory, but for any virtual threads and might be for system threads. It is more generic solution and might take a lot of time to be correctly implemented. So this pr is just temporary fix until jtreg is updated. I could withdraw this PR, but not sure what are the risks/issues if I integrate it. We are going just to have a ugly error reporting for the uncaught threads when test thread factory is used or missed something? - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1785813862
Re: RFR: 8318839: Update test thread factory to catch all exceptions
On Fri, 27 Oct 2023 05:55:43 GMT, David Holmes wrote:
>> It is still used in tests and we should ignore it like jtreg doing.
>
> Shouldn't this code first retrieve the current default exception handler, and
> then check whether t is a virtual thread, and if so handle the exception as
> appropriate (not sure System.exit is appropriate ..). Then for non-virtual
> threads it delegates to the previous default handler.
>
> final UncaughtExceptionHandler originalUEH =
> Thread.getDefaultUncaughtExceptionHandler();
> Thread.setDefaultUncaughtExceptionHandler((t, e) -> {
> if (t.isVirtual()) {
> // ...
> } else {
> originalUEH.uncaughtException(t, e);
> }
> });
There shouldn't be an original UHE. jtreg doesn't set it. The goal is to add
this handler for all threads, not only virtual.
Please note, that it is planned to add it only until the similar problem in
jtreg is completely fixed.
I thought to add something like
Thread.setDefaultUncaughtExceptionHandler((t, e) -> {
UHE.ecxeptionThrown = e;
}
...
@Override
public Thread newThread(Runnable task) {
return VIRTUAL_TF.newThread(new Runnable() -> {
task.run();
if (UHE.ecxeptionThrown != null) {
throw new RuntimeException(UHE.ecxeptionThrown);
}
);
}
}
So test actually throws exceptions. It might miss exceptions for threads that
finish later than the main thread. but might it is ok.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16369#discussion_r1374790707
Re: RFR: 8318839: Update test thread factory to catch all exceptions
On Thu, 26 Oct 2023 08:34:39 GMT, Alan Bateman wrote: > Having a UHE invoke System.exit is surprising. Are you saying that this is > only for cases where a test launches a child VM with the thread factory set? It is for cases when the test is started in a virtual thread. I don't see a better way to process unexpected exceptions right now. I don't want to complicate the interface between jtreg and the plugin for this temporary fix. - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1782028651
Re: RFR: 8318839: Update test thread factory to catch all exceptions
On Thu, 26 Oct 2023 08:34:39 GMT, Alan Bateman wrote: > Stepping back a bit. ThreadGroup is legacy and we eventually want it to go > away. We've been deprecating and degrading it very slowly over many releases. > So I think jtreg will eventually need to change. Right now, it creates > AgentVMThreadGroup for agent VM mode so it controls the UHE where it starts > the "main thread". I think it will eventually need to change this to set the > system-wide UHE but this means it would handling uncaught exception thrown by > "system threads", we may have to audit some of the exception handling if > things come out of the woodwork. I think we shouldn't allow any unhandled exceptions in system threads also. This might just hide issues. The tests that might erase such exceptions should be written to for VM. - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1782033148
Re: RFR: 8318839: Update test thread factory to catch all exceptions
On Wed, 25 Oct 2023 21:08:01 GMT, Leonid Mesnik wrote: > The jtreg starts the main thread in a separate ThreadGroup and checks > unhandled exceptions for this group. However, it doesn't catch all unhandled > exceptions. There is a jtreg issue for this > https://bugs.openjdk.org/browse/CODETOOLS-7903526. > Catching such issues for virtual threads is important because they are not > included in any groups. So this fix implements the handler for the test > thread factory. > > A few tests start failing. > > The test > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java > has testcases for platform and virtual threads. So, there is there's no need > to run it with the thread factory. > > The test > java/lang/Thread/virtual/ThreadAPI.java > tests UncaughtExceptionHandler and virtual threads. No need to run it with a > thread factory. > > Test > test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check the > default empty handler. > > Probably, we need some common approach about dealing with the > UncaughtExceptionHandler in jtreg. > Hello Leonid, looking at the changes in this PR, what's being proposed is > that when jtreg launches tests through a virtual thread, then this wrapping > code will set a JVM level UncaughtExceptionHandler by calling > Thread.setDefaultUncaughtExceptionHandler(...). The implementation of this > UncaughtExceptionHandler calls System.exit(1). Wouldn't that kill the test > VM? I think that would then impact everything else including jtreg report > generation and such for the test, isn't it? The jtreg correctly reports such failures. It is expected that JVM might fail. The only difference is that the reason for failure is System.exit(1) and the exception. > I had a look at https://bugs.openjdk.org/browse/JDK-8318839 but it doesn't > have enough details to help understand what currently happens when a test > launched through a virtual thread from jtreg throws an uncaught exception. > How/what gets reported for that test execution? The jtreg correctly catches and reports failures thrown by the main virtual thread. However, it ignores exceptions thrown by any other threads started by the test. For platform threads, jtreg uses ThreafGroup (AgentVMThreadGroup or MainThreadGroup) to report failures in other threads. However, there is no way to use such an approach to catch exceptions for all test threads when virtual threads are used. See for details: https://github.com/openjdk/jtreg/blob/ef3865581bdfc55c6315a8538222fc3a91b2b872/src/share/classes/com/sun/javatest/regtest/agent/MainWrapper.java#L72 I have a PR to implement the global exception handling here: https://github.com/openjdk/jtreg/pull/172 Now it is the only proposal and might take a long time to complete it. So the current fix helps us to find issues with the virtual thread test factory until jtreg is fixed. - PR Comment: https://git.openjdk.org/jdk/pull/16369#issuecomment-1782021953
Re: RFR: 8318839: Update test thread factory to catch all exceptions
On Thu, 26 Oct 2023 06:09:34 GMT, Jaikiran Pai wrote:
>> The jtreg starts the main thread in a separate ThreadGroup and checks
>> unhandled exceptions for this group. However, it doesn't catch all unhandled
>> exceptions. There is a jtreg issue for this
>> https://bugs.openjdk.org/browse/CODETOOLS-7903526.
>> Catching such issues for virtual threads is important because they are not
>> included in any groups. So this fix implements the handler for the test
>> thread factory.
>>
>> A few tests start failing.
>>
>> The test
>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java
>> has testcases for platform and virtual threads. So, there is there's no need
>> to run it with the thread factory.
>>
>> The test
>> java/lang/Thread/virtual/ThreadAPI.java
>> tests UncaughtExceptionHandler and virtual threads. No need to run it with a
>> thread factory.
>>
>> Test
>> test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check
>> the default empty handler.
>>
>> Probably, we need some common approach about dealing with the
>> UncaughtExceptionHandler in jtreg.
>
> test/jtreg_test_thread_factory/src/share/classes/Virtual.java line 37:
>
>> 35: // The virtual threads don't belong to any group and need
>> global handler.
>> 36: Thread.setDefaultUncaughtExceptionHandler((t, e) -> {
>> 37: if (e instanceof ThreadDeath) {
>
> `ThreadDeath` has been deprecated for removal since Java 20, so this should
> no longer be needed.
It is still used in tests and we should ignore it like jtreg doing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16369#discussion_r1373886743
RFR: 8318839: Update test thread factory to catch all exceptions
The jtreg starts the main thread in a separate ThreadGroup and checks unhandled exceptions for this group. However, it doesn't catch all unhandled exceptions. There is a jtreg issue for this https://bugs.openjdk.org/browse/CODETOOLS-7903526. Catching such issues for virtual threads is important because they are not included in any groups. So this fix implements the handler for the test thread factory. A few tests start failing. The test serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorVMEventsTest.java has testcases for platform and virtual threads. So, there is there's no need to run it with the thread factory. The test java/lang/Thread/virtual/ThreadAPI.java tests UncaughtExceptionHandler and virtual threads. No need to run it with a thread factory. Test test/jdk/java/util/concurrent/tck/ThreadTest.java is updated to not check the default empty handler. Probably, we need some common approach about dealing with the UncaughtExceptionHandler in jtreg. - Commit messages: - 8318839: Update test thread factory to catch all exceptions Changes: https://git.openjdk.org/jdk/pull/16369/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16369=00 Issue: https://bugs.openjdk.org/browse/JDK-8318839 Stats: 18 lines in 4 files changed: 15 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16369.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16369/head:pull/16369 PR: https://git.openjdk.org/jdk/pull/16369
Re: RFR: 8315097: Rename createJavaProcessBuilder [v6]
On Tue, 24 Oct 2023 07:49:30 GMT, Leo Korinth wrote: >> This pull request renames `createJavaProcessBuilder` to >> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to >> `createTestJavaProcessBuilder`. Both are implemented through a private >> `createJavaProcessBuilder`. It also updates the java doc. >> >> This is so that it should be harder to by mistake use >> `createLimitedTestJavaProcessBuilder` that is problematic because it will >> not forward JVM flags to the tested JVM. > > 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 lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1695447658
RFR: 8316452: java/lang/instrument/modules/AppendToClassPathModuleTest.java ignores VM flags
The test uses specific classpath, and jar and is intended to test modules. So I marked is as flagless, - Commit messages: - 8316452 Changes: https://git.openjdk.org/jdk/pull/16080/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16080=00 Issue: https://bugs.openjdk.org/browse/JDK-8316452 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16080.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16080/head:pull/16080 PR: https://git.openjdk.org/jdk/pull/16080
RFR: 8316464: 3 sun/tools tests ignore VM flags
I marked tests sun/tools/jcmd/TestProcessHelper.java sun/tools/jinfo/JInfoTest.java as headless. They used different specific VM options and are not worth executing with other VM flags. And fixed sun/tools/jstat/JStatInterval.java Tested with tier1, local execution of tests, and running sun/tools/jstat/JStatInterval.java with different options. - Commit messages: - 8316464 Changes: https://git.openjdk.org/jdk/pull/16078/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16078=00 Issue: https://bugs.openjdk.org/browse/JDK-8316464 Stats: 9 lines in 3 files changed: 3 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16078.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16078/head:pull/16078 PR: https://git.openjdk.org/jdk/pull/16078
RFR: 8316445: Mark com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java as vm.flagless
Test com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java check how beans work with VM flags and ignore external flags. It doesn't make sense to run it with external options so just mark it as flagless. Tested with running tier1 and test with/without additional options. - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/15823/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15823=00 Issue: https://bugs.openjdk.org/browse/JDK-8316445 Stats: 3 lines in 2 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15823.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15823/head:pull/15823 PR: https://git.openjdk.org/jdk/pull/15823
Re: RFR: 8315097: Rename createJavaProcessBuilder
On Mon, 28 Aug 2023 15:54:08 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. The. changes looks good. Please update copyrights for changed filed. I expect that you completed execution of all tests to ensure that nothing is broken. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1598884378
Integrated: 8314330: java/foreign tests should respect vm flags when start new processes
On Wed, 16 Aug 2023 00:14:47 GMT, Leonid Mesnik wrote: > The test helper which spawn new jvms is updated to start them using VM flags > for testing. This pull request has now been integrated. Changeset: 7b28d360 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/7b28d3608a10b26af376c8f6d142d97c708c9f11 Stats: 11 lines in 1 file changed: 0 ins; 8 del; 3 mod 8314330: java/foreign tests should respect vm flags when start new processes Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/15302
Re: RFR: 8314330: java/foreign tests should respect vm flags when start new processes [v2]
> The test helper which spawn new jvms is updated to start them using VM flags > for testing. 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/15302/files - new: https://git.openjdk.org/jdk/pull/15302/files/27f5eb83..23184fc1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15302=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15302=00-01 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15302.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15302/head:pull/15302 PR: https://git.openjdk.org/jdk/pull/15302
RFR: 8314330: java/foreign tests should respect vm flags when start new processes
The test helper which spawn new jvms is updated to start them using VM flags for testing. - Commit messages: - 8314330: java/foreign tests should respect vm flags when start new processes Changes: https://git.openjdk.org/jdk/pull/15302/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15302=00 Issue: https://bugs.openjdk.org/browse/JDK-8314330 Stats: 10 lines in 1 file changed: 1 ins; 7 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15302.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15302/head:pull/15302 PR: https://git.openjdk.org/jdk/pull/15302
Re: [jdk21] RFR: 8310822: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on generic-x64
On Fri, 23 Jun 2023 19:30:56 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/lang/ScopedValue/StressStackOverflow.java > on generic-x64. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/60#pullrequestreview-1495844009
Re: [jdk21] RFR: 8310822: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on generic-x64
On Fri, 23 Jun 2023 19:30:56 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/lang/ScopedValue/StressStackOverflow.java > on generic-x64. 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/lang/ScopedValue/StressStackOverflow.java 8308609 generic-x64 java/lang/ScopedValue/StressStackOverflow.java#no-vmcontinuations ? - PR Review: https://git.openjdk.org/jdk21/pull/60#pullrequestreview-1495822775 PR Review Comment: https://git.openjdk.org/jdk21/pull/60#discussion_r1240249942
Re: RFR: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper
On Fri, 2 Jun 2023 21:30:46 GMT, Chris Plummer wrote: > Normally when a virtual thread wrapper is used to run a test, the main thread > is renamed to "old-m-a-i-n" and the new virtual thread that will act as the > main thread is named "main". Neither is being done by `ProcessTools.main()`. > This can cause problems for tests that expect the main thread that the test > is running in to be called "main". It is instead left unnamed. This is > causing the following 4 tests to fail: > > com/sun/jdi/JdbMethodExitTest.java > com/sun/jdi/JdbStepTest.java > com/sun/jdi/JdbStopThreadTest.java > com/sun/jdi/JdbStopThreadidTest.java > > These tests also fail due to > [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be > fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due to > [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will also > subsequently be fixed. > > Note this fix messed up one runtime test. It was expecting an exception > message to mention the "main" thread rather than "old-m-a-i-n". Loosening the > exception message matching pattern a bit solved the problem. > > Testing was done by running all of tier1 and tier5. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14292#pullrequestreview-1458712928
Re: RFR: 8308475: Make the thread dump files generated by jcmd Thread.dump_to_file jtreg failure handler action easily accessible
On Sun, 21 May 2023 09:19:47 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to improve the > accessibility of the thread dump files that are generated by the `jcmd > Thread.dump_to_file` command configured in the failure handler > configurations? This addresses https://bugs.openjdk.org/browse/JDK-8308475. > > The changes in this PR include: > - Enhancement to `GatherProcessInfoTimeoutHandler` to allow configuring a > `successArtifacts` action parameter which can be used to generate links to > files that are generated by the failure handler commands. > - Introduction of a `%iterCount` token to allow ability to refer to the > current iteration when the command is repeated > - The `jcmd Thread.dump_to_file` is now configured to create the thread > dumps in `json` format. Additionally, it has now been configured to create > the thread dumps 6 times, just like the `jstack` command. > > Detailed explanation of the `successArtifacts` parameter and the `%iterCount` > token is provided in the JBS comment here > https://bugs.openjdk.org/browse/JDK-8308475?focusedCommentId=14583072=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14583072 > > Tests have been run locally with this change and tier1, tier2 and tier3 tests > on CI system to verify this change works and doesn't cause regression. Cool! - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14071#pullrequestreview-1442947346
Integrated: 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 pull request has now been integrated. Changeset: 563152f3 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/563152f32dd2c8617c0e0955d55c5bbce23627fb Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8308223: failure handler missed jcmd.vm.info command Reviewed-by: stefank - PR: https://git.openjdk.org/jdk/pull/14018
RFR: 8308223: failure handler missed jcmd.vm.info command
Trivial fix that added missed useful command. Tested by running make of failure handler and verifying results. - Commit messages: - 8308223 Changes: https://git.openjdk.org/jdk/pull/14018/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14018=00 Issue: https://bugs.openjdk.org/browse/JDK-8308223 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14018.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14018/head:pull/14018 PR: https://git.openjdk.org/jdk/pull/14018
Re: RFR: 8307966: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java on linux-x64
On Fri, 12 May 2023 00:05:21 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java > on linux-x64. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13946#pullrequestreview-1423608862
Re: RFR: 8307966: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java on linux-x64
On Fri, 12 May 2023 00:05:21 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java > on linux-x64. Might be this failure is platform agnostic and should be generic all. We just don't run this combination on macosx so often. ( - PR Review: https://git.openjdk.org/jdk/pull/13946#pullrequestreview-1423603714
Integrated: 8307307: Improve ProcessTools.java to don't try to run Virtual wrapper for incompatible processes
On Thu, 4 May 2023 15:52:36 GMT, Leonid Mesnik wrote: > The ProcessTools has some support of jtreg thread factory functionality. > It tries to run the new process using virtual thread to run `main()` method. > This fix updates it to skip the java runs where no main class is involved and > more correctly process options which has 2nd argument. > Also is sets `main.wrapper` property to allow launched process understand id > any wrappers is used. This pull request has now been integrated. Changeset: 7f05f6f7 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/7f05f6f7c77c10dd2aed291af20664c9130e35f9 Stats: 35 lines in 2 files changed: 16 ins; 4 del; 15 mod 8307307: Improve ProcessTools.java to don't try to run Virtual wrapper for incompatible processes Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/13808
Re: RFR: 8307307: Improve ProcessTools.java to don't try to run Virtual wrapper for incompatible processes [v3]
> The ProcessTools has some support of jtreg thread factory functionality. > It tries to run the new process using virtual thread to run `main()` method. > This fix updates it to skip the java runs where no main class is involved and > more correctly process options which has 2nd argument. > Also is sets `main.wrapper` property to allow launched process understand id > any wrappers is used. 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 four additional commits since the last revision: - excluded failed tests - Merge branch 'master' of https://github.com/openjdk/jdk into 8307307 - fixed some params - 8307307: Improve ProcessTools.java to don't try to run Virtual wrapper for incompatible processes - Changes: - all: https://git.openjdk.org/jdk/pull/13808/files - new: https://git.openjdk.org/jdk/pull/13808/files/106be911..bbe57f2a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13808=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13808=01-02 Stats: 21542 lines in 477 files changed: 14730 ins; 3355 del; 3457 mod Patch: https://git.openjdk.org/jdk/pull/13808.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13808/head:pull/13808 PR: https://git.openjdk.org/jdk/pull/13808
Re: RFR: 8307307: Improve ProcessTools.java to don't try to run Virtual wrapper for incompatible processes [v2]
> The ProcessTools has some support of jtreg thread factory functionality. > It tries to run the new process using virtual thread to run `main()` method. > This fix updates it to skip the java runs where no main class is involved and > more correctly process options which has 2nd argument. > Also is sets `main.wrapper` property to allow launched process understand id > any wrappers is used. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed some params - Changes: - all: https://git.openjdk.org/jdk/pull/13808/files - new: https://git.openjdk.org/jdk/pull/13808/files/0047016b..106be911 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13808=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13808=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13808.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13808/head:pull/13808 PR: https://git.openjdk.org/jdk/pull/13808
RFR: 8307486: ProcessTools.java should wait until vthread is completed before checking exceptions
Updated processtools to check exception after join(). Tested with running CI virtual thread tests. - Commit messages: - 8307486: ProcessTools.java should wait until vthread is completed before checking exceptions Changes: https://git.openjdk.org/jdk/pull/13873/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13873=00 Issue: https://bugs.openjdk.org/browse/JDK-8307486 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13873.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13873/head:pull/13873 PR: https://git.openjdk.org/jdk/pull/13873
RFR: 8307307: Improve ProcessTools.java to don't try to run Virtual wrapper for incompatible processes
The ProcessTools has some support of jtreg thread factory functionality. It tries to run the new process using virtual thread to run `main()` method. This fix updates it to skip the java runs where no main class is involved and more correctly process options which has 2nd argument. Also is sets `main.wrapper` property to allow launched process understand id any wrappers is used. - Commit messages: - 8307307: Improve ProcessTools.java to don't try to run Virtual wrapper for incompatible processes Changes: https://git.openjdk.org/jdk/pull/13808/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13808=00 Issue: https://bugs.openjdk.org/browse/JDK-8307307 Stats: 33 lines in 1 file changed: 14 ins; 4 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/13808.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13808/head:pull/13808 PR: https://git.openjdk.org/jdk/pull/13808
Integrated: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output"
On Thu, 27 Apr 2023 01:06:23 GMT, Leonid Mesnik wrote: > The ProcessTools.startProcess (...) has been updated to completely read > streams after process has been completed. > The test was updated to run 5 times with different number of lines and line > sizes. This pull request has now been integrated. Changeset: 64ac9a05 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/64ac9a05e85020d24e33ba55cffa1bd9b269218a Stats: 63 lines in 2 files changed: 29 ins; 18 del; 16 mod 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" Reviewed-by: dholmes - PR: https://git.openjdk.org/jdk/pull/13683
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v4]
> The ProcessTools.startProcess (...) has been updated to completely read > streams after process has been completed. > The test was updated to run 5 times with different number of lines and line > sizes. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: catching Cancellation exception - Changes: - all: https://git.openjdk.org/jdk/pull/13683/files - new: https://git.openjdk.org/jdk/pull/13683/files/d02b889a..8f350c8f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13683=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13683=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v3]
On Thu, 27 Apr 2023 16:35:59 GMT, Leonid Mesnik wrote: >> The ProcessTools.startProcess (...) has been updated to completely read >> streams after process has been completed. >> The test was updated to run 5 times with different number of lines and line >> sizes. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > move buffers registration before pumping start point The testing: - tier1 - tier5 to verify that nothing is brokens, - run jdk/test/lib/process/ProcessToolsStartProcessTest.java with 50 iterations instead of 5 on each platform 100 times - manually added some thread.sleep into startProcess and verify that not failures still appear - PR Comment: https://git.openjdk.org/jdk/pull/13683#issuecomment-1526061664
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v3]
> The ProcessTools.startProcess (...) has been updated to completely read > streams after process has been completed. > The test was updated to run 5 times with different number of lines and line > sizes. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: move buffers registration before pumping start point - Changes: - all: https://git.openjdk.org/jdk/pull/13683/files - new: https://git.openjdk.org/jdk/pull/13683/files/10f1c5c2..d02b889a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13683=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13683=01-02 Stats: 28 lines in 1 file changed: 10 ins; 9 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v2]
On Thu, 27 Apr 2023 04:59:00 GMT, David Holmes wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix
>
> test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java line 79:
>
>> 77: System.out.print("FAILED: wrong number of lines in
>> Consumer output\n");
>> 78: success = false;
>> 79: System.out.print(out.getStdout());
>
> Why isn't this printing output?
fixed.
> test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java line 95:
>
>> 93: public static void main(String[] args) throws Exception {
>> 94: if (args.length > 0) {
>> 95: for (int i = 0; i < Integer.parseInt(args[0]); i++) {
>
> This will call parseInt on each iteration of the loop.
fixed.
> test/lib/jdk/test/lib/process/ProcessTools.java line 183:
>
>> 181: // It is needed to wait until stream is flushed
>> after
>> 182: // process is completed.
>> 183: task.get();
>
> This looks problematic - if you block here you are holding the object monitor
> as this is a synchronized method.
You are right. It is needed to give a chance to write to this buffer. fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178707275
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178707373
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178708269
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v2]
> The ProcessTools.startProcess (...) has been updated to completely read > streams after process has been completed. > The test was updated to run 5 times with different number of lines and line > sizes. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fix - Changes: - all: https://git.openjdk.org/jdk/pull/13683/files - new: https://git.openjdk.org/jdk/pull/13683/files/c16d33f8..10f1c5c2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13683=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13683=00-01 Stats: 9 lines in 2 files changed: 4 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output"
The ProcessTools.startProcess (...) has been updated to completely read streams after process has been completed. The test was updated to run 5 times with different number of lines and line sizes. - Commit messages: - fixed num of iters - fix Changes: https://git.openjdk.org/jdk/pull/13683/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13683=00 Issue: https://bugs.openjdk.org/browse/JDK-8306946 Stats: 63 lines in 2 files changed: 26 ins; 21 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
Integrated: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Fri, 21 Apr 2023 21:43:39 GMT, Leonid Mesnik wrote: > ProcessTools.startProcess() creates process and read it's output error > streams. So the any other using of corresponding Process.getInputStream() and > Process.getErrorStream() doesn't get process streams. > > This fix preserve process streams content and allow to read reuse the date. > The ByteArrayOutputStream is used as a buffer. > It stores all process output, never trying to clean date which has been read. > > The regression test has been provided with issue. > > I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake > instead of updating it. > > I run all tests to ensure that no failures are introduced. This pull request has now been integrated. Changeset: 2e340e85 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/2e340e855b760e381793107f2a4d74095bd40199 Stats: 211 lines in 3 files changed: 199 ins; 2 del; 10 mod 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/13594
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time [v2]
On Wed, 26 Apr 2023 09:38:45 GMT, Serguei Spitsyn wrote: >> Right. From the API caller's POV, it is asking for InputStreams that it can >> use to read the process' stdout or stderr streams. > > Okay, thanks. > I'm thinking about simple/short comments on this to make it clear this naming > mismatch is intentional. There is a comment in line 156 explaining the purpose of these streams. Also, I renamed out/err names to stdOut/stdErr to make it clearer. - PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1177949748
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time [v2]
> ProcessTools.startProcess() creates process and read it's output error > streams. So the any other using of corresponding Process.getInputStream() and > Process.getErrorStream() doesn't get process streams. > > This fix preserve process streams content and allow to read reuse the date. > The ByteArrayOutputStream is used as a buffer. > It stores all process output, never trying to clean date which has been read. > > The regression test has been provided with issue. > > I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake > instead of updating it. > > I run all tests to ensure that no failures are introduced. Leonid Mesnik has updated the pull request incrementally with two additional commits since the last revision: - fixed - renamed out to stdOut - Changes: - all: https://git.openjdk.org/jdk/pull/13594/files - new: https://git.openjdk.org/jdk/pull/13594/files/782e9cd6..b8bb6cb9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13594=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13594=00-01 Stats: 15 lines in 1 file changed: 1 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/13594.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13594/head:pull/13594 PR: https://git.openjdk.org/jdk/pull/13594
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Tue, 25 Apr 2023 03:06:09 GMT, Serguei Spitsyn wrote:
>> ProcessTools.startProcess() creates process and read it's output error
>> streams. So the any other using of corresponding Process.getInputStream()
>> and Process.getErrorStream() doesn't get process streams.
>>
>> This fix preserve process streams content and allow to read reuse the date.
>> The ByteArrayOutputStream is used as a buffer.
>> It stores all process output, never trying to clean date which has been
>> read.
>>
>> The regression test has been provided with issue.
>>
>> I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake
>> instead of updating it.
>>
>> I run all tests to ensure that no failures are introduced.
>
> test/lib/jdk/test/lib/process/ProcessTools.java line 792:
>
>> 790: @Override
>> 791: public InputStream getInputStream() {
>> 792: return out;
>
> This is a little bit confusing that the `getInputStream()` returns `out`
> stream.
> Just wanted to double-check if it is intentional and was not needed for
> `getOutputStream()` instead.
Agree, it is confusing, even in standard j.l.Process API . The `InputStream
java.lang.Process.getInputStream()`" returns **output** stream of started
process. So for our implementation ProcessImpl the 'out' and 'err' mean output
and error streams. However they are returned as InputStreams so users could
read them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1175985058
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Mon, 24 Apr 2023 20:57:31 GMT, Chris Plummer wrote: >> ProcessTools.startProcess() creates process and read it's output error >> streams. So the any other using of corresponding Process.getInputStream() >> and Process.getErrorStream() doesn't get process streams. >> >> This fix preserve process streams content and allow to read reuse the date. >> The ByteArrayOutputStream is used as a buffer. >> It stores all process output, never trying to clean date which has been >> read. >> >> The regression test has been provided with issue. >> >> I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake >> instead of updating it. >> >> I run all tests to ensure that no failures are introduced. > > test/lib/jdk/test/lib/process/ProcessTools.java line 249: > >> 247: stdout.addOutputStream(out.getOutputStream()); >> 248: stderr.addOutputStream(err.getOutputStream()); >> 249: > > Overall this looks good, although I'm a bit unclear on how some of the > underpinnings work, allowing the output to appear in these output streams, > and also in the LineForwarder output (above), and for that matter, in the > lineConsumer output (below). I guess there is some multiplexing of the output > that I just don't grasp, but appears to be something that already worked. StreamPumper read process stdout, stderr streams and write read data to registered streams. So it works as multiplexer. - PR Review Comment: https://git.openjdk.org/jdk/pull/13594#discussion_r1175930914
RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
ProcessTools.startProcess() creates process and read it's output error streams. So the any other using of corresponding Process.getInputStream() and Process.getErrorStream() doesn't get process streams. This fix preserve process streams content and allow to read reuse the date. The ByteArrayOutputStream is used as a buffer. It stores all process output, never trying to clean date which has been read. The regression test has been provided with issue. I closed previous PR https://github.com/openjdk/jdk/pull/13560 by mistake instead of updating it. I run all tests to ensure that no failures are introduced. - Commit messages: - copyrights fixed - typo fixed - fixed to use buffer. - JStackStressTest.java updated. - 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time Changes: https://git.openjdk.org/jdk/pull/13594/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13594=00 Issue: https://bugs.openjdk.org/browse/JDK-8233725 Stats: 210 lines in 3 files changed: 198 ins; 2 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13594.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13594/head:pull/13594 PR: https://git.openjdk.org/jdk/pull/13594
Withdrawn: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
On Thu, 20 Apr 2023 16:09:29 GMT, Leonid Mesnik wrote: > ProcessTools.startProcess() creates process and read it's output error > streams. So the any other using of corresponding Process.getInputStream() and > Process.getErrorStream() doesn't get process streams. > > This fix preserve process streams content and allow to read it after process > completion. The another possible solution would be to throw exception when > user tries to read already drained streams to fail tests earlier. However it > complicates usage of ProcessTools.startProcess() methods. > > The regression test has been provided with issue. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/13560
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time [v2]
On Thu, 20 Apr 2023 22:54:49 GMT, David Holmes wrote:
>> test/lib/jdk/test/lib/process/ProcessTools.java line 750:
>>
>>> 748: public InputStream getInputStream() {
>>> 749: try {
>>> 750: waitFor();
>>
>> With this added `waitFor()` the assumption now is that the caller doesn't
>> intent to do incremental reads of the output as the process generates it.
>> For example, if the test were to send some command to the process and then
>> want to read the resulting output, and do this repeatedly, it won't able to
>> use the InputStream to do that.
>
> I have to agree with Chris. You are changing a fundamental property of this
> API. We no longer just start the process, we are forced to wait for it to
> complete.
Exactly. I added note about implementation in the javadoc to make it clear. I
don't see any good solution for this problem.
The only other possible solution which I see is to throw Exception here,
forcing user to use lineConsumer.
However the any usage of OutputAnalyzer with startProcess() would clearly and
quickly fails.
Also, the
public static Process startProcess(String name,
ProcessBuilder processBuilder)
could be modified to allow to read process streams. The test should drain tests
by itself in such case to avoid hang.
However, it don't see any good way to implement this method correctly if
already read the process streams.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13560#discussion_r1173161520
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time [v2]
On Thu, 20 Apr 2023 22:22:06 GMT, Chris Plummer wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> JStackStressTest.java updated.
>
> test/jdk/sun/tools/jhsdb/JStackStressTest.java line 86:
>
>> 84: } catch (InterruptedException e) {
>> 85: }
>> 86: OutputAnalyzer jshellOutput = new
>> OutputAnalyzer(jShellProcess);
>
> It's not clear to me how moving this is fixing anything.
It is the main issues with current approach. The outputanalyzer tries to read
from streams which are available only when process finishes. This test interact
with process so it just hangs waiting of process completion.
> test/jdk/sun/tools/jstatd/JstatdTest.java line 357:
>
>> 355: assertEquals(stdout.size(), 1, "Output should contain one
>> line");
>> 356: assertTrue(stdout.get(0).startsWith("jstatd started"), "List
>> should start with 'jstatd started'");
>> 357: assertNotEquals(output.getExitValue(), 0,
>
> Before your fix, was the "jstatd started" line being missed because of this
> bug.
Yep. the output was "empty" before fix.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13560#discussion_r1173155279
PR Review Comment: https://git.openjdk.org/jdk/pull/13560#discussion_r1173155433
Re: RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time [v2]
> ProcessTools.startProcess() creates process and read it's output error > streams. So the any other using of corresponding Process.getInputStream() and > Process.getErrorStream() doesn't get process streams. > > This fix preserve process streams content and allow to read it after process > completion. The another possible solution would be to throw exception when > user tries to read already drained streams to fail tests earlier. However it > complicates usage of ProcessTools.startProcess() methods. > > The regression test has been provided with issue. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: JStackStressTest.java updated. - Changes: - all: https://git.openjdk.org/jdk/pull/13560/files - new: https://git.openjdk.org/jdk/pull/13560/files/c1534166..38d82fa5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13560=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13560=00-01 Stats: 3 lines in 1 file changed: 1 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13560.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13560/head:pull/13560 PR: https://git.openjdk.org/jdk/pull/13560
RFR: 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time
ProcessTools.startProcess() creates process and read it's output error streams. So the any other using of corresponding Process.getInputStream() and Process.getErrorStream() doesn't get process streams. This fix preserve process streams content and allow to read it after process completion. The another possible solution would be to throw exception when user tries to read already drained streams to fail tests earlier. However it complicates usage of ProcessTools.startProcess() methods. The regression test has been provided with issue. - Commit messages: - 8233725: ProcessTools.startProcess() has output issues when using an OutputAnalyzer at the same time Changes: https://git.openjdk.org/jdk/pull/13560/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13560=00 Issue: https://bugs.openjdk.org/browse/JDK-8233725 Stats: 180 lines in 3 files changed: 163 ins; 2 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/13560.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13560/head:pull/13560 PR: https://git.openjdk.org/jdk/pull/13560
Re: RFR: 8304896: Update to use jtreg 7.2
On Mon, 17 Apr 2023 14:56:16 GMT, Christian Stein wrote: > Please review the change to update to using jtreg 7.2. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the requiredVersion has been updated in the various `TEST.ROOT` > files. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13496#pullrequestreview-1388984542
Integrated: 8305875: Test TraceVirtualThreadLocals should be run with continuations only
On Tue, 11 Apr 2023 23:38:23 GMT, Leonid Mesnik wrote: > Test TraceVirtualThreadLocals verifies that thread locals are dumped for > virtual threads. It fails when continuations are not available and virtual > threads are emulated. > > The test failed on linux-x86 so I just want to mark it to have green github > actions results. This pull request has now been integrated. Changeset: 92521b10 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/92521b100f1eb785eabd101870f631f555c3b135 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8305875: Test TraceVirtualThreadLocals should be run with continuations only Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/13436
Re: RFR: 8305875: Test TraceVirtualThreadLocals should be run with continuations only
On Tue, 11 Apr 2023 23:38:23 GMT, Leonid Mesnik wrote: > Test TraceVirtualThreadLocals verifies that thread locals are dumped for > virtual threads. It fails when continuations are not available and virtual > threads are emulated. > > The test failed on linux-x86 so I just want to mark it to have green github > actions results. The test verifies how property jdk.traceVirtualThreadLocals works to dump info. It works with virtual threads only. - PR Comment: https://git.openjdk.org/jdk/pull/13436#issuecomment-1504729526
RFR: 8305875: Test TraceVirtualThreadLocals should be run with continuations only
Test TraceVirtualThreadLocals verifies that thread locals are dumped for virtual threads. It fails when continuations are not available and virtual threads are emulated. The test failed on linux-x86 so I just want to mark it to have green github actions results. - Commit messages: - fixed TraceVirtualThreadLocals.java Changes: https://git.openjdk.org/jdk/pull/13436/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13436=00 Issue: https://bugs.openjdk.org/browse/JDK-8305875 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13436.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13436/head:pull/13436 PR: https://git.openjdk.org/jdk/pull/13436
Re: RFR: 8304919: Implementation of Virtual Threads [v4]
On Wed, 29 Mar 2023 08:00:36 GMT, Alan Bateman wrote: >> JEP 444 proposes to make virtual threads a permanent feature in Java 21. The >> APIs that were preview APIs in Java 19/20 are changed to permanent and their >> `@since`/equivalent are changed to 21 (as per the guidance in JEP 12). The >> JNI and JVMTI versions are bumped as this is the first change in 21 to need >> the new version number. A lot of tests are updated to drop `@enablePreview` >> and --enable-preview. >> >> There is one API change from Java 19/20, the preview API >> Thread.Builder.allowSetThreadLocals(boolean) is dropped. This requires an >> update to the JVMTI GetThreadInfo implementation to read the TCCL >> consistently. >> >> In addition, there are a small number of implementation changes to sync up >> from the loom fibers branch: >> >> - A number of stack frames are `@Hidden` to reduce noise in the stack >> traces. This exposed a few issues with the stack walker code. More >> specifically, the cases where end of a continuation falls precisely at the >> end of the batch, or where the remaining frames are hidden, weren't handled >> correctly. >> - The code to emit the JFR jdk.ThreadSleepEvent is refactored so it's in >> Thread rather than in two classes. >> - A few robustness improvements for OOME and SOE. There is more to do here, >> for future PRs. >> - New system property to print a stack trace when a virtual thread sets its >> own value of a TL. >> - ThreadPerTaskExecutor is changed to use FutureTask. >> >> Testing: tier1-6. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Fix ThreadSleepEvent again Test changes looks good. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13203#pullrequestreview-1363976888
Re: RFR: 8304919: Implementation of Virtual Threads [v2]
On Wed, 29 Mar 2023 00:08:21 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/System.java line 2566:
>>
>>> 2564:
>>> 2565: public V executeOnCarrierThread(Callable task)
>>> throws Exception {
>>> 2566: if (Thread.currentThread() instanceof VirtualThread
>>> vthread) {
>>
>> Any specific reason to don't use Thread.currentThread().isVirtual()?
>
> To use the pattern variable to call `executeOnCarrierThread` on it?
ough, missed the last words in the line, thanks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151273326
Re: RFR: 8304919: Implementation of Virtual Threads [v2]
On Tue, 28 Mar 2023 19:36:18 GMT, Alan Bateman wrote:
>> JEP 444 proposes to make virtual threads a permanent feature in Java 21. The
>> APIs that were preview APIs in Java 19/20 are changed to permanent and their
>> `@since`/equivalent are changed to 21 (as per the guidance in JEP 12). The
>> JNI and JVMTI versions are bumped as this is the first change in 21 to need
>> the new version number. A lot of tests are updated to drop `@enablePreview`
>> and --enable-preview.
>>
>> There is one API change from Java 19/20, the preview API
>> Thread.Builder.allowSetThreadLocals(boolean) is dropped. This requires an
>> update to the JVMTI GetThreadInfo implementation to read the TCCL
>> consistently.
>>
>> In addition, there are a small number of implementation changes to sync up
>> from the loom fibers branch:
>>
>> - A number of stack frames are `@Hidden` to reduce noise in the stack
>> traces. This exposed a few issues with the stack walker code. More
>> specifically, the cases where end of a continuation falls precisely at the
>> end of the batch, or where the remaining frames are hidden, weren't handled
>> correctly.
>> - The code to emit the JFR jdk.ThreadSleepEvent is refactored so it's in
>> Thread rather than in two classes.
>> - A few robustness improvements for OOME and SOE. There is more to do here,
>> for future PRs.
>> - New system property to print a stack trace when a virtual thread sets its
>> own value of a TL.
>> - ThreadPerTaskExecutor is changed to use FutureTask.
>>
>> Testing: tier1-6.
>
> Alan Bateman 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:
>
> - ThreadSleepEvent refactoring
> - Merge
> - Merge
> - Initial sync from fibers branch
Changes requested by lmesnik (Reviewer).
src/java.base/share/classes/java/lang/System.java line 2566:
> 2564:
> 2565: public V executeOnCarrierThread(Callable task)
> throws Exception {
> 2566: if (Thread.currentThread() instanceof VirtualThread
> vthread) {
Any specific reason to don't use Thread.currentThread().isVirtual()?
test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java line 31:
> 29: * @summary Tests that JNI monitors work correctly with virtual threads
> 30: * @library /test/lib
> 31: * @compile JNIMonitor.java
I think that test file is compiled implicitly. So this line could be just
removed.
The same is for all other similar tests.
test/jdk/com/sun/jdi/TestScaffold.java line 980:
> 978:
> 979: if (wrapper.equals("Virtual")) {
> 980: threadFactory = Thread.ofVirtual().factory();
Should be line
469: argInfo.targetVMArgs += "--enable-preview ";
removed also?
test/jdk/com/sun/management/ThreadMXBean/VirtualThreads.java line 143:
> 141: long[] tids = new long[] { tid0, tid1 };
> 142: long[] cpuTimes = bean.getThreadCpuTime(tids);
> 143: if (Thread.currentThread().isVirtual()) {
How it worked before?
test/jdk/java/lang/Thread/virtual/GetStackTrace.java line 30:
> 28: * @requires vm.continuations
> 29: * @modules java.base/java.lang:+open
> 30: * @run testng/othervm -XX:+UnlockDiagnosticVMOptions
> -XX:+ShowHiddenFrames GetStackTrace
shouldn't be main/othervm ?
test/jdk/java/lang/Thread/virtual/VirtualThreadPinnedEventThrows.java line 29:
> 27: * @modules java.base/jdk.internal.event
> 28: * @compile/module=java.base
> jdk/internal/event/VirtualThreadPinnedEvent.java
> 29: * @run junit VirtualThreadPinnedEventThrows
Shouldn't be 'junit/othervm' used here to ensure that updated
VirtualThreadPinnedEvent is used? I don't know these details of jtreg.
test/jdk/jdk/incubator/concurrent/StructuredTaskScope/PreviewFeaturesNotEnabled.java
line 2:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
Not sure I understand what happens. The file is
test/jdk/jdk/incubator/concurrent/StructuredTaskScope/PreviewFeaturesNotEnabled.java
while it contains is public class VirtualThreadPinnedEvent. Copied by mistake?
-
PR Review: https://git.openjdk.org/jdk/pull/13203#pullrequestreview-1361847681
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151259675
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151175072
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151168150
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151168861
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151112603
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151153758
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151119165
Re: RFR: JDK-8304163: Move jdk.internal.module.ModuleInfoWriter to the test library [v2]
On Sat, 18 Mar 2023 19:14:09 GMT, Mandy Chung wrote: >> `ModuleInfoWriter` is not used by the runtime. Move it to the test library >> as `jdk.test.lib.util.ModuleInfoWriter`. The tests are updated to use the >> test library instead. `ModuleInfoWriter` depends on `jdk.internal.module` >> types and the Classfile API. Hence `@modules >> java.base/jdk.internal.classfile` and other classfile subpackages are added. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > move @library after @modules per the recommended ordering Changes requested by lmesnik (Reviewer). test/jdk/java/lang/ModuleTests/AnnotationsTest.java line 61: > 59: * java.base/jdk.internal.module > 60: * @library /test/lib > 61: * @build jdk.test.lib.util.ModuleInfoWriter You don't need to build library classes explicitly. I think @library /test/lib it enough. - PR: https://git.openjdk.org/jdk/pull/13085
Integrated: 8303697: ProcessTools doesn't print last line of process output
On Wed, 15 Mar 2023 05:41:33 GMT, Leonid Mesnik wrote: > The StreamPumper is fixed to process the last line even it is not finishes > with '\n' or '\r'. The test included. Testing with tier1-3 also to verify > that tests are not broken. This pull request has now been integrated. Changeset: 8d2ebf24 Author: Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/8d2ebf248e2884fbf138b603ae82f81bd0926cf3 Stats: 87 lines in 2 files changed: 82 ins; 0 del; 5 mod 8303697: ProcessTools doesn't print last line of process output Reviewed-by: dholmes, stuefe - PR: https://git.openjdk.org/jdk/pull/13034
Integrated: 8304314: StackWalkTest.java fails after CODETOOLS-7903373
On Thu, 16 Mar 2023 13:26:35 GMT, Leonid Mesnik wrote: > The test depends on the jtreg stack and should be updated. > The fix is backported from loom repo where jtreg with CODETOOLS-7903373 is > used. > Tested with current promoted jtreg (without CODETOOLS-7903373) by running > test. This pull request has now been integrated. Changeset: d5a15070 Author: Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/d5a150706e9070557533135489a73fc8cefc0cec Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8304314: StackWalkTest.java fails after CODETOOLS-7903373 Reviewed-by: alanb, mchung - PR: https://git.openjdk.org/jdk/pull/13058
Re: RFR: 8303697: ProcessTools doesn't print last line of process output [v5]
> The StreamPumper is fixed to process the last line even it is not finishes > with '\n' or '\r'. The test included. Testing with tier1-3 also to verify > that tests are not broken. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: typo fix - Changes: - all: https://git.openjdk.org/jdk/pull/13034/files - new: https://git.openjdk.org/jdk/pull/13034/files/47df1034..a1d7be0b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13034=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13034=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13034.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13034/head:pull/13034 PR: https://git.openjdk.org/jdk/pull/13034
Re: RFR: 8303697: ProcessTools doesn't print last line of process output [v3]
On Thu, 16 Mar 2023 06:19:14 GMT, Thomas Stuefe wrote: >> Leonid Mesnik has updated the pull request incrementally with one additional >> commit since the last revision: >> >> comments added > > That also affects OutputAnalyzer, right? Does this affect parsing or is this > only printing? > > Anyway, patch looks good to me, modulo of what David wrote. If you fix his > remarks, its good to me. @tstuefe Thank you for review. This issue is not related to OutputAnalyzer, I think. However it affects any line processing in ProcessTools.startProcess() and not only printing. - PR: https://git.openjdk.org/jdk/pull/13034
Re: RFR: 8303697: ProcessTools doesn't print last line of process output [v4]
> The StreamPumper is fixed to process the last line even it is not finishes > with '\n' or '\r'. The test included. Testing with tier1-3 also to verify > that tests are not broken. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fixed test and comments - Changes: - all: https://git.openjdk.org/jdk/pull/13034/files - new: https://git.openjdk.org/jdk/pull/13034/files/aa12782b..47df1034 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13034=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13034=02-03 Stats: 17 lines in 2 files changed: 11 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13034.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13034/head:pull/13034 PR: https://git.openjdk.org/jdk/pull/13034
Re: jtreg test test/jdk/java/lang/StackWalker/StackWalkTest.java fails after jtreg commit 7903373
Hi I’ve sent fix out for review: https://github.com/openjdk/jdk/pull/13058 Leonid From: Tobias Hartmann Date: Thursday, March 16, 2023 at 12:20 AM To: Kosta Stojiljkovic , core-libs-dev@openjdk.org Cc: Leonid Mesnik Subject: Re: jtreg test test/jdk/java/lang/StackWalker/StackWalkTest.java fails after jtreg commit 7903373 Hi Kosta, Thanks again for the report! This test is owned by core-libs/java.lang, I'm forwarding to core-libs-dev and CC'ing Leonid, the author of https://bugs.openjdk.org/browse/CODETOOLS-7903373. I can see these failures in our testing as well but no one filed a bug yet. I filed: https://bugs.openjdk.org/browse/JDK-8304314 Best regards, Tobias On 15.03.23 22:40, Kosta Stojiljkovic wrote: > Dear all, > > The test in ..test/jdk/java/lang/StackWalker/StackWalkTest.java fails with > the latest jtreg build, with the following error: > > ... > recursion chain > ... > ... > ... > at StackWalkTest$Test.call(StackWalkTest.java:223) >at StackWalkTest.runTest(StackWalkTest.java:270) >at StackWalkTest.main(StackWalkTest.java:325) >at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >at java.base/java.lang.reflect.Method.invoke(Method.java:578) >at > com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) >at java.base/java.lang.Thread.run(Thread.java:1623) > Caused by: java.lang.IndexOutOfBoundsException: Index: 1004, Size: 1004 >at > java.base/java.util.LinkedList.checkElementIndex(LinkedList.java:559) >at java.base/java.util.LinkedList.get(LinkedList.java:480) >at StackRecorderUtil.compareFrame(StackRecorderUtil.java:64) >at StackWalkTest.consume(StackWalkTest.java:145) >... 1018 more > > JavaTest Message: Test threw exception: java.lang.RuntimeException: extra > non-infra stack frame at count 1004: > > > --- > > In essence, the test detects an extra non-infra stack frame for the > MainWrapper$MainTask's frame. The test should disregard MainTask's stack > frame, since it's coming from an infrastructure class - > com.sun.javatest.regtest.agent.MainWrapper. The code correctly checks if the > stack frame belongs to the mentioned infrastructure class, but it also looks > for the inner class - com.sun.javatest.regtest.agent.MainWrapper$MainThread. > > I believe the problem comes from the following commit (7903373) to the jtreg > repository: > https://github.com/openjdk/jtreg/commit/5b9e661eb6ee9dd9a9d2690986bbf9ce303a8f03 > > This commit changed the name of the class MainThread to MainTask, thus making > the hardcoded check in the StackWalkTest fail to recognize this extra stack > frame as an infra frame. > > Could you please try to reproduce and let me know if I am missing or > misunderstanding something. > > Best, > Kosta Stojiljkovic
