Re: RFR: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
On Wed, 20 Oct 2021 18:34:36 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this tiny patch? > > from JBS: >> serviceability/jvmti/GetObjectSizeClass.java doesn't ignore external flags >> (where it matters) and hence should not have been marked w/ vm.flagless > > Thanks, > -- Igor Thanks Leonid! - PR: https://git.openjdk.java.net/jdk/pull/6050
Integrated: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
On Wed, 20 Oct 2021 18:34:36 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this tiny patch? > > from JBS: >> serviceability/jvmti/GetObjectSizeClass.java doesn't ignore external flags >> (where it matters) and hence should not have been marked w/ vm.flagless > > Thanks, > -- Igor This pull request has now been integrated. Changeset: cea3f010 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/cea3f010460c4b45e76bfac8a5b193c49fdd274a Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless Reviewed-by: lmesnik - PR: https://git.openjdk.java.net/jdk/pull/6050
RFR: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
Hi all, could you please review this tiny patch? from JBS: > serviceability/jvmti/GetObjectSizeClass.java doesn't ignore external flags > (where it matters) and hence should not have been marked w/ vm.flagless Thanks, -- Igor - Commit messages: - 8275666 Changes: https://git.openjdk.java.net/jdk/pull/6050/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6050=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275666 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6050.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6050/head:pull/6050 PR: https://git.openjdk.java.net/jdk/pull/6050
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v2]
On Tue, 17 Aug 2021 14:58:48 GMT, Mikhailo Seledtsov wrote: >> Please review this change that updates the buildJdkDockerImage() test >> library API. >> >> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l >> support for containers is not tested". >> The initial intent was to extend the buildJdkDockerImage() API of >> DockerTestUtils to accept custom Dockerfile content. >> As I analyzed the usage of buildJdkDockerImage() I realized that: >> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest" >> its use has been obsolete for some time, in favor of Dockerfile >> generated by DockerTestUtils >> - 3rd argument "buildDirName" is also always the same: "jdk-docker" >> >> Hence I thought it would be a good idea to simplify this API and make it >> up-to-date. >> >> Also, since the method signature is being updated, I thought it would be a >> good idea to also change the name to use more generic container terminology: >> buildJdkDockerImage() --> buildJdkContainerImage() > > Mikhailo Seledtsov has updated the pull request incrementally with one > additional commit since the last revision: > > Addressing review feedback Looks good and trivial to me. - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5134
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage()
On Mon, 16 Aug 2021 23:31:41 GMT, Mikhailo Seledtsov
wrote:
> Please review this change that updates the buildJdkDockerImage() test library
> API.
>
> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l
> support for containers is not tested".
> The initial intent was to extend the buildJdkDockerImage() API of
> DockerTestUtils to accept custom Dockerfile content.
> As I analyzed the usage of buildJdkDockerImage() I realized that:
> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
> its use has been obsolete for some time, in favor of Dockerfile
> generated by DockerTestUtils
> - 3rd argument "buildDirName" is also always the same: "jdk-docker"
>
> Hence I thought it would be a good idea to simplify this API and make it
> up-to-date.
>
> Also, since the method signature is being updated, I thought it would be a
> good idea to also change the name to use more generic container terminology:
> buildJdkDockerImage() --> buildJdkContainerImage()
Changes requested by iignatyev (Reviewer).
test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 145:
> 143:
> 144: // Create an image build/staging directory
> 145: Path buildDir = Paths.get(".", "image-build");
to make it more robust and possible to have more than one container within a
test, I'd use `imagName` + "image-build" as build dir name.
test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 179:
> 177: * @throws Exception
> 178: */
> 179: public static void buildImage(String imageName, Path buildDir)
> throws Exception {
it seems there are no users of this method, do we need to keep it public?
-
PR: https://git.openjdk.java.net/jdk/pull/5134
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage()
On Mon, 16 Aug 2021 23:47:04 GMT, Igor Ignatyev wrote:
>> Please review this change that updates the buildJdkDockerImage() test
>> library API.
>>
>> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l
>> support for containers is not tested".
>> The initial intent was to extend the buildJdkDockerImage() API of
>> DockerTestUtils to accept custom Dockerfile content.
>> As I analyzed the usage of buildJdkDockerImage() I realized that:
>> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>> its use has been obsolete for some time, in favor of Dockerfile
>> generated by DockerTestUtils
>> - 3rd argument "buildDirName" is also always the same: "jdk-docker"
>>
>> Hence I thought it would be a good idea to simplify this API and make it
>> up-to-date.
>>
>> Also, since the method signature is being updated, I thought it would be a
>> good idea to also change the name to use more generic container terminology:
>> buildJdkDockerImage() --> buildJdkContainerImage()
>
> test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 145:
>
>> 143:
>> 144: // Create an image build/staging directory
>> 145: Path buildDir = Paths.get(".", "image-build");
>
> to make it more robust and possible to have more than one container within a
> test, I'd use `imagName` + "image-build" as build dir name.
you also don't need to have `.` as a first argument, `Paths.get("x")` returns
you a path to "x" in current directory.
-
PR: https://git.openjdk.java.net/jdk/pull/5134
[jdk17] Integrated: 8067223: [TESTBUG] Rename Whitebox API package
On Wed, 28 Jul 2021 17:13:49 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor This pull request has now been integrated. Changeset: ada58d13 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/ada58d13f78eb8a240220c45c573335eeb47cf07 Stats: 532 lines in 36 files changed: 449 ins; 0 del; 83 mod 8067223: [TESTBUG] Rename Whitebox API package Reviewed-by: dholmes, kvn - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Mon, 2 Aug 2021 15:56:39 GMT, Vladimir Kozlov wrote: > I agree with these revised changes for JDK 17. Thanks for your review, Vladimir. I'll rerun my testing before integrating (just for good luck). -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains 12 new > commits since the last revision: > > - fixed ctw build > - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class > - updated requires.VMProps > - updated TEST.ROOT > - adjusted hotspot source > - added test > - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox) > - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class > - removed sun/hotspot/parser/DiagnosticCommand > - deprecated sun/hotspot classes >disallowed s.h.WhiteBox w/ security manager > - ... and 2 more: > https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860 Hi David, > This set of changes seems much more manageable to me. thank you for your review, David. > Not sure about the new deprecation warnings for the old WB classes .. might > that not itself trigger some failures? If not then I don't see how the > deprecation warnings help with transitioning to the new WB class? the deprecation warnings (hopefully) will help people not to forget that they should use the new WB class in new tests. Thanks, -- Igor Hi Jie, > Shall we also update the copyright year like > test/lib/sun/hotspot/cpuinfo/CPUInfo.java? we most certainly shall, just pushed the commit that updates the copyright years in the touched files. Cheers, -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v3]
> Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor Igor Ignatyev has updated the pull request incrementally with two additional commits since the last revision: - copyright update - fixed typo in ClassFileInstaller - Changes: - all: https://git.openjdk.java.net/jdk17/pull/290/files - new: https://git.openjdk.java.net/jdk17/pull/290/files/237e8860..fcaf41f8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=290=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=290=01-02 Stats: 10 lines in 10 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk17/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Vladimir, David, I've (forced) pushed a smaller version of the renaming. instead of removing `sun.hotspot` classes, it copies them to `jdk.test.whitebox` (w/ `s.h.parser.DiagnosticCommand` being removed as it's used in WhiteBox signature and it was easier to update a few tests that use it), updates hotspot code to register native methods for both `sun.hotspot.WhiteBox` and `jdk.test.whitebox.WhiteBox` classes. To make it easier and not to introduce extra dependency, I've made it impossible to use `s.h.WB` w/ a security manager enabled, otherwise there would be a dependency b/w `s.h.WB` and `j.t.w.WB$WhiteBoxPermission` or there would be 2 permissions. There are no open JDK tests that are impacted by this limitation. With minor tweaks in closed source, the patch successfully passes Oracle's tier1-4. -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
> Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor Igor Ignatyev has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 12 new commits since the last revision: - fixed ctw build - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class - updated requires.VMProps - updated TEST.ROOT - adjusted hotspot source - added test - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox) - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class - removed sun/hotspot/parser/DiagnosticCommand - deprecated sun/hotspot classes disallowed s.h.WhiteBox w/ security manager - ... and 2 more: https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860 - Changes: - all: https://git.openjdk.java.net/jdk17/pull/290/files - new: https://git.openjdk.java.net/jdk17/pull/290/files/8f12f2cf..237e8860 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=290=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=290=00-01 Stats: 3248 lines in 939 files changed: 969 ins; 0 del; 2279 mod Patch: https://git.openjdk.java.net/jdk17/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package
On Thu, 29 Jul 2021 01:30:37 GMT, Vladimir Kozlov wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > I know that tests fixes could be pushed during RDP2 without approval. > But these one is very big and it is not a fix - it is enhancement. > > What is the reason you want to push it into JDK 17 just few days before first > Release Candidate? Instead of pushing it into JDK 18. > > And I can't even review it because GutHub UI hangs on these big changes. @vnkozlov, @dholmes-ora, Thank you for looking at this! I want this to be integrated into JDK 17 b/c some "external" libraries use (used to use) WhiteBox API, e.g. jcstress[[2]] used WhiteBox API to deoptimize compiled methods[[3]], and it would be easier for maintainers of such libraries to condition package name based on JDK major version. Also, given JDK 17 is an LTS, it would be beneficial for everyone not to have big differences in test bases b/w it and the mainline. according to JEP 3, test RFEs are allowed until the very end and don't require late enhancement approval: "Enhancements to tests and documentation during RDP 1 and RDP 2 do not require approval, as long as the relevant issues are identified with a noreg-self or noreg-doc label, as appropriate"[[1]]. So, process-wise, I don't see any issues w/ integrating this RFE, yet, I fully agree that due to its size, this patch can be disruptive and can cause massive failures, which is something we obviously don't want at the current stage of JDK 17. I like David's idea about phasing this clean-up, and, due to the reasons described above, I would like to integrate the first part, copying WhiteBox classes to the new package structure and associated changes w/o updating all the tests, into JDK 17 and update the tests on the mainline (w/ backporting into jdk17u). WDYT? Cheers, -- Igor [1]: https://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process [2]: https://github.com/openjdk/jcstress [3]: https://github.com/openjdk/jcstress/blob/df83b446f187ae0b0fa31fa54decb59db9e955da/jcstress-core/src/main/java/org/openjdk/jcstress/vm/WhiteBoxSupport.java - PR: https://git.openjdk.java.net/jdk17/pull/290
[jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package
Hi all, could you please review this big tedious and trivial(-ish) patch which moves `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? the majority of the patch is the following substitutions: - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` - `s/sun.hotspot.code/jdk.test.whitebox.code/g` - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` testing: tier1-4 Thanks, -- Igor - Commit messages: - copyright year - update TEST.ROOT - jdk: s/sun\.hotspot\.gc/jdk.test.whitebox.gc/g - jdk: s/sun\.hotspot\.code/jdk.test.whitebox.code/g - jdk: s/sun\.hotspot\.WhiteBox/jdk.test.whitebox.WhiteBox/g - hotspot: 's~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g' - hotspot: s/sun\.hotspot\.parser/jdk.test.whitebox.parser/g - hotspot: s/sun\.hotspot\.cpuinfo/jdk.test.whitebox.cpuinfo/g - hotspot: s/sun\.hotspot\.code/jdk.test.whitebox.code/g - hotspot: 's/sun\.hotspot\.gc/jdk.test.whitebox.gc/g' - ... and 9 more: https://git.openjdk.java.net/jdk17/compare/c8ae7e5b...8f12f2cf Changes: https://git.openjdk.java.net/jdk17/pull/290/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=290=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8067223 Stats: 2310 lines in 949 files changed: 0 ins; 0 del; 2310 mod Patch: https://git.openjdk.java.net/jdk17/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk17/pull/290
[jdk17] Integrated: 8271173: serviceability/jvmti/GetObjectSizeClass.java doesn't check exit code
On Fri, 23 Jul 2021 04:34:50 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review the patch that adds an exit-code check to
> `serviceability/jvmti/GetObjectSizeClass.java` test?
>
> from JBS:
>> `serviceability/jvmti/GetObjectSizeClass.java` test spawns a new JVM but
>> doesn't check its exit code which might lead to both type-I and type-II
>> errors
>
> testing: `serviceability/jvmti/GetObjectSizeClass.java` on
> `{linux,windows,macos}-x64`
>
> Thanks,
> -- Igor
This pull request has now been integrated.
Changeset: e90ed6cc
Author:Igor Ignatyev
URL:
https://git.openjdk.java.net/jdk17/commit/e90ed6cc38ab8f8a2c7c740da1cb38144622b4eb
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
8271173: serviceability/jvmti/GetObjectSizeClass.java doesn't check exit code
Reviewed-by: dholmes
-
PR: https://git.openjdk.java.net/jdk17/pull/277
Re: [jdk17] RFR: 8271173: serviceability/jvmti/GetObjectSizeClass.java doesn't check exit code
On Fri, 23 Jul 2021 04:34:50 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review the patch that adds an exit-code check to
> `serviceability/jvmti/GetObjectSizeClass.java` test?
>
> from JBS:
>> `serviceability/jvmti/GetObjectSizeClass.java` test spawns a new JVM but
>> doesn't check its exit code which might lead to both type-I and type-II
>> errors
>
> testing: `serviceability/jvmti/GetObjectSizeClass.java` on
> `{linux,windows,macos}-x64`
>
> Thanks,
> -- Igor
Thanks, David.
-
PR: https://git.openjdk.java.net/jdk17/pull/277
[jdk17] RFR: 8271173: serviceability/jvmti/GetObjectSizeClass.java doesn't check exit code
Hi all,
could you please review the patch that adds an exit-code check to
`serviceability/jvmti/GetObjectSizeClass.java` test?
from JBS:
> `serviceability/jvmti/GetObjectSizeClass.java` test spawns a new JVM but
> doesn't check its exit code which might lead to both type-I and type-II errors
testing: `serviceability/jvmti/GetObjectSizeClass.java` on
`{linux,windows,macos}-x64`
Thanks,
-- Igor
-
Commit messages:
- 8271173
Changes: https://git.openjdk.java.net/jdk17/pull/277/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=277=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8271173
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk17/pull/277.diff
Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/277/head:pull/277
PR: https://git.openjdk.java.net/jdk17/pull/277
[jdk17] Integrated: 8268564: mark hotspot serviceability/attach tests which ignore external VM flags
On Thu, 10 Jun 2021 18:23:45 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this one-liner that adds `@requires vm.flagless` to 2 > `serviceability/attach` tests that ignore external VM flags? > > Thanks, > -- Igor This pull request has now been integrated. Changeset: 21abcc4a Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/21abcc4a5a539644be93726436ed4454ad9aaf18 Stats: 4 lines in 2 files changed: 3 ins; 1 del; 0 mod 8268564: mark hotspot serviceability/attach tests which ignore external VM flags Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk17/pull/8
[jdk17] Integrated: 8268531: mark SDTProbesGNULinuxTest as ignoring external VM flags
On Thu, 10 Jun 2021 16:46:12 GMT, Igor Ignatyev wrote: > (recreating openjdk/jdk#4453 against jdk17) > > Hi all, > > could you please review this patch that adds `@requires vm.flagless` to > `SDTProbesGNULinuxTest` test as it ignores all external VM flags? > > Thanks, > -- Igor This pull request has now been integrated. Changeset: f83c6b8a Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/f83c6b8a6a92a37197a3b83ba093f26e820c4ac9 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8268531: mark SDTProbesGNULinuxTest as ignoring external VM flags Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk17/pull/1
[jdk17] Integrated: 8268541: mark hotspot serviceability/sa tests which ignore external VM flags
On Thu, 10 Jun 2021 18:08:45 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this one-liner that adds `@requires vm.flagless` to > `serviceability/sa/TestJhsdbJstackLineNumbers.java` as it ignores external VM > flags? > > Thanks, > -- Igor This pull request has now been integrated. Changeset: 8366c693 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/8366c6936eaad411082ec6a9e569da07c5f3f0cd Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8268541: mark hotspot serviceability/sa tests which ignore external VM flags Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk17/pull/6
[jdk17] Integrated: 8268563: mark hotspot serviceability/jvmti tests which ignore external VM flags
On Thu, 10 Jun 2021 18:19:46 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this one-liner that adds `@requires vm.flagless` to 6 > `serviceability/jvmti` tests that ignore external VM flags? > > Thanks, > -- Igor This pull request has now been integrated. Changeset: 5b198986 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/5b198986cef4fa0e77d02136212ecc01ec9b22c0 Stats: 8 lines in 6 files changed: 6 ins; 0 del; 2 mod 8268563: mark hotspot serviceability/jvmti tests which ignore external VM flags Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk17/pull/7
Re: [jdk17] RFR: 8268564: mark hotspot serviceability/attach tests which ignore external VM flags
On Thu, 10 Jun 2021 18:23:45 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this one-liner that adds `@requires vm.flagless` to 2 > `serviceability/attach` tests that ignore external VM flags? > > Thanks, > -- Igor Thank you, Serguei. - PR: https://git.openjdk.java.net/jdk17/pull/8
Re: [jdk17] RFR: 8268531: mark SDTProbesGNULinuxTest as ignoring external VM flags
On Thu, 10 Jun 2021 16:46:12 GMT, Igor Ignatyev wrote: > (recreating openjdk/jdk#4453 against jdk17) > > Hi all, > > could you please review this patch that adds `@requires vm.flagless` to > `SDTProbesGNULinuxTest` test as it ignores all external VM flags? > > Thanks, > -- Igor Serguei, thank you for the review. - PR: https://git.openjdk.java.net/jdk17/pull/1
Re: [jdk17] RFR: 8268541: mark hotspot serviceability/sa tests which ignore external VM flags
On Thu, 10 Jun 2021 18:08:45 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this one-liner that adds `@requires vm.flagless` to > `serviceability/sa/TestJhsdbJstackLineNumbers.java` as it ignores external VM > flags? > > Thanks, > -- Igor thank you, Serguei. - PR: https://git.openjdk.java.net/jdk17/pull/6
Re: [jdk17] RFR: 8268563: mark hotspot serviceability/jvmti tests which ignore external VM flags
On Thu, 10 Jun 2021 18:19:46 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this one-liner that adds `@requires vm.flagless` to 6 > `serviceability/jvmti` tests that ignore external VM flags? > > Thanks, > -- Igor Serguei, thank you for your review. - PR: https://git.openjdk.java.net/jdk17/pull/7
Re: [jdk17] RFR: 8268564: mark hotspot serviceability/attach tests which ignore external VM flags
On Thu, 10 Jun 2021 21:27:09 GMT, Chris Plummer wrote: > Can you explain a bit more how `@requires vm.flagless` works? The way I read > it, that would indicate that the test should not be run if any flags are > specified. I'm not sure why you would want to do that. I understand that the > test itself launches a subprocess without passing on the flags, but I don't > see why that would preclude running the test if any flags are specified. Hi Chris, sure, you have read it correctly -- `@requires vm.flagless` excludes a test from execution if "any" flags are specified. there are two main reasons to do that: a) a test is very sensitive to the testing configuration and nearly any changes to the configuration render the test invalid; b) a test ignores all external flags, hence running it w/ any is a waste and misleading. in the case of this patch (as well as other sub-tasks of [8246500](https://bugs.openjdk.java.net/browse/JDK-8246500)), it's the latter. you can read more about `vm.flagless` and the motivation in the main-main [bug](https://bugs.openjdk.java.net/browse/JDK-8151707) and/or the original [discussion](https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-June/041981.html) on hotspot-dev. Cheers, -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/8
[jdk17] RFR: 8268564: mark hotspot serviceability/attach tests which ignore external VM flags
Hi all, could you please review this one-liner that adds `@requires vm.flagless` to 2 `serviceability/attach` tests that ignore external VM flags? Thanks, -- Igor - Commit messages: - 8268564 Changes: https://git.openjdk.java.net/jdk17/pull/8/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=8=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268564 Stats: 4 lines in 2 files changed: 3 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk17/pull/8.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/8/head:pull/8 PR: https://git.openjdk.java.net/jdk17/pull/8
[jdk17] RFR: 8268563: mark hotspot serviceability/jvmti tests which ignore external VM flags
Hi all, could you please review this one-liner that adds `@requires vm.flagless` to 6 `serviceability/jvmti` tests that ignore external VM flags? Thanks, -- Igor - Commit messages: - update copyright year - 8268563 Changes: https://git.openjdk.java.net/jdk17/pull/7/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=7=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268563 Stats: 8 lines in 6 files changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk17/pull/7.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/7/head:pull/7 PR: https://git.openjdk.java.net/jdk17/pull/7
[jdk17] RFR: 8268541: mark hotspot serviceability/sa tests which ignore external VM flags
Hi all, could you please review this one-liner that adds `@requires vm.flagless` to `serviceability/sa/TestJhsdbJstackLineNumbers.java` as it ignores external VM flags? Thanks, -- Igor - Commit messages: - 8268541 Changes: https://git.openjdk.java.net/jdk17/pull/6/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=6=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268541 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk17/pull/6.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/6/head:pull/6 PR: https://git.openjdk.java.net/jdk17/pull/6
[jdk17] Integrated: 8268539: several serviceability/sa tests should be run in driver mode
On Thu, 10 Jun 2021 17:11:15 GMT, Igor Ignatyev wrote: > 8268539: several serviceability/sa tests should be run in driver mode This pull request has now been integrated. Changeset: 5b8c51f5 Author: Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/5b8c51f59a5f23930ee43bea30201c1ff88c44cd Stats: 40 lines in 23 files changed: 0 ins; 0 del; 40 mod 8268539: several serviceability/sa tests should be run in driver mode Backport-of: 78cb6776b6d43b67457993a109719b36ee892d60 - PR: https://git.openjdk.java.net/jdk17/pull/4
[jdk17] Integrated: 8268539: several serviceability/sa tests should be run in driver mode
8268539: several serviceability/sa tests should be run in driver mode - Commit messages: - 8268539 Changes: https://git.openjdk.java.net/jdk17/pull/4/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=4=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268539 Stats: 40 lines in 23 files changed: 0 ins; 0 del; 40 mod Patch: https://git.openjdk.java.net/jdk17/pull/4.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/4/head:pull/4 PR: https://git.openjdk.java.net/jdk17/pull/4
Integrated: 8268539: several serviceability/sa tests should be run in driver mode
On Thu, 10 Jun 2021 10:07:48 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this _test-only_ patch that switches the execution > mode of several `serviceability/sa` tests to `driver`? all these tests seem > not to perform actual testing, and just organize other JDK executions and > validates their outputs. > > `ClhsdbDumpclass.java` had to be updated to have access to > `./jdk/test/lib/apps/LingeredApp.class` file. Although this also can be fixed > by propagating the classpath setting to `javap`, using the path to `.class` > file is more clear and relable, and I actually suspect that before `javap` > read not the dumped `.class` file, but the one from the test's classpath. > > Thanks, > -- Igor This pull request has now been integrated. Changeset: 78cb6776 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/78cb6776b6d43b67457993a109719b36ee892d60 Stats: 40 lines in 23 files changed: 0 ins; 0 del; 40 mod 8268539: several serviceability/sa tests should be run in driver mode Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/4460
Re: RFR: 8268539: several serviceability/sa tests should be run in driver mode
On Thu, 10 Jun 2021 10:07:48 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this _test-only_ patch that switches the execution > mode of several `serviceability/sa` tests to `driver`? all these tests seem > not to perform actual testing, and just organize other JDK executions and > validates their outputs. > > `ClhsdbDumpclass.java` had to be updated to have access to > `./jdk/test/lib/apps/LingeredApp.class` file. Although this also can be fixed > by propagating the classpath setting to `javap`, using the path to `.class` > file is more clear and relable, and I actually suspect that before `javap` > read not the dumped `.class` file, but the one from the test's classpath. > > Thanks, > -- Igor Thank you, Serguie. - PR: https://git.openjdk.java.net/jdk/pull/4460
[jdk17] RFR: 8268531: mark SDTProbesGNULinuxTest as ignoring external VM flags
(recreating openjdk/jdk#4453 against jdk17) Hi all, could you please review this patch that adds `@requires vm.flagless` to `SDTProbesGNULinuxTest` test as it ignores all external VM flags? Thanks, -- Igor - Commit messages: - update copyright year - 8268531 Changes: https://git.openjdk.java.net/jdk17/pull/1/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=1=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268531 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk17/pull/1.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/1/head:pull/1 PR: https://git.openjdk.java.net/jdk17/pull/1
Re: RFR: 8268531: mark SDTProbesGNULinuxTest as ignoring external VM flags
On Thu, 10 Jun 2021 07:24:26 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this patch that adds `@requires vm.flagless` to > `SDTProbesGNULinuxTest` test as it ignores all external VM flags? > > Thanks, > -- Igor closed in favor of openjdk/jdk17#1 - PR: https://git.openjdk.java.net/jdk/pull/4453
Integrated: 8268542: serviceability/logging/TestFullNames.java tests only 1st test case
On Thu, 10 Jun 2021 10:26:46 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small fix for
> `serviceability/logging/TestFullNames.java` test?
> from JBS:
>> serviceability/logging/TestFullNames.java test contains two test cases:
>> specifying filename w/ and w/o "file=" prefix. yet, the test doesn't remove
>> the file, hence its 2nd test case doesn't really test much.
>
> the patch simply calls `File::delete` inside the loop and verifies that the
> file doesn't exist at the beginning of each iteration.
>
> testing: `serviceability/logging/TestFullNames.java` on
> `{linux,windows,macosx}-x64`
>
> Thanks,
> -- Igor
This pull request has now been integrated.
Changeset: 74007890
Author:Igor Ignatyev
URL:
https://git.openjdk.java.net/jdk/commit/74007890bb9a3fa3a65683a3f480e399f2b1a0b6
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
8268542: serviceability/logging/TestFullNames.java tests only 1st test case
Reviewed-by: sspitsyn
-
PR: https://git.openjdk.java.net/jdk/pull/4461
Re: RFR: 8268542: serviceability/logging/TestFullNames.java tests only 1st test case
On Thu, 10 Jun 2021 10:26:46 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small fix for
> `serviceability/logging/TestFullNames.java` test?
> from JBS:
>> serviceability/logging/TestFullNames.java test contains two test cases:
>> specifying filename w/ and w/o "file=" prefix. yet, the test doesn't remove
>> the file, hence its 2nd test case doesn't really test much.
>
> the patch simply calls `File::delete` inside the loop and verifies that the
> file doesn't exist at the beginning of each iteration.
>
> testing: `serviceability/logging/TestFullNames.java` on
> `{linux,windows,macosx}-x64`
>
> Thanks,
> -- Igor
Thanks, Serguei!
-
PR: https://git.openjdk.java.net/jdk/pull/4461
RFR: 8268542: serviceability/logging/TestFullNames.java tests only 1st test case
Hi all,
could you please review this small fix for
`serviceability/logging/TestFullNames.java` test?
from JBS:
> serviceability/logging/TestFullNames.java test contains two test cases:
> specifying filename w/ and w/o "file=" prefix. yet, the test doesn't remove
> the file, hence its 2nd test case doesn't really test much.
the patch simply calls `File::delete` inside the loop and verifies that the
file doesn't exist at the beginning of each iteration.
testing: `serviceability/logging/TestFullNames.java` on
`{linux,windows,macosx}-x64`
Thanks,
-- Igor
-
Commit messages:
- 8268542
Changes: https://git.openjdk.java.net/jdk/pull/4461/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4461=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8268542
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4461.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4461/head:pull/4461
PR: https://git.openjdk.java.net/jdk/pull/4461
RFR: 8268539: several serviceability/sa tests should be run in driver mode
Hi all, could you please review this _test-only_ patch that switches the execution mode of several `serviceability/sa` tests to `driver`? all these tests seem not to perform actual testing, and just organize other JDK executions and validates their outputs. `ClhsdbDumpclass.java` had to be updated to have access to `./jdk/test/lib/apps/LingeredApp.class` file. Although this also can be fixed by propagating the classpath setting to `javap`, using the path to `.class` file is more clear and relable, and I actually suspect that before `javap` read not the dumped `.class` file, but the one from the test's classpath. Thanks, -- Igor - Commit messages: - use path to .class file in ClhsdbDumpclass - update copyright year - 8268539 Changes: https://git.openjdk.java.net/jdk/pull/4460/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4460=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268539 Stats: 40 lines in 23 files changed: 0 ins; 0 del; 40 mod Patch: https://git.openjdk.java.net/jdk/pull/4460.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4460/head:pull/4460 PR: https://git.openjdk.java.net/jdk/pull/4460
Re: RFR: 8268534: some serviceability/jvmti tests should be run in driver mode
On Thu, 10 Jun 2021 08:14:16 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small patch that switches execution mode in 4
> `serviceability/jvmti` tests?
> in all instances, the main test classes seem not to do actual testing and
> just orchestrate execution.
>
> testing: `serviceability/jvmti` on `{linux,windows,macosx}-x64`
>
> Thanks,
> -- Igor
Thank you, Serguei!
-
PR: https://git.openjdk.java.net/jdk/pull/4454
Re: RFR: 8268532: several serviceability/attach tests should be run in driver mode
On Thu, 10 Jun 2021 08:15:49 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small patch that switches execution mode in 4
> `serviceability/attach` tests?
> in all instances, the main test classes seem not to do actual testing and
> just orchestrate execution.
>
> testing: `serviceability/attach` on `{linux,windows,macosx}-x64`
>
> Thanks,
> -- Igor
Thanks, Serguei.
-
PR: https://git.openjdk.java.net/jdk/pull/4455
Integrated: 8268532: several serviceability/attach tests should be run in driver mode
On Thu, 10 Jun 2021 08:15:49 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small patch that switches execution mode in 4
> `serviceability/attach` tests?
> in all instances, the main test classes seem not to do actual testing and
> just orchestrate execution.
>
> testing: `serviceability/attach` on `{linux,windows,macosx}-x64`
>
> Thanks,
> -- Igor
This pull request has now been integrated.
Changeset: 92f0b6d4
Author:Igor Ignatyev
URL:
https://git.openjdk.java.net/jdk/commit/92f0b6d49e5a4dbdd6c95b8d526187adb33aa827
Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod
8268532: several serviceability/attach tests should be run in driver mode
Reviewed-by: sspitsyn
-
PR: https://git.openjdk.java.net/jdk/pull/4455
Integrated: 8268538: mark hotspot serviceability/logging tests which ignore external VM flags
On Thu, 10 Jun 2021 08:41:41 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review the patch which adds `@requires vm.flagless` to > serviceability/logging` tests that ignore external VM flags? > > Thanks, > -- Igor This pull request has now been integrated. Changeset: 964118f7 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/964118f7fd49786cfe60e1144800a02afc0fdb56 Stats: 12 lines in 5 files changed: 6 ins; 1 del; 5 mod 8268538: mark hotspot serviceability/logging tests which ignore external VM flags Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/4458
Re: RFR: 8268538: mark hotspot serviceability/logging tests which ignore external VM flags
On Thu, 10 Jun 2021 08:41:41 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review the patch which adds `@requires vm.flagless` to > serviceability/logging` tests that ignore external VM flags? > > Thanks, > -- Igor Thank you for your reviews, Serguei! - PR: https://git.openjdk.java.net/jdk/pull/4458
Re: RFR: 8268536: mark hotspot serviceability/dcmd tests which ignore external VM flags
On Thu, 10 Jun 2021 08:28:25 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review the patch which adds `@requires vm.flagless` to > `serviceability/dcmd/gc/RunFinalizationTest.java` and > `serviceability/dcmd/compiler/CodeHeapAnalyticsParams.java` tests as they > completely ignore external vm flags? > > Thanks, > -- Igor Thank you, Serguei. - PR: https://git.openjdk.java.net/jdk/pull/4457
Integrated: 8268536: mark hotspot serviceability/dcmd tests which ignore external VM flags
On Thu, 10 Jun 2021 08:28:25 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review the patch which adds `@requires vm.flagless` to > `serviceability/dcmd/gc/RunFinalizationTest.java` and > `serviceability/dcmd/compiler/CodeHeapAnalyticsParams.java` tests as they > completely ignore external vm flags? > > Thanks, > -- Igor This pull request has now been integrated. Changeset: 05090fc8 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/05090fc8fdef3c02c91cbd79e661f763893580c2 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod 8268536: mark hotspot serviceability/dcmd tests which ignore external VM flags Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/4457
Integrated: 8268530: resourcehogs/serviceability/jvmti/GetObjectSizeOverflow.java should be run in driver mode
On Thu, 10 Jun 2021 07:02:36 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small and test-only change for
> `resourcehogs/serviceability/jvmti/GetObjectSizeOverflow.java` test?
>
> the test's main class just orchestrates execution and hence should be run in
> a driver mode, the patch also makes small refactoring of the test code:
> removes redundant imports, simplifies process creation, and adds checks of
> the exit code.
>
> testing: `resourcehogs/serviceability` tests on `{linux,windows,macosx}-x64`
>
> Thanks,
> -- Igor
This pull request has now been integrated.
Changeset: ae29f9ca
Author:Igor Ignatyev
URL:
https://git.openjdk.java.net/jdk/commit/ae29f9cacdac8bfe7fc1d287edbfb21c81686d4c
Stats: 11 lines in 1 file changed: 1 ins; 4 del; 6 mod
8268530: resourcehogs/serviceability/jvmti/GetObjectSizeOverflow.java should be
run in driver mode
Reviewed-by: sspitsyn
-
PR: https://git.openjdk.java.net/jdk/pull/4452
Re: RFR: 8268530: resourcehogs/serviceability/jvmti/GetObjectSizeOverflow.java should be run in driver mode
On Thu, 10 Jun 2021 07:02:36 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small and test-only change for
> `resourcehogs/serviceability/jvmti/GetObjectSizeOverflow.java` test?
>
> the test's main class just orchestrates execution and hence should be run in
> a driver mode, the patch also makes small refactoring of the test code:
> removes redundant imports, simplifies process creation, and adds checks of
> the exit code.
>
> testing: `resourcehogs/serviceability` tests on `{linux,windows,macosx}-x64`
>
> Thanks,
> -- Igor
Thanks, Serguei!
-
PR: https://git.openjdk.java.net/jdk/pull/4452
RFR: 8268534: some serviceability/jvmti tests should be run in driver mode
Hi all,
could you please review this small patch that switches execution mode in 4
`serviceability/jvmti` tests?
in all instances, the main test classes seem not to do actual testing and just
orchestrate execution.
testing: `serviceability/jvmti` on `{linux,windows,macosx}-x64`
Thanks,
-- Igor
-
Commit messages:
- copyright
- 8268534
Changes: https://git.openjdk.java.net/jdk/pull/4454/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4454=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8268534
Stats: 7 lines in 4 files changed: 0 ins; 0 del; 7 mod
Patch: https://git.openjdk.java.net/jdk/pull/4454.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4454/head:pull/4454
PR: https://git.openjdk.java.net/jdk/pull/4454
RFR: 8268532: several serviceability/attach tests should be run in driver mode
Hi all,
could you please review this small patch that switches execution mode in 4
`serviceability/attach` tests?
in all instances, the main test classes seem not to do actual testing and just
orchestrate execution.
testing: `serviceability/attach` on `{linux,windows,macosx}-x64`
Thanks,
-- Igor
-
Commit messages:
- one more test
- copyright year
- 8268532
Changes: https://git.openjdk.java.net/jdk/pull/4455/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4455=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8268532
Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod
Patch: https://git.openjdk.java.net/jdk/pull/4455.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4455/head:pull/4455
PR: https://git.openjdk.java.net/jdk/pull/4455
RFR: 8268538: mark hotspot serviceability/logging tests which ignore external VM flags
Hi all, could you please review the patch which adds `@requires vm.flagless` to serviceability/logging` tests that ignore external VM flags? Thanks, -- Igor - Commit messages: - update copyright year - 8268538 Changes: https://git.openjdk.java.net/jdk/pull/4458/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4458=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268538 Stats: 12 lines in 5 files changed: 6 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/4458.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4458/head:pull/4458 PR: https://git.openjdk.java.net/jdk/pull/4458
RFR: 8268536: mark hotspot serviceability/dcmd tests which ignore external VM flags
Hi all, could you please review the patch which adds `@requires vm.flagless` to `serviceability/dcmd/gc/RunFinalizationTest.java` and `serviceability/dcmd/compiler/CodeHeapAnalyticsParams.java` tests as they completely ignore external vm flags? Thanks, -- Igor - Commit messages: - update copyright year - 8268536 Changes: https://git.openjdk.java.net/jdk/pull/4457/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4457=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268536 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4457.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4457/head:pull/4457 PR: https://git.openjdk.java.net/jdk/pull/4457
RFR: 8268530: resourcehogs/serviceability/jvmti/GetObjectSizeOverflow.java should be run in driver mode
Hi all,
could you please review this small and test-only change for
`resourcehogs/serviceability/jvmti/GetObjectSizeOverflow.java` test?
the test's main class just orchestrates execution and hence should be run in a
driver mode, the patch also makes small refactoring of the test code: removes
redundant imports, simplifies process creation, and adds checks of the exit
code.
testing: `resourcehogs/serviceability` tests on `{linux,windows,macosx}-x64`
Thanks,
-- Igor
-
Commit messages:
- check exit code of jar and GetObjectSizeOverflowAgent processes
- simply jar process creation
- cleaned up imports
- use try-with-resource
- 8268530
Changes: https://git.openjdk.java.net/jdk/pull/4452/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4452=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8268530
Stats: 11 lines in 1 file changed: 1 ins; 4 del; 6 mod
Patch: https://git.openjdk.java.net/jdk/pull/4452.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4452/head:pull/4452
PR: https://git.openjdk.java.net/jdk/pull/4452
RFR: 8268531: mark SDTProbesGNULinuxTest as ignoring external VM flags
Hi all, could you please review this patch that adds `@requires vm.flagless` to `SDTProbesGNULinuxTest` test as it ignores all external VM flags? Thanks, -- Igor - Commit messages: - update copyright year - 8268531 Changes: https://git.openjdk.java.net/jdk/pull/4453/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4453=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268531 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4453.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4453/head:pull/4453 PR: https://git.openjdk.java.net/jdk/pull/4453
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov wrote: >> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related >> to Ahead-of-Time Compiler from JDK: >> >> - `jdk.aot` module — the `jaotc` tool >> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution >> - related HotSpot code guarded by `#if INCLUDE_AOT` >> >> Additionally, remove tests as well as code in makefiles related to AoT >> compilation. >> >> Tested hs-tier1-4 > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Remove exports from Graal module to jdk.aot Test changes look good to me. - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8246494: introduce vm.flagless at-requires property
On Wed, 17 Mar 2021 07:02:59 GMT, Serguei Spitsyn wrote: >> resurrecting old >> [RFR](https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-June/041981.html): >> >>> Hi all, >>> >>> could you please review the patch which introduces a new @requires property >>> to filter out the tests which ignore externally provided JVM flags? >>> >>> the idea behind this patch is to have a way to clearly mark tests which >>> ignore flags, so >>> a) it's obvious that they don't execute a flag-guarded code/feature, and >>> extra care should be taken to use them to verify any flag-guarded changed; >>> b) they can be easily excluded from runs w/ flags. >>> >>> @requires and VMProps allows us to achieve both, so it's been decided to >>> add a new property `vm.flagless`. `vm.flagless` is set to false if there >>> are any XX flags other than `-XX:MaxRAMPercentage` and >>> `-XX:CreateCoredumpOnCrash` (which are known to be set almost always) or >>> any X flags other `-Xmixed`; in other words any tests w/ `@requires >>> vm.flagless` will be excluded from runs w/ any other X / XX flags passed >>> via `-vmoption` / `-javaoption`. in rare cases, when one still wants to run >>> the tests marked by `vm.flagless` w/ external flags, `vm.flagless` can be >>> forcefully set to true by setting any value to `TEST_VM_FLAGLESS` env. >>> variable. >>> >>> this patch adds necessary common changes and marks common tests, namely >>> Scimark, GTestWrapper and TestNativeProcessBuilder. Component-specific >>> tests will be marked separately by the corresponding subtasks of 8151707[1]. >>> >>> please note, the patch depends on CODETOOLS-7902336[2], which will be >>> included in the next jtreg version, so this patch is to be integrated only >>> after jtreg5.1 is promoted and we switch to use it by 8246387[3]. >>> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8246494 >>> webrev: http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00 >>> testing: marked tests w/ different XX and X flags w/ and w/o >>> TEST_VM_FLAGLESS env. var, and w/o any flags >>> >>> [1] https://bugs.openjdk.java.net/browse/JDK-8151707 >>> [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7902336 >>> [3] https://bugs.openjdk.java.net/browse/JDK-8246387 >>> >> >> after offline discussion with @pliden, it has been decided to reduce the >> scope of [8246499](https://bugs.openjdk.java.net/browse/JDK-8246499) and not >> mark the tests that use `UseXGC` flags for selection, e.g. >> `test/hotspot/jtreg/gc/z/TestSmallHeap.java`. >> >> Thanks, >> -- Igor > > Igor, > The fix looks good to me. > Thanks, > Serguei Thanks, Serguei! - PR: https://git.openjdk.java.net/jdk/pull/2800
Integrated: 8246494: introduce vm.flagless at-requires property
On Tue, 2 Mar 2021 23:27:21 GMT, Igor Ignatyev wrote: > resurrecting old > [RFR](https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-June/041981.html): > >> Hi all, >> >> could you please review the patch which introduces a new @requires property >> to filter out the tests which ignore externally provided JVM flags? >> >> the idea behind this patch is to have a way to clearly mark tests which >> ignore flags, so >> a) it's obvious that they don't execute a flag-guarded code/feature, and >> extra care should be taken to use them to verify any flag-guarded changed; >> b) they can be easily excluded from runs w/ flags. >> >> @requires and VMProps allows us to achieve both, so it's been decided to add >> a new property `vm.flagless`. `vm.flagless` is set to false if there are any >> XX flags other than `-XX:MaxRAMPercentage` and `-XX:CreateCoredumpOnCrash` >> (which are known to be set almost always) or any X flags other `-Xmixed`; in >> other words any tests w/ `@requires vm.flagless` will be excluded from runs >> w/ any other X / XX flags passed via `-vmoption` / `-javaoption`. in rare >> cases, when one still wants to run the tests marked by `vm.flagless` w/ >> external flags, `vm.flagless` can be forcefully set to true by setting any >> value to `TEST_VM_FLAGLESS` env. variable. >> >> this patch adds necessary common changes and marks common tests, namely >> Scimark, GTestWrapper and TestNativeProcessBuilder. Component-specific tests >> will be marked separately by the corresponding subtasks of 8151707[1]. >> >> please note, the patch depends on CODETOOLS-7902336[2], which will be >> included in the next jtreg version, so this patch is to be integrated only >> after jtreg5.1 is promoted and we switch to use it by 8246387[3]. >> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8246494 >> webrev: http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00 >> testing: marked tests w/ different XX and X flags w/ and w/o >> TEST_VM_FLAGLESS env. var, and w/o any flags >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8151707 >> [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7902336 >> [3] https://bugs.openjdk.java.net/browse/JDK-8246387 >> > > after offline discussion with @pliden, it has been decided to reduce the > scope of [8246499](https://bugs.openjdk.java.net/browse/JDK-8246499) and not > mark the tests that use `UseXGC` flags for selection, e.g. > `test/hotspot/jtreg/gc/z/TestSmallHeap.java`. > > Thanks, > -- Igor This pull request has now been integrated. Changeset: e333b6e1 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/e333b6e1 Stats: 81 lines in 6 files changed: 75 ins; 0 del; 6 mod 8246494: introduce vm.flagless at-requires property Reviewed-by: mseledtsov, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/2800
Re: RFR: 8246494: introduce vm.flagless at-requires property
On Wed, 17 Mar 2021 00:45:00 GMT, Mikhailo Seledtsov wrote: >> resurrecting old >> [RFR](https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-June/041981.html): >> >>> Hi all, >>> >>> could you please review the patch which introduces a new @requires property >>> to filter out the tests which ignore externally provided JVM flags? >>> >>> the idea behind this patch is to have a way to clearly mark tests which >>> ignore flags, so >>> a) it's obvious that they don't execute a flag-guarded code/feature, and >>> extra care should be taken to use them to verify any flag-guarded changed; >>> b) they can be easily excluded from runs w/ flags. >>> >>> @requires and VMProps allows us to achieve both, so it's been decided to >>> add a new property `vm.flagless`. `vm.flagless` is set to false if there >>> are any XX flags other than `-XX:MaxRAMPercentage` and >>> `-XX:CreateCoredumpOnCrash` (which are known to be set almost always) or >>> any X flags other `-Xmixed`; in other words any tests w/ `@requires >>> vm.flagless` will be excluded from runs w/ any other X / XX flags passed >>> via `-vmoption` / `-javaoption`. in rare cases, when one still wants to run >>> the tests marked by `vm.flagless` w/ external flags, `vm.flagless` can be >>> forcefully set to true by setting any value to `TEST_VM_FLAGLESS` env. >>> variable. >>> >>> this patch adds necessary common changes and marks common tests, namely >>> Scimark, GTestWrapper and TestNativeProcessBuilder. Component-specific >>> tests will be marked separately by the corresponding subtasks of 8151707[1]. >>> >>> please note, the patch depends on CODETOOLS-7902336[2], which will be >>> included in the next jtreg version, so this patch is to be integrated only >>> after jtreg5.1 is promoted and we switch to use it by 8246387[3]. >>> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8246494 >>> webrev: http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00 >>> testing: marked tests w/ different XX and X flags w/ and w/o >>> TEST_VM_FLAGLESS env. var, and w/o any flags >>> >>> [1] https://bugs.openjdk.java.net/browse/JDK-8151707 >>> [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7902336 >>> [3] https://bugs.openjdk.java.net/browse/JDK-8246387 >>> >> >> after offline discussion with @pliden, it has been decided to reduce the >> scope of [8246499](https://bugs.openjdk.java.net/browse/JDK-8246499) and not >> mark the tests that use `UseXGC` flags for selection, e.g. >> `test/hotspot/jtreg/gc/z/TestSmallHeap.java`. >> >> Thanks, >> -- Igor > > These changes look good to me. Thanks, Misha! -- Igor - PR: https://git.openjdk.java.net/jdk/pull/2800
Integrated: 8263549: 8263412 can cause jtreg testlibrary split
On Sat, 13 Mar 2021 04:31:31 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor This pull request has now been integrated. Changeset: a7aba2b6 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/a7aba2b6 Stats: 1738 lines in 867 files changed: 2 ins; 67 del; 1669 mod 8263549: 8263412 can cause jtreg testlibrary split Reviewed-by: iklam, dcubed - PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v3]
On Sat, 13 Mar 2021 14:20:20 GMT, Daniel D. Daugherty wrote: >> Igor Ignatyev has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > I downloaded the patch and used Ioi's cmd pattern to scroll through > the changes. I can't honestly say that I looked at every line since 867 > changed files would overwhelm anyone's brain... > > I did notice a couple of `@run main` instead of `@run driver` calls > to the ClassFileInstaller, but those are pre-existing. > > Thumbs up. Hi Dan, Thanks for your review! > I did notice a couple of @run main instead of @run driver calls to the > ClassFileInstaller, but those are pre-existing. I noticed this too, planning to fix that with a separate RFE. -- Igor - PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v3]
> Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor Igor Ignatyev has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: fix compilation error in IncorrectAOTLibraryTest test - Changes: - all: https://git.openjdk.java.net/jdk/pull/2985/files - new: https://git.openjdk.java.net/jdk/pull/2985/files/ff6d4f91..3a3b7a84 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2985=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2985=01-02 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2985.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2985/head:pull/2985 PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v3]
On Sat, 13 Mar 2021 06:16:37 GMT, Ioi Lam wrote: >> Igor Ignatyev has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. The pull request contains one >> new commit since the last revision: >> >> fix compilation error in IncorrectAOTLibraryTest test > > I did this and scanned the differences (with the diff file from the webrev) > and it looks reasonable to me. > > grep '^[+-]' diff.txt | grep -v Copyright | grep -v '^.[+-]' | less > > It looks like most of the changes are mechanical. There were only a few cases > where manual changes were made. I trusted that you have tested those cases > individually. > > But I don't understand why this error can happen. It seems like jtreg would > allow two test cases to interfere with each other. Hi Ioi, thanks for review this, I ran the whole tier1-3 jobs which should provide enough coverage. as oracle builds don't have AOT feature enabled, I missed a compilation error in `IncorrectAOTLibraryTest` test. the test failed in GitHub action and should be fixed by [3a3b7a8](https://github.com/openjdk/jdk/pull/2985/commits/3a3b7a846289181b466b3c1eb478a0a571d9468b). -- Igor - PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v2]
> Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor Igor Ignatyev has updated the pull request incrementally with one additional commit since the last revision: fix compilation error in IncorrectAOTLibraryTest test - Changes: - all: https://git.openjdk.java.net/jdk/pull/2985/files - new: https://git.openjdk.java.net/jdk/pull/2985/files/6e53ad97..ff6d4f91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2985=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2985=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2985.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2985/head:pull/2985 PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split
On Sat, 13 Mar 2021 04:31:31 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor note for reviewers: the big part of the patch is just `sed -i 's/ ClassFileInstaller / jdk.test.lib.helpers.ClassFileInstaller /g'` - PR: https://git.openjdk.java.net/jdk/pull/2985
RFR: 8263549: 8263412 can cause jtreg testlibrary split
Hi all, could you please review this dull patch that replaces `ClassFileInstaller` w/ `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to ensure we won't get split testlibrary, and removes `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). from JBS: > after JDK-8263412, we might (again) encounter NCDFE b/c parts of > testlibraries aren't on the classpath. this happens when jtreg builds > `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, > but `ClassFileInstaller` as part of shared testibrary directory in one test, > when in the following test, jtreg sees `ClassFileInstaller` in the shared > directory, hence javac won't recompile it/its dependencies, but in runtime > `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we > get NCDFE. testing: - [x] `grep ' ClassFileInstaller[^.]` - [ ] tier1-3 Thanks, -- Igor - Commit messages: - fixup - update copyright year - rm test/lib/ClassFileInstaller.java - 's/ ClassFileInstaller / jdk.test.lib.helpers.ClassFileInstaller /g' Changes: https://git.openjdk.java.net/jdk/pull/2985/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2985=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263549 Stats: 1736 lines in 867 files changed: 0 ins; 67 del; 1669 mod Patch: https://git.openjdk.java.net/jdk/pull/2985.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2985/head:pull/2985 PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8246494: introduce vm.flagless at-requires property
On Tue, 2 Mar 2021 23:27:21 GMT, Igor Ignatyev wrote: > resurrecting old > [RFR](https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-June/041981.html): > >> Hi all, >> >> could you please review the patch which introduces a new @requires property >> to filter out the tests which ignore externally provided JVM flags? >> >> the idea behind this patch is to have a way to clearly mark tests which >> ignore flags, so >> a) it's obvious that they don't execute a flag-guarded code/feature, and >> extra care should be taken to use them to verify any flag-guarded changed; >> b) they can be easily excluded from runs w/ flags. >> >> @requires and VMProps allows us to achieve both, so it's been decided to add >> a new property `vm.flagless`. `vm.flagless` is set to false if there are any >> XX flags other than `-XX:MaxRAMPercentage` and `-XX:CreateCoredumpOnCrash` >> (which are known to be set almost always) or any X flags other `-Xmixed`; in >> other words any tests w/ `@requires vm.flagless` will be excluded from runs >> w/ any other X / XX flags passed via `-vmoption` / `-javaoption`. in rare >> cases, when one still wants to run the tests marked by `vm.flagless` w/ >> external flags, `vm.flagless` can be forcefully set to true by setting any >> value to `TEST_VM_FLAGLESS` env. variable. >> >> this patch adds necessary common changes and marks common tests, namely >> Scimark, GTestWrapper and TestNativeProcessBuilder. Component-specific tests >> will be marked separately by the corresponding subtasks of 8151707[1]. >> >> please note, the patch depends on CODETOOLS-7902336[2], which will be >> included in the next jtreg version, so this patch is to be integrated only >> after jtreg5.1 is promoted and we switch to use it by 8246387[3]. >> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8246494 >> webrev: http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00 >> testing: marked tests w/ different XX and X flags w/ and w/o >> TEST_VM_FLAGLESS env. var, and w/o any flags >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8151707 >> [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7902336 >> [3] https://bugs.openjdk.java.net/browse/JDK-8246387 >> > > after offline discussion with @pliden, it has been decided to reduce the > scope of [8246499](https://bugs.openjdk.java.net/browse/JDK-8246499) and not > mark the tests that use `UseXGC` flags for selection, e.g. > `test/hotspot/jtreg/gc/z/TestSmallHeap.java`. > > Thanks, > -- Igor ping? - PR: https://git.openjdk.java.net/jdk/pull/2800
Re: RFR: 8263412: ClassFileInstaller can't be used by classes outside of default package
On Fri, 12 Mar 2021 02:08:09 GMT, Mikhailo Seledtsov
wrote:
>> Hi all,
>>
>> could you please review the patch which moves `ClassFileInstaller` class to
>> `jdk.test.lib.helpers` package?
>> to reduce changes in the tests, `ClassFileInstaller` in the default package
>> is kept w/ just `main` method that calls `jdk.test.lib.helpers.
>> ClassFileInstaller::main`.
>>
>> from JBS:
>>> ClassFileInstaller is in the default package hence it's impossible to use
>>> it directly by classes in any other package. although ClassFileInstaller is
>>> mainly used directly from jtreg test description, there are cases when one
>>> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet
>>> to do. that these driver classes have to either be in a default package or
>>> use reflection, both approaches have drawbacks.
>>>
>>> to solve that, we can move ClassFileInstaller implementation to a package,
>>> and to avoid unneeded churn, keep package-less ClassFileInstaller class
>>> which will call package-full impl.
>>>
>>> the need for this patch has raised as part of
>>> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8254129).
>>
>> testing:
>> - [x] `:jdk_core` against `{macos,windows,linux}-x64`
>> - [x] `:jdk_svc` against `{macos,windows,linux}-x64`
>> - [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories
>> against `{macos,windows,linux}-x64`
>>
>> Thanks,
>> -- Igor
>
> Marked as reviewed by mseledtsov (Committer).
Ioi, Coleen, Misha, thanks for your reviews.
-- Igor
-
PR: https://git.openjdk.java.net/jdk/pull/2932
Integrated: 8263412: ClassFileInstaller can't be used by classes outside of default package
On Thu, 11 Mar 2021 05:47:00 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review the patch which moves `ClassFileInstaller` class to
> `jdk.test.lib.helpers` package?
> to reduce changes in the tests, `ClassFileInstaller` in the default package
> is kept w/ just `main` method that calls `jdk.test.lib.helpers.
> ClassFileInstaller::main`.
>
> from JBS:
>> ClassFileInstaller is in the default package hence it's impossible to use it
>> directly by classes in any other package. although ClassFileInstaller is
>> mainly used directly from jtreg test description, there are cases when one
>> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet
>> to do. that these driver classes have to either be in a default package or
>> use reflection, both approaches have drawbacks.
>>
>> to solve that, we can move ClassFileInstaller implementation to a package,
>> and to avoid unneeded churn, keep package-less ClassFileInstaller class
>> which will call package-full impl.
>>
>> the need for this patch has raised as part of
>> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8254129).
>
> testing:
> - [x] `:jdk_core` against `{macos,windows,linux}-x64`
> - [x] `:jdk_svc` against `{macos,windows,linux}-x64`
> - [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories
> against `{macos,windows,linux}-x64`
>
> Thanks,
> -- Igor
This pull request has now been integrated.
Changeset: e834f99d
Author:Igor Ignatyev
URL: https://git.openjdk.java.net/jdk/commit/e834f99d
Stats: 472 lines in 114 files changed: 145 ins; 229 del; 98 mod
8263412: ClassFileInstaller can't be used by classes outside of default package
Reviewed-by: iklam, coleenp, mseledtsov
-
PR: https://git.openjdk.java.net/jdk/pull/2932
RFR: 8263412: ClassFileInstaller can't be used by classes outside of default package
Hi all,
could you please review the patch which moves `ClassFileInstaller` class to
`jdk.test.lib.helpers` package?
to reduce changes in the tests, `ClassFileInstaller` in the default package is
kept w/ just `main` method that calls `jdk.test.lib.helpers.
ClassFileInstaller::main`.
from JBS:
> ClassFileInstaller is in the default package hence it's impossible to use it
> directly by classes in any other package. although ClassFileInstaller is
> mainly used directly from jtreg test description, there are cases when one
> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet
> to do. that these driver classes have to either be in a default package or
> use reflection, both approaches have drawbacks.
>
> to solve that, we can move ClassFileInstaller implementation to a package,
> and to avoid unneeded churn, keep package-less ClassFileInstaller class which
> will call package-full impl.
>
> the need for this patch has raised as part of
> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8263412).
testing:
- [x] `:jdk_core` against `{macos,windows,linux}-x64`
- [x] `:jdk_svc` against `{macos,windows,linux}-x64`
- [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories
against `{macos,windows,linux}-x64`
Thanks,
-- Igor
-
Commit messages:
- make ClassFileInstaller.Manifest::from* public
- updated copyright years
- import j.t.l.helpers.ClassFileInstaller
- added jdk/test/lib/helpers/ClassFileInstaller
Changes: https://git.openjdk.java.net/jdk/pull/2932/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2932=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8263412
Stats: 473 lines in 114 files changed: 145 ins; 229 del; 99 mod
Patch: https://git.openjdk.java.net/jdk/pull/2932.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2932/head:pull/2932
PR: https://git.openjdk.java.net/jdk/pull/2932
Re: RFR(S) : 8246494 : introduce vm.flagless at-requires property
RFR got migrated to github as https://github.com/openjdk/jdk/pull/2800
-- Igor
On Jun 5, 2020, at 9:10 AM, Igor Ignatyev
mailto:igor.ignat...@oracle.com>> wrote:
Hi Per,
you are reading this correctly, make
TEST=test/hotspot/jtreg/gc/z/TestSmallHeap.java JTREG="VM_OPTIONS=-XX:+UseZGC"
won't execute gc/z/TestSmallHeap.java; and I don't see it to be incorrect. Let
me try to explain why using gc/z/TestSmallHeap.java as a running example.
A hotspot test is expected not to be just runnable in an out-of-box
configuration, but also to serve its purpose as much as possible (which is not
always 100% given some tests require special build flavor, environment setup,
etc); in other words, a test is to at least have all necessary VM flags within
it and not to hope that someone will provide them. gc/z/TestSmallHeap.java does
that, it explicitly selects zGC, so there is no need for -XX:+UseZGC to achieve
that. Given this test can be run only when zGC can be selected, it @requires
vm.gc.Z, which is set to true if zGC is already explicitly selected or if zGC
is available and no other GC is specified, and the latter holds for an
out-of-box configuration (assuming that zGC is available in the JVM under
test); thus, again, you don't have to specify -XX:+UseZGC to run this test. So
there are no "technical" reasons to run gc/z/TestSmallHeap.java (or any other
gc/z/ tests) with -XX:+UseZGC. The proposed patches don't change that fact in
any way.
The patches exclude the tests that ignore external VM flags from execution if
any significant VM flags are specified. gc/z/TestSmallHeap.java ignores all
externally provided VM flags, including -XX:+UseZGC. And although in the case
of -XX:+UseZGC, it's harmless, in almost all other cases it's not. Just to give
you a few examples:
Let's say you are fixing a bug in zGC which could be reproduced by
gc/z/TestSmallHeap.java. You came up with two alternative solutions, one of
which is guarded by `if (UseNewCode)`. To test these solutions, you ran gc/z
tests twice: with -XX:+UseZGC -XX:+UseNewCode, and all tests passed; with
XX:+UseZGC, and many tests (but not gc/z/TestSmallHeap.java) failed. So based
on these results, you decided that the guarded solution is perfect, cleaned up
the code, sent it out for review, got it pushed, and minutes later found out
that gc/z/TestSmallHeap.java and some other tests which ignore VM flags failed.
It would take you some time, to realize that you hadn't tested your UseNewCode
solution by these tests. Yet were these tests excluded from your testing, it
would be much easier for you to spot that and react accordingly.
Here is another scenario, you decided to change the default value of ZUncommit,
so you ran different tests with `XX:+UseZGC -XX:-ZUncommit`, all green, you
pushed a trivial change s/true/false in z_globals.hpp, next thing you knew a
bunch of zGC specific tests failed in CI. And again, these were the tests that
silently ignored `XX:+UseZGC -XX:-ZUncommit`.
Or a slight variation, zGC-supported was added to a future JIT, gc/z tests were
run with the flag combination which enabled the future JIT, all passed, the
victory was declared; N releases later; default JIT got changed to the future
JIT; the next CI build is a disaster, with lots of tests failing from the bugs
which had not been found N/2 years ago.
Although I understand that it might take some getting used to from you and
others who used to run gc/x tests with -XX:+Use${X}GC, I am certain that this
will improve the overall quality of hotspot, save not only machine time (from
running these tests with other flags) but engineers time from analyzing
surprising failures, and increase confidence and trust in the hotspot test
suite.
In a word, I can see how this can be a bit surprising, yet still less
surprising than the current behavior, but I don't see it as incorrect, it just
surfaces limitations of certain tests. From my (slightly biased) point of view,
it's the right thing to do.
Thanks.
-- Igor
On Jun 5, 2020, at 1:20 AM, Per Liden
mailto:per.li...@oracle.com>> wrote:
Hi Igor,
When looking at the follow-up sub-tasks for this, I see for example this:
http://cr.openjdk.java.net/~iignatyev/8246499/webrev.00/test/hotspot/jtreg/gc/z/TestSmallHeap.java.udiff.html
Maybe I'm misunderstanding how this is supposed to work, but it looks like this
test would now _not_ be executed if I do:
make TEST=test/hotspot/jtreg/gc/z/TestSmallHeap.java
JTREG="VM_OPTIONS=-XX:+UseZGC"
Is that so? In that case, that seems incorrect.
cheers,
Per
On 6/3/20 11:30 PM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00
70 lines changed: 66 ins; 0 del; 4 mod
Hi all,
could you please review the patch which introduces a new @requires property to
filter out the tests which ignore externally provided JVM flags?
the idea behind this patch is to have a way to clearly mark tests which ignore
flags,
Re: RFR: 8260296: SA's dumpreplaydata fails [v3]
On Thu, 4 Feb 2021 09:40:33 GMT, Roland Westrelin wrote: >> `OutputAnalyzer`/ `OutputBuffer` already have an exit value, which arguable >> has nothing to do w/ the text output either, so I think it's ok to add a pid >> into `OutputBuffer` i-face and both of its implementations. > > I just pushed a change that does this. Does that look ok to you? yes, thank you. - PR: https://git.openjdk.java.net/jdk/pull/2195
Re: RFR: 8260296: SA's dumpreplaydata fails [v4]
On Thu, 4 Feb 2021 09:44:13 GMT, Roland Westrelin wrote: >> I noticed that the SA's dumpreplaydata command fails with: >> >> java.lang.AssertionError: CLHSDB wasn't run successfully: Opening core file, >> please wait... >> hsdb> Exception in thread "main" java.lang.InternalError: ciMetadata does >> not appear to be polymorphic >> >> with a simple test program. This happens because the SA can't find the >> vtable symbol for ciMetadata (build produced by gcc 9.2.1). AFAIU, >> there's nothing in our build system that hides that symbol. I had to >> move one method's definition from the header file to the cpp file for >> the symbol to be visible again. >> >> We have a test that checks dumpreplaydata but it doesn't catch that >> problem. The test produces a replay file from a core file with the SA >> by running a simple test with -Xcomp and CICrash=1. So the replay data >> has very little or no profile data (which is what causes the problem >> above). I propose running a slightly more complicated test method and >> crashing after the method has had time to run for long enough to >> collect profile data. >> >> The other shortcoming of the test is that it doesn't look at the >> content of the replay file. It only warns if they differ. The replay >> file produced by the VM and the one produced by the SA should be >> identical (except for comment lines). So I propose we check that. >> >> Finally, I can't run that test on my system because core files are >> handled by systemd (I'm running some recent version of fedora). I >> suppose, the system can be configured differently but having the test >> work out the box is nice. I extended the test case to handle that. >> >> With the improved test, there are a few differences between the VM and >> SA replay files caused by VM changes that were not mirrored in the >> SA. I fixed those. > > Roland Westrelin 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 10 additional > commits since the last revision: > > - pid in OutputAnalyzer > - pid in OutputAnalyzer > - Merge branch 'master' into JDK-8260296 > - convert all tests > - Merge branch 'master' into JDK-8260296 > - use CoreUtils > - whitespaces > - SA fixes > - VM fix > - test LGTM, you still need to update the copyright years as @plummercj [mentioned](https://github.com/openjdk/jdk/pull/2195#issuecomment-768115732). - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2195
Re: RFR: 8260296: SA's dumpreplaydata fails [v3]
On Wed, 3 Feb 2021 12:04:00 GMT, Roland Westrelin wrote:
>> test/lib/jdk/test/lib/process/ProcessTools.java line 461:
>>
>>> 459: }
>>> 460:
>>> 461: static public class OutputAnalyzerAndPID {
>>
>> can we either change `OutputAnalyzer` to store pid (and use -1 for cases
>> when there is no one) or make `OutputAnalyzer` non-final and have
>> `OutputAnalyzerAndPID` extending `OutputAnalyzer`?
>
> Thanks for reviewing this. I did not store the pid in the OutputAnalyzer
> because it doesn't seem to belong there as it has nothing to do with the text
> output of a test. But if you think that's ok. that's fine with me too. Do you
> prefer an extra field in OutputAnalyzer or a new subclass?
`OutputAnalyzer`/ `OutputBuffer` already have an exit value, which arguable has
nothing to do w/ the text output either, so I think it's ok to add a pid into
`OutputBuffer` i-face and both of its implementations.
-
PR: https://git.openjdk.java.net/jdk/pull/2195
Re: RFR: 8260296: SA's dumpreplaydata fails [v3]
On Wed, 27 Jan 2021 08:07:04 GMT, Roland Westrelin wrote:
>> I noticed that the SA's dumpreplaydata command fails with:
>>
>> java.lang.AssertionError: CLHSDB wasn't run successfully: Opening core file,
>> please wait...
>> hsdb> Exception in thread "main" java.lang.InternalError: ciMetadata does
>> not appear to be polymorphic
>>
>> with a simple test program. This happens because the SA can't find the
>> vtable symbol for ciMetadata (build produced by gcc 9.2.1). AFAIU,
>> there's nothing in our build system that hides that symbol. I had to
>> move one method's definition from the header file to the cpp file for
>> the symbol to be visible again.
>>
>> We have a test that checks dumpreplaydata but it doesn't catch that
>> problem. The test produces a replay file from a core file with the SA
>> by running a simple test with -Xcomp and CICrash=1. So the replay data
>> has very little or no profile data (which is what causes the problem
>> above). I propose running a slightly more complicated test method and
>> crashing after the method has had time to run for long enough to
>> collect profile data.
>>
>> The other shortcoming of the test is that it doesn't look at the
>> content of the replay file. It only warns if they differ. The replay
>> file produced by the VM and the one produced by the SA should be
>> identical (except for comment lines). So I propose we check that.
>>
>> Finally, I can't run that test on my system because core files are
>> handled by systemd (I'm running some recent version of fedora). I
>> suppose, the system can be configured differently but having the test
>> work out the box is nice. I extended the test case to handle that.
>>
>> With the improved test, there are a few differences between the VM and
>> SA replay files caused by VM changes that were not mirrored in the
>> SA. I fixed those.
>
> Roland Westrelin 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 seven additional
> commits since the last revision:
>
> - convert all tests
> - Merge branch 'master' into JDK-8260296
> - use CoreUtils
> - whitespaces
> - SA fixes
> - VM fix
> - test
a few more nits
test/lib/jdk/test/lib/process/ProcessTools.java line 461:
> 459: }
> 460:
> 461: static public class OutputAnalyzerAndPID {
Suggestion:
public static class OutputAnalyzerAndPID {
test/lib/jdk/test/lib/process/ProcessTools.java line 461:
> 459: }
> 460:
> 461: static public class OutputAnalyzerAndPID {
can we either change `OutputAnalyzer` to store pid (and use -1 for cases when
there is no one) or make `OutputAnalyzer` non-final and have
`OutputAnalyzerAndPID` extending `OutputAnalyzer`?
-
Changes requested by iignatyev (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2195
Re: RFR: 8260296: SA's dumpreplaydata fails [v3]
On Wed, 27 Jan 2021 08:07:04 GMT, Roland Westrelin wrote:
>> I noticed that the SA's dumpreplaydata command fails with:
>>
>> java.lang.AssertionError: CLHSDB wasn't run successfully: Opening core file,
>> please wait...
>> hsdb> Exception in thread "main" java.lang.InternalError: ciMetadata does
>> not appear to be polymorphic
>>
>> with a simple test program. This happens because the SA can't find the
>> vtable symbol for ciMetadata (build produced by gcc 9.2.1). AFAIU,
>> there's nothing in our build system that hides that symbol. I had to
>> move one method's definition from the header file to the cpp file for
>> the symbol to be visible again.
>>
>> We have a test that checks dumpreplaydata but it doesn't catch that
>> problem. The test produces a replay file from a core file with the SA
>> by running a simple test with -Xcomp and CICrash=1. So the replay data
>> has very little or no profile data (which is what causes the problem
>> above). I propose running a slightly more complicated test method and
>> crashing after the method has had time to run for long enough to
>> collect profile data.
>>
>> The other shortcoming of the test is that it doesn't look at the
>> content of the replay file. It only warns if they differ. The replay
>> file produced by the VM and the one produced by the SA should be
>> identical (except for comment lines). So I propose we check that.
>>
>> Finally, I can't run that test on my system because core files are
>> handled by systemd (I'm running some recent version of fedora). I
>> suppose, the system can be configured differently but having the test
>> work out the box is nice. I extended the test case to handle that.
>>
>> With the improved test, there are a few differences between the VM and
>> SA replay files caused by VM changes that were not mirrored in the
>> SA. I fixed those.
>
> Roland Westrelin 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 seven additional
> commits since the last revision:
>
> - convert all tests
> - Merge branch 'master' into JDK-8260296
> - use CoreUtils
> - whitespaces
> - SA fixes
> - VM fix
> - test
test/lib/jdk/test/lib/util/CoreUtils.java line 166:
> 164: for (int i = 0; i < 10; i++) {
> 165: Thread.sleep(5000);
> 166: OutputAnalyzer out =
> ProcessTools.executeProcess("coredumpctl", "dump", "-1", "-o", core,
> Long.valueOf(pid).toString());
you can use `String::valueOf` to a string represenatation of `long`:
Suggestion:
OutputAnalyzer out =
ProcessTools.executeProcess("coredumpctl", "dump", "-1", "-o", core,
String.valueOf(pid));
-
PR: https://git.openjdk.java.net/jdk/pull/2195
Re: RFR: 8259799: vmTestbase/nsk/jvmti/Breakpoint/breakpoint001 is incorrect [v2]
On Thu, 14 Jan 2021 20:32:17 GMT, Leonid Mesnik wrote:
>> est vmTestbase/nsk/jvmti/Breakpoint/breakpoint001 has incorrect check of
>> strcmp results here:
>>
>> for (i=0; i> if (strcmp(methNam,METHODS[i][0]) &&
>> strcmp(methSig,METHODS[i][1])) {
>> printf("CHECK PASSED: method name: "%s"\tsignature: "%s" %d\n",
>>methNam, methSig, i);
>> if (checkStatus == PASSED)
>> bpEvents[i]++;
>> break;
>> }
>>
>> So test passed when both strcmp (name,sig) are not zero.
>>
>> The test passes only because there are 2 methods that are checked and it
>> increases counters for "incorrect" methods.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> copyrights and ident fixed
Marked as reviewed by iignatyev (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/2084
Re: RFR: 8259799: vmTestbase/nsk/jvmti/Breakpoint/breakpoint001 is incorrect
On Thu, 14 Jan 2021 19:09:59 GMT, Leonid Mesnik wrote:
> est vmTestbase/nsk/jvmti/Breakpoint/breakpoint001 has incorrect check of
> strcmp results here:
>
> for (i=0; i if (strcmp(methNam,METHODS[i][0]) &&
> strcmp(methSig,METHODS[i][1])) {
> printf("CHECK PASSED: method name: "%s"\tsignature: "%s" %d\n",
>methNam, methSig, i);
> if (checkStatus == PASSED)
> bpEvents[i]++;
> break;
> }
>
> So test passed when both strcmp (name,sig) are not zero.
>
> The test passes only because there are 2 methods that are checked and it
> increases counters for "incorrect" methods.
@lmesnik , looks good to me, besides a few nits.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/Breakpoint/breakpoint001/breakpoint001.cpp
line 2:
> 1: /*
> 2: * Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights
> reserved.
it's 2021
test/hotspot/jtreg/vmTestbase/nsk/jvmti/Breakpoint/breakpoint001/breakpoint001.cpp
line 173:
> 171:
> 172: for (i=0; i 173: if (strcmp(methNam,METHODS[i][0]) == 0 &&
could you please add space before `,`?
-
Marked as reviewed by iignatyev (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2084
Withdrawn: 8254861: reformat code in vmTestbase/nsk/jdb
On Thu, 15 Oct 2020 22:47:43 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this patch which reformats/cleans up the code of > vmTestbase/nsk/jdb tests? > > the part of this patch is a spinoff from #350. > > testing: > * [x] `vmTestbase/nsk/jdb` tests on `linux-x64-debug` This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8256181: Remove Allocation of old generation on alternate memory devices functionality [v2]
On Wed, 11 Nov 2020 15:38:10 GMT, Thomas Schatzl wrote: >> Hi all, >> >> can I get reviews for this change that removes the "Allocation of old >> generation of Java heap on alternate memory devices" functionality >> introduced with JDK 12 with >> [JDK-8202286](https://bugs.openjdk.java.net/browse/JDK-8202286) due to being >> >> - not used by anyone >> - not maintained by anyone, i.e. several bugs open for a long time and bit >> rotting >> - requiring some workarounds for new feature development wrt to heap >> management >> >> All flags covered by this feature were experimental flags, so there are no >> additional procedural issues to take. >> >> I tried to remove all but a few minor cleanups that I thought useful, but of >> course this is very subjective. >> >> Testing: hs-tier1-5 > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > ayang review Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1162
Re: RFR: 8255982: Extend BasicJMapTest to test with different GC Heap [v5]
On Wed, 11 Nov 2020 15:42:36 GMT, Lin Zang wrote: >> The implementation of jmap tool depends on the implementation of object >> iteration by different GC heap. >> This patch extend the BasicJMapTest to cover differet GC Heap. > > Lin Zang has updated the pull request incrementally with one additional > commit since the last revision: > > recover build tag configuration Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1094
Re: RFR: 8255982: Extend BasicJMapTest to test with different GC Heap [v4]
On Wed, 11 Nov 2020 11:24:08 GMT, Lin Zang wrote: >> The implementation of jmap tool depends on the implementation of object >> iteration by different GC heap. >> This patch extend the BasicJMapTest to cover differet GC Heap. > > Lin Zang has updated the pull request incrementally with one additional > commit since the last revision: > > Add test id Hi @linzang , it all looks good to me, I'd, however, keep `@build` tags as they were before, this is one of a few difference b/w jdk and hotspot test suites ( in this particular case, it actually matters as discrepancies can lead to sporadic failures). in jdk test-suite, people use a sufficient set of `@build` tag. -- Igor - Changes requested by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1094
Re: RFR: 8255616: Disable AOT and Graal in Oracle OpenJDK
On Fri, 30 Oct 2020 17:40:51 GMT, Vladimir Kozlov wrote: > We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an > experimental feature. We shipped Graal as an experimental JIT compiler in JDK > 10. We haven't seen much use of these features, and the effort required to > support and enhance them is significant. We therefore intend to disable these > features in Oracle builds as of JDK 16. > > We'll leave the sources for these features in the repository, in case any one > else is interested in building them. But we will not update or test them. > > We'll continue to build and ship JVMCI as an experimental feature in Oracle > builds. > > Tested changes in all tiers. > > I verified that with these changes I still able to build Graal in open repo > and run graalunit testing: > > `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh > /mydir/graalunit_lib/` > `open$ bash configure --with-debug-level=fastdebug > --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg` > `open$ make jdk-image` > `open$ make test-image` > `open$ make run-test TEST=compiler/graalunit/HotspotTest.java` LGTM - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/960
Re: RFR: 8255200: ProblemList com/sun/jdi/EATests.java for ZGC
On Wed, 21 Oct 2020 21:04:55 GMT, Daniel D. Daugherty wrote: > Reduce the noise in the JDK16 CI by ProblemListing com/sun/jdi/EATests.java > for ZGC. Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/787
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Wed, 21 Oct 2020 20:12:51 GMT, Chris Plummer wrote: > Do we have coding guidelines that say to indent them 8? old "official" guidelines don't tell much about it, @aioobe's guidelines don't special-case array initiation, so it is covered by: > A continuation line should be indented in one of the following four ways > Variant 1: With 8 extra spaces relative to the indentation of the previous > line. > Variant 2: With 8 extra spaces relative to the starting column of the wrapped > expression. > Variant 3: Aligned with previous sibling expression (as long as it is clear > that it’s a continuation line) > Variant 4: Aligned with previous method call in a chained expression. which will suggest that 8 is the right indent here. google's code-style says that "array initializer may _optionally_ be formatted as if it were a "block-like construct.", which would mean 4, but note "may" and "_optionally_". personally I'd prefer consistency here, but if you think it will make the lives of emacs-lovers miserable, although I'm sure you can configure emacs to treat array-init as line continuation, and not as a block, I can revert that. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Tue, 20 Oct 2020 01:13:33 GMT, Chris Plummer wrote: >> although horizontal alignment (of variable names, initialization, >> expressions, etc) seems to somewhat improve >> readability, it almost always associated with a higher maintenance cost, and >> the current consensus is not to do that. >> from 'Horizontal Whitespace' section of the same >> [guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-whitespace): >>> In variable declarations it is not recommended to align types and variables. >> *Motivation* >> The improvement in readability when aligning variable names is negligible >> compared to the efforts needed to keep them >> aligned as the code evolves. Realigning all variables when one of the types >> change also causes unnecessarily >> complicated patches to review. other java code guidelines either discourage >> horizontal alignment or consider it >> optional and provide the same motivation as above to why it's better not to >> have it. > > While in general I agree that gratuitous aligning of variables is not > desirable, this is a special case where it adds > value (enough that I think it's worth doing). The value here is due to the > fact that the variables are building on each > other. When they are aligned it's easier to scan for the variable (and its > value) that another variable builds on. You > can keep your change in place if you feel that consistency and maintainabilty > is more important here than readability. Although I don't think that the test code has the same frequency of changes as the product code, I'd still prefer consistency and maintainability over some small penalty (which is IMHO negligible) in readability. frankly, it's also partially motivated by my laziness as I don't want to go over all these files again and examine each and every initialization for possible (scanty) readability improvements from horizontal alignment. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 23:56:00 GMT, Chris Plummer wrote: >> b/c you get indentation unit for each block, 1st block is `fields001` class >> definition, next block is the array >> initializations (start at L#86 and L#99 for `checkedFields1` and >> `checkedFields2` respecitively) > > I don't follow. You seem to have added an extra 4 (after having already > indented 4 extra spaces) simply because it is > an array initialization. Why is the indentation any different than it would > be for something like a "for" loop block. > The body of the block is indented 4 spaces more than the "for" statement. > For the static initialization the body of > that "static" block should be indented 4 more than the "static" statement. I > don't see anything in the style guide that > would indicate otherwise. oh, sorry, it appears I was trying to explain the new code while looking at the old one... full disclaimer, which I thought was obvious, the vast majority of the changes here aren't done by me, they are done by auto-formatter, or more precisely by IntelliJ IDEA's formatter. IDEA doesn't consider array initialization as a new block, and what's actually at play here is an indent for statement continuation (which is 2x of regular indent). - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 18:37:58 GMT, Chris Plummer wrote: >> Igor Ignatyev has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright years > > test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002.java > line 87: > >> 85: static final String DEBUGGEE_CLASS = TEST_CLASS + "a"; >> 86: static final String FIRST_BREAK = DEBUGGEE_CLASS + ".main"; >> 87: static final String LAST_BREAK = DEBUGGEE_CLASS + ".lastBreak"; > > I'm not so sure I'd consider this change an improvement. Maybe remove some of > the extra spaces but keep the alignment. although horizontal alignment (of variable names, initialization, expressions, etc) seems to somewhat improve readability, it almost always associated with a higher maintenance cost, and the current consensus is not to do that. from 'Horizontal Whitespace' section of the same [guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-whitespace): > In variable declarations it is not recommended to align types and variables. *Motivation* The improvement in readability when aligning variable names is negligible compared to the efforts needed to keep them aligned as the code evolves. Realigning all variables when one of the types change also causes unnecessarily complicated patches to review. other java code guidelines either discourage horizontal alignment or consider it optional and provide the same motivation as above to why it's better not to have it. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 18:51:15 GMT, Chris Plummer wrote: >> Igor Ignatyev has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright years > > test/hotspot/jtreg/vmTestbase/nsk/jdb/fields/fields001/fields001.java line > 108: > >> 106: "ii_aa", "oi_aa", >> 107: "ii_aaa", "oi_aaa" >> 108: }; > > Why are these indented 8 instead of 4? b/c you get indentation unit for each block, 1st block is `fields001` class definition, next block is the array initializations (start at L#86 and L#99 for `checkedFields1` and `checkedFields2` respecitively) > test/hotspot/jtreg/vmTestbase/nsk/jdb/hidden_class/hc001/hc001.java line 323: > >> 321: "xx.yyy/0x111/0x222", >> 322: "xx./0x111.0x222", >> 323: "xx.yyy.zzz/" > > Why are these indented 8 instead of 4? due to the same reasons in the case w/ `fields001`, these lines have 3 unit indentation, 1st for `hc001` class, 2nd for `testInvalidCommands` method, 3rd for `invClassNames` array initialization, so they have 3x4 = 12 spaces. - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
On Mon, 19 Oct 2020 18:44:56 GMT, Chris Plummer wrote:
>> Igor Ignatyev has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update copyright years
>
> test/hotspot/jtreg/vmTestbase/nsk/jdb/caught_exception/caught_exception002/caught_exception002a.java
> line 42:
>
>> 40: }
>> 41:
>> 42: public int runIt(String[] args, PrintStream out) {
>
> Is there a style guide that says this is the preferred way to declare an
> array type? I count 3700 occurrences of
> "String args[]" in ours tests.
yes, there is, e.g. [Java Style
Guidelines](http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-variable-declarations)
by Andreas
Lundblad:
> Square brackets of arrays should be at the type (String[] args) and not on
> the variable (String args[]).
although @aioobe's guidelines have been accepted (yet?) to the openjdk guide
(see openjdk/guide#14), this particular
item is agreed on and accepted almost unanimously in the industry.
-
PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb
On Thu, 15 Oct 2020 22:47:43 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this patch which reformats/cleans up the code of > vmTestbase/nsk/jdb tests? > > the part of this patch is a spinoff from #350. > > testing: > * [x] `vmTestbase/nsk/jdb` tests on `linux-x64-debug` ping? - PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: 8254861: reformat code in vmTestbase/nsk/jdb [v2]
> Hi all, > > could you please review this patch which reformats/cleans up the code of > vmTestbase/nsk/jdb tests? > > the part of this patch is a spinoff from #350. > > testing: > * [x] `vmTestbase/nsk/jdb` tests on `linux-x64-debug` Igor Ignatyev has updated the pull request incrementally with one additional commit since the last revision: update copyright years - Changes: - all: https://git.openjdk.java.net/jdk/pull/689/files - new: https://git.openjdk.java.net/jdk/pull/689/files/1b6fa98f..e43129d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=689=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=689=00-01 Stats: 64 lines in 64 files changed: 0 ins; 0 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/689.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/689/head:pull/689 PR: https://git.openjdk.java.net/jdk/pull/689
RFR: 8254861: reformat code in vmTestbase/nsk/jdb
Hi all, could you please review this patch which reformats/cleans up the code of vmTestbase/nsk/jdb tests? the part of this patch is a spinoff from #350. testing: * [x] `vmTestbase/nsk/jdb` tests on `linux-x64-debug` - Commit messages: - 8254861: reformat code in vmTestbase/nsk/jdb Changes: https://git.openjdk.java.net/jdk/pull/689/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=689=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254861 Stats: 3829 lines in 130 files changed: 572 ins; 489 del; 2768 mod Patch: https://git.openjdk.java.net/jdk/pull/689.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/689/head:pull/689 PR: https://git.openjdk.java.net/jdk/pull/689
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Thu, 8 Oct 2020 11:00:41 GMT, Aleksei Voitylov wrote: > @iignatev I resolved the conflict in whitebox.cpp and fixed a minor style nit > on the way. Could you take a look? LGTM - PR: https://git.openjdk.java.net/jdk/pull/49
Integrated: 8253878: clean up nsk/share/jvmti/ArgumentHandler
On Wed, 30 Sep 2020 22:11:43 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this small patch which reformats `ArgumentHandler`, > removes unused > `findOptionStringValue(String name, String defaultValue)` method, fixes typos? This pull request has now been integrated. Changeset: 55c282bb Author: Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/55c282bb Stats: 43 lines in 1 file changed: 1 ins; 25 del; 17 mod 8253878: clean up nsk/share/jvmti/ArgumentHandler Reviewed-by: cjplummer - PR: https://git.openjdk.java.net/jdk/pull/443
Re: RFR: 8253878: clean up nsk/share/jvmti/ArgumentHandler
On Wed, 30 Sep 2020 23:28:11 GMT, Chris Plummer wrote: >> Hi all, >> >> could you please review this small patch which reformats `ArgumentHandler`, >> removes unused >> `findOptionStringValue(String name, String defaultValue)` method, fixes >> typos? > > Marked as reviewed by cjplummer (Reviewer). thanks Chris. - PR: https://git.openjdk.java.net/jdk/pull/443
Re: RFR: 8253878: clean up nsk/share/jvmti/ArgumentHandler
On Wed, 30 Sep 2020 22:58:38 GMT, Chris Plummer wrote: >> Hi all, >> >> could you please review this small patch which reformats `ArgumentHandler`, >> removes unused >> `findOptionStringValue(String name, String defaultValue)` method, fixes >> typos? > > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/ArgumentHandler.java line 104: > >> 102: * or null if this option has no value >> 103: * @return true if option is admissible and has proper value; >> 104: * false if option is not admissible > > Why is this line not indented? I guess b/c this is how IDEA thinks `@return` javadoc should be aligned. I have checked `java.base` javadoc, and some classes have indentation here, some don't. either way is fine w/ me. - PR: https://git.openjdk.java.net/jdk/pull/443
Re: RFR: 8253878: clean up nsk/share/jvmti/ArgumentHandler
On Wed, 30 Sep 2020 23:00:30 GMT, Chris Plummer wrote: >> Hi all, >> >> could you please review this small patch which reformats `ArgumentHandler`, >> removes unused >> `findOptionStringValue(String name, String defaultValue)` method, fixes >> typos? > > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/ArgumentHandler.java line 126: > >> 124: * If optionString is null this method just does nothing. >> 125: * >> 126: * @throws BadOption if known option has illegal value > > Shouldn't BadOption use or ? no, `@throws` should be followed by exception class-name, see [1] [1]: https://docs.oracle.com/en/java/javase/15/docs/specs/javadoc/doc-comment-spec.html#throws - PR: https://git.openjdk.java.net/jdk/pull/443
RFR: 8253878: clean up nsk/share/jvmti/ArgumentHandler
Hi all, could you please review this small patch which reformats `ArgumentHandler`, removes unused `findOptionStringValue(String name, String defaultValue)` method, fixes typos? - Commit messages: - 8253878: clean up nsk/share/jvmti/ArgumentHandler Changes: https://git.openjdk.java.net/jdk/pull/443/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=443=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253878 Stats: 43 lines in 1 file changed: 1 ins; 25 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/443.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/443/head:pull/443 PR: https://git.openjdk.java.net/jdk/pull/443
Integrated: 8252003: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jvmti
On Fri, 25 Sep 2020 23:48:30 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review the patch which removes `PropertyResolvingWrapper`
> from `vmTestbase/nsk/jvmti` tests? as
> `jtreg` doesn't support spaces in the arguments and doesn't handle `"` in any
> special ways, the patch also:
> - `s/"-javaOpts=/-javaOpts="/`
> - makes `nsk.jvmti.scenarios.general_functions.GF08` to use 2nd arg as
> `verboseType` and 3rd and the rest args
>concatenated as `phrase` and updates the tests accordingly
> - removes spaces and surrounding `"` from `nsk.jvmti.test.property*`
> - removes `"` surrounding `-agentlib:`, replaces spaces in `-agentlib` with
> `,` and updates `ArgumentHandler` to treat
>`,` (as well as ` ` and `~`) as options delimiters, so it's consistent w/
> `jvmti_tools.cpp`
>
> testing: ✅ `vmTestbase/nsk/jvmti` on {linux,windows,macos}-x64
This pull request has now been integrated.
Changeset: ca0e014e
Author:Igor Ignatyev
URL: https://git.openjdk.java.net/jdk/commit/ca0e014e
Stats: 599 lines in 165 files changed: 14 ins; 149 del; 436 mod
8252003: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jvmti
Reviewed-by: cjplummer, sspitsyn
-
PR: https://git.openjdk.java.net/jdk/pull/370
Re: RFR: 8252003: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jvmti [v2]
On Wed, 30 Sep 2020 21:34:22 GMT, Serguei Spitsyn wrote:
>>> What are the meaning of these properties ?:
>>> -Dnsk.jvmti.test.property=value_of_nsk.jvmti.test.property
>>> -Dnsk.jvmti.test.property=initial_value_of_nsk.jvmti.test.property
>>> -Dnsk.jvmti.test.property.empty.new=initial_value_of_nsk.jvmti.test.property.empty.new
>>
>> these are properties used by jvmti tests, the tests compare the actual value
>> w/ expected values hardcoded in the test
>> code. the purpose of these properties is different in different tests,
>> ranging from just checking values by an agent to
>> changing the value by the agent and checking that the value was changed.
>>
>>>
>>> Could you, please, explain this change? :
>>>
>>>
>> ... scenarios/general_functions/GF08/gf08t001/TestDriver.java
>> @@ -87,8 +87,8 @@ public static void main(String[] args) throws Exception {
>> nsk.jvmti.scenarios.general_functions.GF08.gf08t.main(new String[] {
>> "gf08t001",
>>
>> nsk.jvmti.scenarios.general_functions.GF08.gf08t001.class.getName(),
>> - keyPhrase,
>> - "gc"});
>> + "gc",
>> + keyPhrase});
>> }
>> }
>>
>> it's the same changes I described
>> [above](https://github.com/openjdk/jdk/pull/370#discussion_r497655837), in
>> two words,
>> due to how jtreg handles spaces and `"` in the arguments, the positions of
>> `phrase` and `verboseType` have been
>> switched.
>
> Okay, thanks!
Serguei, Chris, thanks a lot for your reviews.
-
PR: https://git.openjdk.java.net/jdk/pull/370
