Re: RFR: 8255526: Enable jcheck whitespace checking of build system files
On Wed, 28 Oct 2020 22:31:06 GMT, Magnus Ihse Bursie wrote: >> Awesome, finally! > > I need to put this on hold for a while. It turned out that the Skara jcheck > disallows initial tabs, which makes sense for sane source code formats (java, > c++) but is unfortunately an incorrect assumption for makefiles. > > (I tested that the modification to jcheck detected trailing whitespaces, but > it never occurred to me that I needed to modify a line starting with tab to > show that this fix would not work...) Fix for Skara in PR https://github.com/openjdk/skara/pull/929. - PR: https://git.openjdk.java.net/jdk/pull/898
Re: make test TEST="micro:" got java.lang.UnsupportedClassVersionError
FWIW I just integrated the change to remove --enable-preview from the micros build, so the UnsupportedClassVersionError should be gone if you sync up. No --enable-preview needed. /Claes On 2020-10-28 11:09, Liu, Xin wrote: Hi, Claes, Thank you for the great guideline of micro benchmarking. now I have a clearer picture for it. I'm happy to learn that 5-10 minutes for each Benchmark! Let me polish mine. Thanks, --lx On 10/27/20, 3:59 AM, "Claes Redestad" wrote: CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. Hi lx, the need to add --enable-preview is known, and it's a bug introduced a couple of months ago thanks to a suggestion I did (without realizing that compiling one micro with --enable-preview would poison all of them..). This issue is tracked by a few bugs, see https://bugs.openjdk.java.net/browse/JDK-8250669 and https://bugs.openjdk.java.net/browse/JDK-8253828 The fix will likely be to remove the --enable-preview from the micros build after Records goes out of preview and then think more carefully on how to add support for benchmarking preview features. As to your other question: When adding a microbenchmark it's appreciated if the default configuration makes it as quick to run as possible without sacrificing quality. Often the default number of forks, iteration runtime et.c. can be tuned down by using custom settings in @Fork et.c. Custom parameters used can be limited to a select handful. Still it's normal for a "good" microbenchmark to require at least five, usually ten minutes per @Benchmark to produce reasonably trustworthy results. I can't speak for what others do, but we (Oracle) select what to run regularly from the available microbenchmarks at our discretion, though. That means adding a microbenchmark that takes a prohibitively long time won't be disruptive to our nightly testing, but for obvious (mostly economic) reasons we'd be unlikely to add an extremely long-running micro to regular coverage. /Claes On 2020-10-27 10:06, Liu, Xin wrote: > Hi, > > I follow the instruction on https://urldefense.com/v3/__https://htmlpreview.github.io/?https:**Agithub.com*openjdk*jdk*blob*master*doc*testing.html*microbenchmarks__;Ly8vLy8vLy8j!!GqivPVa7Brio!ND3mbOTCkCdkICq5NKxlPCWh7dWYhtMdQdg0GabOEKe0FlDTJDkj-lfNyWujH7AmDts$ to run a new microbenchmark. > I got error message as follows. > java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/java/lang/reflect/proxy/jmh_generated/ProxyBench_newProxyInstance1i_jmhTest (class file version 60.65535). Try running with '--enable-preview' > at java.base/java.lang.ClassLoader.defineClass1(Native Method) > at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1010) > at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150) > at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:855) > at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:753) > at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:676) > at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:634) > at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182) > at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519) > at java.base/java.lang.Class.forName0(Native Method) > at java.base/java.lang.Class.forName(Class.java:377) > at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) > at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) > at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:232) > at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:138) > at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:75) > at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) > at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) > > > If I add an extra flag “—enable-preview” , the micro bench will work. > make test TEST="micro:java.lang.reflect" MICRO="VM_OPTIONS=--enable-preview" > > I am using jmh from make/devkit/createJMHBundle.sh, which gives me jmh-core-1.26.jar > Is that jmh-core too old or we should adjust RunTest.gmk a little bit to have that flag? > > Does Openjdk CI run benchmarks to detect performance regression? If so, could you
Integrated: 8250669: Running JMH micros is broken after JDK-8248135
On Wed, 28 Oct 2020 20:50:49 GMT, Claes Redestad wrote: > A microbenchmark with --enable-preview was added in JDK-8248135. This had the > unintended side effect that we now had to always run with --enable-preview. > > With records out of preview, I think the best course of action now is to not > build the micros with preview enabled, and leave it for a future RFE to > figure out how to add proper support for preview features. This pull request has now been integrated. Changeset: a7595b2a Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/a7595b2a Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod 8250669: Running JMH micros is broken after JDK-8248135 Reviewed-by: ecaspole, erikj - PR: https://git.openjdk.java.net/jdk/pull/914
Re: RFR: 8250669: Running JMH micros is broken after JDK-8248135
On Wed, 28 Oct 2020 20:50:49 GMT, Claes Redestad wrote: > A microbenchmark with --enable-preview was added in JDK-8248135. This had the > unintended side effect that we now had to always run with --enable-preview. > > With records out of preview, I think the best course of action now is to not > build the micros with preview enabled, and leave it for a future RFE to > figure out how to add proper support for preview features. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/914
Re: RFR: 8250669: Running JMH micros is broken after JDK-8248135
On Wed, 28 Oct 2020 20:56:55 GMT, Eric Caspole wrote: >> A microbenchmark with --enable-preview was added in JDK-8248135. This had >> the unintended side effect that we now had to always run with >> --enable-preview. >> >> With records out of preview, I think the best course of action now is to not >> build the micros with preview enabled, and leave it for a future RFE to >> figure out how to add proper support for preview features. > > Looks good and thanks for doing this as soon as practical. > Eric @ericcaspole, @erikj79, thanks for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/914
Re: RFR: 8255526: Enable jcheck whitespace checking of build system files
On Wed, 28 Oct 2020 12:49:23 GMT, Erik Joelsson wrote: >> With the new Skara tooling, we can finally enable whitespace testing for >> build system files (makefiles and autoconf files). > > Awesome, finally! I need to put this on hold for a while. It turned out that the Skara jcheck disallows initial tabs, which makes sense for sane source code formats (java, c++) but is unfortunately an incorrect assumption for makefiles. (I tested that the modification to jcheck detected trailing whitespaces, but it never occurred to me that I needed to modify a line starting with tab to show that this fix would not work...) - PR: https://git.openjdk.java.net/jdk/pull/898
Integrated: 8255530: Additional cleanup after JDK-8235710 (elliptic curve removal)
On Wed, 28 Oct 2020 18:42:01 GMT, Magnus Ihse Bursie wrote: > The fix for JDK-8235710 (Remove the legacy elliptic curves) unfortunately did > not remove all code related to elliptic curves in the build system. This pull request has now been integrated. Changeset: edd19888 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/edd19888 Stats: 50 lines in 2 files changed: 0 ins; 50 del; 0 mod 8255530: Additional cleanup after JDK-8235710 (elliptic curve removal) Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/911
RFR: 8250669: Running JMH micros is broken after JDK-8248135
A microbenchmark with --enable-preview was added in JDK-8248135. This had the unintended side effect that we now had to always run with --enable-preview. With records out of preview, I think the best course of action now is to not build the micros with preview enabled, and leave it for a future RFE to figure out how to add proper support for preview features. - Commit messages: - Drop --enable-preview from microbenchmarks Changes: https://git.openjdk.java.net/jdk/pull/914/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=914=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8250669 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/914.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/914/head:pull/914 PR: https://git.openjdk.java.net/jdk/pull/914
Re: RFR: 8250669: Running JMH micros is broken after JDK-8248135
On Wed, 28 Oct 2020 20:50:49 GMT, Claes Redestad wrote: > A microbenchmark with --enable-preview was added in JDK-8248135. This had the > unintended side effect that we now had to always run with --enable-preview. > > With records out of preview, I think the best course of action now is to not > build the micros with preview enabled, and leave it for a future RFE to > figure out how to add proper support for preview features. Looks good and thanks for doing this as soon as practical. Eric - Marked as reviewed by ecaspole (Committer). PR: https://git.openjdk.java.net/jdk/pull/914
Re: RFR: 8255530: Additional cleanup after JDK-8235710 (elliptic curve removal)
On Wed, 28 Oct 2020 18:42:01 GMT, Magnus Ihse Bursie wrote: > The fix for JDK-8235710 (Remove the legacy elliptic curves) unfortunately did > not remove all code related to elliptic curves in the build system. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/911
RFR: 8255530: Additional cleanup after JDK-8235710 (elliptic curve removal)
The fix for JDK-8235710 (Remove the legacy elliptic curves) unfortunately did not remove all code related to elliptic curves in the build system. - Commit messages: - 8255530: Additional cleanup after JDK-8235710 (elliptic curve removal) Changes: https://git.openjdk.java.net/jdk/pull/911/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=911=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255530 Stats: 50 lines in 2 files changed: 0 ins; 50 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/911.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/911/head:pull/911 PR: https://git.openjdk.java.net/jdk/pull/911
Re: RFR: 8235710: Remove the legacy elliptic curves [v2]
On Wed, 28 Oct 2020 10:52:27 GMT, Magnus Ihse Bursie wrote: >> throw new IllegalStateException( >> new InvalidAlgorithmParameterException( >> "Curve not supported: Private: " + >> ((privNC != null) ? privNC.toString() : " unknown") + >> ", PublicKey:" + >> ((pubNC != null) ? pubNC.toString() : " unknown"))); > > I opened https://bugs.openjdk.java.net/browse/JDK-8255530 for the remaining > cleanup. I have the change in a workspace, just hadn't created the bug yet.. thanks - PR: https://git.openjdk.java.net/jdk/pull/289
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 21 Oct 2020 02:28:30 GMT, Kim Barrett wrote: >> Finally returning to this review that was started in April 2020. I've >> recast it as a github PR. I think the security concern raised by Gil >> has been adequately answered. >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html >> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html >> >> Please review a new function: java.lang.ref.Reference.refersTo. >> >> This function is needed to test the referent of a Reference object without >> artificially extending the lifetime of the referent object, as may happen >> when calling Reference.get. Some garbage collectors require extending the >> lifetime of a weak referent when accessed, in order to maintain collector >> invariants. Lifetime extension may occur with any collector when the >> Reference is a SoftReference, as calling get indicates recent access. This >> new function also allows testing the referent of a PhantomReference, which >> can't be accessed by calling get. >> >> The new function uses native methods whose implementations are in the VM so >> they can use the Access API. It is the intent that these methods will be >> intrinsified by optimizing compilers like C2 or graal, but that hasn't been >> implemented yet. Bear that in mind before rushing off to change existing >> uses of Reference.get. >> >> There are two native methods involved, one in Reference and an override in >> PhantomReference, both package private in java.lang.ref. The reason for this >> split is to simplify the intrinsification. This is a change from the version >> from April 2020; that version had a single native method in Reference, >> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category. >> However, adding support for that category in the compilers adds significant >> implementation effort and complexity. Splitting avoids that complexity. >> >> Testing: >> mach5 tier1 >> Locally (linux-x64) verified the new test passes with various garbage >> collectors. > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > improve wording in refersTo javadoc The API looks good, thanks for getting this in. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: 8255526: Enable jcheck whitespace checking of build system files
On Wed, 28 Oct 2020 10:16:36 GMT, Magnus Ihse Bursie wrote: > With the new Skara tooling, we can finally enable whitespace testing for > build system files (makefiles and autoconf files). Awesome, finally! - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/898
Re: RFR: 8255526: Enable jcheck whitespace checking of build system files
On Wed, 28 Oct 2020 10:16:36 GMT, Magnus Ihse Bursie wrote: > With the new Skara tooling, we can finally enable whitespace testing for > build system files (makefiles and autoconf files). Looks fine to me. - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/898
Re: RFR: 8235710: Remove the legacy elliptic curves [v2]
On Tue, 22 Sep 2020 14:04:55 GMT, Sean Mullan wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> remove JDKOPT_DETECT_INTREE_EC from configure.ac > > throw new IllegalStateException( > new InvalidAlgorithmParameterException( > "Curve not supported: Private: " + > ((privNC != null) ? privNC.toString() : " unknown") + > ", PublicKey:" + > ((pubNC != null) ? pubNC.toString() : " unknown"))); I opened https://bugs.openjdk.java.net/browse/JDK-8255530 for the remaining cleanup. - PR: https://git.openjdk.java.net/jdk/pull/289
RFR: 8255526: Enable jcheck whitespace checking of build system files
With the new Skara tooling, we can finally enable whitespace testing for build system files (makefiles and autoconf files). - Commit messages: - 8255526: Enable jcheck whitespace checking of build system files Changes: https://git.openjdk.java.net/jdk/pull/898/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=898=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255526 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/898.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/898/head:pull/898 PR: https://git.openjdk.java.net/jdk/pull/898
Re: make test TEST="micro:" got java.lang.UnsupportedClassVersionError
Hi, Claes, Thank you for the great guideline of micro benchmarking. now I have a clearer picture for it. I'm happy to learn that 5-10 minutes for each Benchmark! Let me polish mine. Thanks, --lx On 10/27/20, 3:59 AM, "Claes Redestad" wrote: CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. Hi lx, the need to add --enable-preview is known, and it's a bug introduced a couple of months ago thanks to a suggestion I did (without realizing that compiling one micro with --enable-preview would poison all of them..). This issue is tracked by a few bugs, see https://bugs.openjdk.java.net/browse/JDK-8250669 and https://bugs.openjdk.java.net/browse/JDK-8253828 The fix will likely be to remove the --enable-preview from the micros build after Records goes out of preview and then think more carefully on how to add support for benchmarking preview features. As to your other question: When adding a microbenchmark it's appreciated if the default configuration makes it as quick to run as possible without sacrificing quality. Often the default number of forks, iteration runtime et.c. can be tuned down by using custom settings in @Fork et.c. Custom parameters used can be limited to a select handful. Still it's normal for a "good" microbenchmark to require at least five, usually ten minutes per @Benchmark to produce reasonably trustworthy results. I can't speak for what others do, but we (Oracle) select what to run regularly from the available microbenchmarks at our discretion, though. That means adding a microbenchmark that takes a prohibitively long time won't be disruptive to our nightly testing, but for obvious (mostly economic) reasons we'd be unlikely to add an extremely long-running micro to regular coverage. /Claes On 2020-10-27 10:06, Liu, Xin wrote: > Hi, > > I follow the instruction on https://htmlpreview.github.io/?https://github.com/openjdk/jdk/blob/master/doc/testing.html#microbenchmarks to run a new microbenchmark. > I got error message as follows. > java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/java/lang/reflect/proxy/jmh_generated/ProxyBench_newProxyInstance1i_jmhTest (class file version 60.65535). Try running with '--enable-preview' > at java.base/java.lang.ClassLoader.defineClass1(Native Method) > at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1010) > at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150) > at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:855) > at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:753) > at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:676) > at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:634) > at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182) > at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519) > at java.base/java.lang.Class.forName0(Native Method) > at java.base/java.lang.Class.forName(Class.java:377) > at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) > at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) > at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:232) > at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:138) > at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:75) > at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) > at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) > > > If I add an extra flag “—enable-preview” , the micro bench will work. > make test TEST="micro:java.lang.reflect" MICRO="VM_OPTIONS=--enable-preview" > > I am using jmh from make/devkit/createJMHBundle.sh, which gives me jmh-core-1.26.jar > Is that jmh-core too old or we should adjust RunTest.gmk a little bit to have that flag? > > Does Openjdk CI run benchmarks to detect performance regression? If so, could you point me to a guideline of microbenmark? > I lack of the basic sense of a “good” microbenchmark. My concern is the new micro I added is too long to break the established test run. > > Thanks, > --lx > >
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 28 Oct 2020 08:54:31 GMT, Peter Levart wrote: >> Some thoughts regarding the parameter type of refersTo. Summary: I think >> `refersTo(T)` is fine and that we don't want to change it to >> `refersTo(Object)`. >> >> I don't think we have a migration issue similar to generifying collections, >> where there was a possibility of changing `contains(Object)` to >> `contains(E)`. If that had been done, it would have been a source >> compatibility issue, because changing the signature of the method >> potentially affects existing code that calls the method. That doesn't apply >> here because we're adding a new method. >> >> The question now falls to whether it's preferable to have more convenience >> with `refersTo(Object)` or more type-safety with `refersTo(T)`. With the >> generic collections issue, the migration issue probably drove the decision >> to keep `contains(Object)`, but this has resulted in a continual set of >> complaints about the lack of an error when code passes an instance of the >> "wrong" type. I think that kind of error is likely to occur with `refersTo`. >> Since we don't have a source compatibility issue here, we can choose the >> safer API and avoid this kind of problem entirely. >> >> The safer API does raise the possibility of having to add inconvenient >> unchecked casts and local variables in certain places, but I think Mandy's >> comment about the code already having a reference of the "right" type is >> correct. Her prototype webrev linked above shows that having to add >> unchecked casts is fairly infrequent. > >> Some thoughts regarding the parameter type of refersTo. Summary: I think >> `refersTo(T)` is fine and that we don't want to change it to >> `refersTo(Object)`. >> > I agree that we don't have a migration problem here that collections had. So > let it be `refersTo(T)` then. I agree as well. - PR: https://git.openjdk.java.net/jdk/pull/498
Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]
On Wed, 28 Oct 2020 03:46:55 GMT, Stuart Marks wrote: > Some thoughts regarding the parameter type of refersTo. Summary: I think > `refersTo(T)` is fine and that we don't want to change it to > `refersTo(Object)`. > I agree that we don't have a migration problem here that collections had. So let it be `refersTo(T)` then. - PR: https://git.openjdk.java.net/jdk/pull/498
