<div dir="ltr">Hello Team,<div><br></div><div><br></div><div><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div>Provided Review Comments for both <b><u>openjdk/jcstress</u> PRs : #119 </b>and<b> #149</b></div><div><br></div><div><a href="https://github.com/openjdk/jcstress/pull/119" target="_blank">https://github.com/openjdk/jcstress/pull/119</a><br></div><div><br></div><div><a href="https://github.com/openjdk/jcstress/pull/149" target="_blank">https://github.com/openjdk/jcstress/pull/149</a><br></div></div><div><br></div><div><br></div><div><br></div><div>Approved <b><u>openjdk/jcstress</u> PR : #150</b><br></div><div><br></div><div><a href="https://github.com/openjdk/jcstress/pull/150">https://github.com/openjdk/jcstress/pull/150</a><br></div><div><br></div><div><br></div><div><br></div>Regards,<div><b>Prakhar Makhija</b></div><div><br></div><div>+91 78294 00970<br></div><div><a href="mailto:matcdac@gmail.com" target="_blank">matcdac@gmail.com</a></div><div><a href="https://www.linkedin.com/in/prakhar-makhija-17794b107/" target="_blank">https://www.linkedin.com/in/prakhar-makhija-17794b107/</a><br></div><div><br></div></div></div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 4, 2024 at 11:55 AM <<a href="mailto:jcstress-dev-request@openjdk.org" target="_blank">jcstress-dev-request@openjdk.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Send jcstress-dev mailing list submissions to<br>
<a href="mailto:jcstress-dev@openjdk.org" target="_blank">jcstress-dev@openjdk.org</a><br>
<br>
To subscribe or unsubscribe via the World Wide Web, visit<br>
<a href="https://mail.openjdk.org/mailman/listinfo/jcstress-dev" rel="noreferrer" target="_blank">https://mail.openjdk.org/mailman/listinfo/jcstress-dev</a><br>
or, via email, send a message with subject or body 'help' to<br>
<a href="mailto:jcstress-dev-request@openjdk.org" target="_blank">jcstress-dev-request@openjdk.org</a><br>
<br>
You can reach the person managing the list at<br>
<a href="mailto:jcstress-dev-owner@openjdk.org" target="_blank">jcstress-dev-owner@openjdk.org</a><br>
<br>
When replying, please edit your Subject line so it is more specific<br>
than "Re: Contents of jcstress-dev digest..."<br>
<br>
<br>
Today's Topics:<br>
<br>
1. Re: RFR: CODETOOLS-7903748 - jcstress: Test list should honor<br>
concurrency settings [v5] (Ji?? Van?k)<br>
2. Re: RFR: CODETOOLS-7903748 - jcstress: Test list should honor<br>
concurrency settings [v4] (Ji?? Van?k)<br>
3. Re: RFR: CODETOOLS-7903748 - jcstress: Test list should honor<br>
concurrency settings [v4] (Ji?? Van?k)<br>
4. Re: RFR: CODETOOLS-7903756 - jcstress shuld not pass<br>
debugging parameters to subproceses [v4] (Ji?? Van?k)<br>
<br>
<br>
----------------------------------------------------------------------<br>
<br>
Message: 1<br>
Date: Thu, 4 Jul 2024 05:49:57 GMT<br>
From: Ji?? Van?k <<a href="mailto:jvanek@openjdk.org" target="_blank">jvanek@openjdk.org</a>><br>
To: <<a href="mailto:jcstress-dev@openjdk.org" target="_blank">jcstress-dev@openjdk.org</a>><br>
Subject: Re: RFR: CODETOOLS-7903748 - jcstress: Test list should honor<br>
concurrency settings [v5]<br>
Message-ID:<br>
<wRLNZ4GbD_bgjolLGHRy7ehEb_TWfi725EVxWLV1vlM=.<a href="mailto:d6e1c38f-2687-4e1e-af67-500bc6271cb0@github.com" target="_blank">d6e1c38f-2687-4e1e-af67-500bc6271cb0@github.com</a>><br>
<br>
Content-Type: text/plain; charset=utf-8<br>
<br>
> This is extracting List<TestConfig> configs =prepareRunProgram(classes, tests);<br>
> with all he HW/switches setup to separated method and reusing it in `-l` mode<br>
> <br>
> I'm aware of triplicated removal of -agentlib, and will clean it up as<br>
> CODETOOLS-7903756 will progress.<br>
> <br>
> -l now honours also verbose mode, in which it prints not just matching<br>
> tests but all really run tests, and thus enabling much more easy<br>
> determining of all tests<br>
> <br>
> help adjusted.<br>
> <br>
> Maybe I'm missing plain quick initial all tests metod now, but with <br>
> artificial -c MAX it seems doing exactly that<br>
<br>
Ji?? Van?k has updated the pull request incrementally with one additional commit since the last revision:<br>
<br>
LIST_OPTION_DESCRIPTION split to two lines<br>
<br>
-------------<br>
<br>
Changes:<br>
- all: <a href="https://git.openjdk.org/jcstress/pull/149/files" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/149/files</a><br>
- new: <a href="https://git.openjdk.org/jcstress/pull/149/files/f4ac4222..713c5a2d" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/149/files/f4ac4222..713c5a2d</a><br>
<br>
Webrevs:<br>
- full: <a href="https://webrevs.openjdk.org/?repo=jcstress&pr=149&range=04" rel="noreferrer" target="_blank">https://webrevs.openjdk.org/?repo=jcstress&pr=149&range=04</a><br>
- incr: <a href="https://webrevs.openjdk.org/?repo=jcstress&pr=149&range=03-04" rel="noreferrer" target="_blank">https://webrevs.openjdk.org/?repo=jcstress&pr=149&range=03-04</a><br>
<br>
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod<br>
Patch: <a href="https://git.openjdk.org/jcstress/pull/149.diff" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/149.diff</a><br>
Fetch: git fetch <a href="https://git.openjdk.org/jcstress.git" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress.git</a> pull/149/head:pull/149<br>
<br>
PR: <a href="https://git.openjdk.org/jcstress/pull/149" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/149</a><br>
<br>
<br>
------------------------------<br>
<br>
Message: 2<br>
Date: Thu, 4 Jul 2024 05:49:57 GMT<br>
From: Ji?? Van?k <<a href="mailto:jvanek@openjdk.org" target="_blank">jvanek@openjdk.org</a>><br>
To: <<a href="mailto:jcstress-dev@openjdk.org" target="_blank">jcstress-dev@openjdk.org</a>><br>
Subject: Re: RFR: CODETOOLS-7903748 - jcstress: Test list should honor<br>
concurrency settings [v4]<br>
Message-ID:<br>
<c2X65bWtSWiCPXcigFgMCbNt9Dnl3JeOxDHzNBx5udY=.<a href="mailto:d49b7c4e-e620-4c6e-a72b-d31763be8260@github.com" target="_blank">d49b7c4e-e620-4c6e-a72b-d31763be8260@github.com</a>><br>
<br>
Content-Type: text/plain; charset=utf-8<br>
<br>
On Wed, 3 Jul 2024 19:30:06 GMT, PM <<a href="mailto:duke@openjdk.org" target="_blank">duke@openjdk.org</a>> wrote:<br>
<br>
>> Ji?? Van?k has updated the pull request incrementally with two additional commits since the last revision:<br>
>> <br>
>> - Moved listing of tests to separate method: listTests<br>
>> - -l description moved to constant<br>
><br>
> jcstress-core/src/main/java/org/openjdk/jcstress/Options.java line 50:<br>
> <br>
>> 48: public class Options {<br>
>> 49: <br>
>> 50: private static final String LIST_OPTION_DESCRIPTION="List the available tests matching the requested settings, after all filters (like CPU count) are applied. In verbose mode it prints all real combinations which will run.";<br>
> <br>
> feel free to split this in two lines if you prefer, you are right on the readability aspect, anyways the concatenation would happen just once at the application startup<br>
<br>
thanx! Split.<br>
<br>
-------------<br>
<br>
PR Review Comment: <a href="https://git.openjdk.org/jcstress/pull/149#discussion_r1665155175" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/149#discussion_r1665155175</a><br>
<br>
<br>
------------------------------<br>
<br>
Message: 3<br>
Date: Thu, 4 Jul 2024 06:08:29 GMT<br>
From: Ji?? Van?k <<a href="mailto:jvanek@openjdk.org" target="_blank">jvanek@openjdk.org</a>><br>
To: <<a href="mailto:jcstress-dev@openjdk.org" target="_blank">jcstress-dev@openjdk.org</a>><br>
Subject: Re: RFR: CODETOOLS-7903748 - jcstress: Test list should honor<br>
concurrency settings [v4]<br>
Message-ID:<br>
<skvJ7gQ-Vv8REzHDZnDsrAWxEojWX26E8ssWb6ya5XA=.<a href="mailto:2aa97ef4-5dcb-4d02-b7c6-55782fdc7cac@github.com" target="_blank">2aa97ef4-5dcb-4d02-b7c6-55782fdc7cac@github.com</a>><br>
<br>
Content-Type: text/plain; charset=utf-8<br>
<br>
On Wed, 3 Jul 2024 19:21:08 GMT, PM <<a href="mailto:duke@openjdk.org" target="_blank">duke@openjdk.org</a>> wrote:<br>
<br>
>> Ji?? Van?k has updated the pull request incrementally with two additional commits since the last revision:<br>
>> <br>
>> - Moved listing of tests to separate method: listTests<br>
>> - -l description moved to constant<br>
><br>
> jcstress-core/src/main/java/org/openjdk/jcstress/infra/runners/TestConfig.java line 264:<br>
> <br>
>> 262: ", strideCount: " + strideCount +<br>
>> 263: ", cpuMap: " + cpuMap +<br>
>> 264: ", " + jvmArgs + "}";<br>
> <br>
> kindly modify this to return a valid json, perhaps try running this method locally once in your machine, and copy paste that json in any online json formatter to check if you encounter any errors<br>
<br>
I never had an intention to return valid json. It is used only in verbose output, to differentiate individual variants of tests. Wihtout the test id, it do no have sense, and the extended info in {} is... complicated. See eg:<br>
<br>
org.openjdk.jcstress.samples.api.API_01_Simple {[actor1, actor2], spinLoopStyle: Thread.onSpinWait(), threads: 2, forkId: 0, maxFootprintMB: 64, compileMode: 0, shClass: (PG 0, CG 0), (PG 0, CG 0), strideSize: 256, strideCount: 40, cpuMap: null, [-XX:+UseBiasedLocking]}<br>
org.openjdk.jcstress.samples.api.API_01_Simple {[actor1, actor2], spinLoopStyle: Thread.onSpinWait(), threads: 2, forkId: 0, maxFootprintMB: 64, compileMode: 0, shClass: (PG 0, CG 0), (PG 0, CG 0), strideSize: 256, strideCount: 40, cpuMap: null, [-XX:-UseBiasedLocking]}<br>
<br>
<br>
<br>
To highlight it is not json, was the original usage of `=`, but I did not had super strong opinion.<br>
As the output should remain mainly human readable, I'm quite against full json (thus bracket before name, and id before actorNames and jvmArgs). But on contrary, the list is indeed huge.<br>
<br>
The main motivation is the line of ""All matching tests combinations - " + testsToPrint.size()" thus real number of run tests. The listing itself is (very) useful candy on top:<br>
<br>
java -jar tests-all/target/jcstress.jar -v -l -t org.openjdk.jcstress.samples.api.API_01_Simple <br>
...<br>
All matching tests combinations - 96<br>
...<br>
<br>
x<br>
<br>
java -jar tests-all/target/jcstress.jar -l -t org.openjdk.jcstress.samples.api.API_01_Simple <br>
...<br>
All matching tests - 1<br>
...<br>
<br>
<br>
<br>
So back to verbose listig output format - I intentionally kept it semi parseabel - grep/sed on output, and readable enough: name info in {} , all firelds with key id, and obvious arrays in [] without key.<br>
AFAICJ, the full json will lower the human readabiity. <br>
What about this - with "-v" it would print it as it is - human readable, fully distinguishible. on "-vv or -vvv" it would print complete fully qulified json. WDYT?<br>
<br>
-------------<br>
<br>
PR Review Comment: <a href="https://git.openjdk.org/jcstress/pull/149#discussion_r1665173411" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/149#discussion_r1665173411</a><br>
<br>
<br>
------------------------------<br>
<br>
Message: 4<br>
Date: Thu, 4 Jul 2024 06:25:05 GMT<br>
From: Ji?? Van?k <<a href="mailto:jvanek@openjdk.org" target="_blank">jvanek@openjdk.org</a>><br>
To: <<a href="mailto:jcstress-dev@openjdk.org" target="_blank">jcstress-dev@openjdk.org</a>><br>
Subject: Re: RFR: CODETOOLS-7903756 - jcstress shuld not pass<br>
debugging parameters to subproceses [v4]<br>
Message-ID:<br>
<MimRJngLQMOKSxr30nggGne8xyE0ONUdSZF9yTcdE1U=.<a href="mailto:73657180-4fd6-4683-8b0e-ed10d5028d3e@github.com" target="_blank">73657180-4fd6-4683-8b0e-ed10d5028d3e@github.com</a>><br>
<br>
Content-Type: text/plain; charset=utf-8<br>
<br>
> No longer passing any -agentlib to subprocesses, unless it is part of -jvmArgsPrepend, thus allowing to debug both jcstress itself or wrked vm as expected:<br>
> <br>
> java -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005 -jar tests-all/target/jcstress.jar -c 1 -jvmArgsPrepend "-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5006"<br>
> <br>
> works.<br>
<br>
Ji?? Van?k has updated the pull request incrementally with one additional commit since the last revision:<br>
<br>
Removed unused imports<br>
<br>
-------------<br>
<br>
Changes:<br>
- all: <a href="https://git.openjdk.org/jcstress/pull/150/files" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/150/files</a><br>
- new: <a href="https://git.openjdk.org/jcstress/pull/150/files/23291cb3..6ffc620b" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/150/files/23291cb3..6ffc620b</a><br>
<br>
Webrevs:<br>
- full: <a href="https://webrevs.openjdk.org/?repo=jcstress&pr=150&range=03" rel="noreferrer" target="_blank">https://webrevs.openjdk.org/?repo=jcstress&pr=150&range=03</a><br>
- incr: <a href="https://webrevs.openjdk.org/?repo=jcstress&pr=150&range=02-03" rel="noreferrer" target="_blank">https://webrevs.openjdk.org/?repo=jcstress&pr=150&range=02-03</a><br>
<br>
Stats: 3 lines in 2 files changed: 0 ins; 3 del; 0 mod<br>
Patch: <a href="https://git.openjdk.org/jcstress/pull/150.diff" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/150.diff</a><br>
Fetch: git fetch <a href="https://git.openjdk.org/jcstress.git" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress.git</a> pull/150/head:pull/150<br>
<br>
PR: <a href="https://git.openjdk.org/jcstress/pull/150" rel="noreferrer" target="_blank">https://git.openjdk.org/jcstress/pull/150</a><br>
<br>
<br>
End of jcstress-dev Digest, Vol 71, Issue 6<br>
*******************************************<br>
</blockquote></div>