Re: RFR: 8288425: Footprint regression due MH creation when initializing StringConcatFactory
On Tue, 14 Jun 2022 15:16:27 GMT, Claes Redestad wrote: > Avoid doing MH creation when initializing `StringConcatFactory` by lazily > initializing to a `@Stable` field instead. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9154
Re: RFR: 8287917: System.loadLibrary does not work on Big Sur if JDK is built with macOS SDK 10.15 and earlier
On Wed, 8 Jun 2022 04:59:07 GMT, Yoshiki Sato wrote:
> Please review this PR.
> SDK 10.15 and earlier reports os.version as 10.16 on Big Sur. This fix will
> check if dynamic linker support, which is supported from Big Sur, is
> available or not on the OS even if os.version is reported as 10.16 instead of
> 11. The os.version 10.16 doesn't include the update version like y of
> 10.x.y. Hence the change only see if it is 10.16 or not.
Looks good. Thanks for fixing this.
As a background, Big Sur reports 10.16 as backward compatibility when built
with macOS SDK 10.15 and earlier. Hence this needs to check for os version
10.16 that supports dynamic linker cache.
test/jdk/java/lang/RuntimeTests/loadLibrary/exeLibraryCache/LibraryFromCache.java
line 45:
> 43: public class LibraryFromCache {
> 44: public static void main(String[] args) throws IOException {
> 45: System.out.println("os.version = " +
> System.getProperty("os.version"));
The copyright end year needs to be updated.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9077
Re: RFR: 8288011: StringConcatFactory: Split application of stringifiers [v2]
On Thu, 9 Jun 2022 16:04:43 GMT, Claes Redestad wrote: >> To take optimal advantage of the pre-existing optimization for repeated >> filters we could split the application of different types of stringifiers. >> >> The resulting difference in order of evaluation is not observable by >> conventional means since all reference type share the same object >> stringifier, and the others are filtering primitives (floats and doubles) >> which have been passed by value already. >> >> This change neutral on many concatenation expression shapes, but for any >> complex expressions with interleaving float/double and reference parameters >> it brings a straightforward reduction in rebinds and underlying LFs >> generated. For example on the >> [MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248) >> test there's a modest 2% reduction in total classes loaded with this change >> (from 16209 to 15872) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Improve comments based on review feedback Looks good to me. Like Jorne's observation, it'd benefit everyone if this could generate just a single new lambda form multiple filters, not just for repeated filters if feasible. Maybe something to explore more in the future with a tradeoff of complexity. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.org/jdk/pull/9082
Re: RFR: JDK-8281001 Class::forName(String) defaults to system class loader if the caller is null [v6]
On Wed, 8 Jun 2022 13:07:20 GMT, Tim Prinzing wrote: >> The Class::forName behavior change to match JNI FindClass is a compatible >> change and seems pretty attractive as it would be expected that >> Class::forName would give the same behavior as FindClass which uses the >> system classloader. The test for 8281006 was enhanced to test for this >> change. Merged master to pick up fixes to unrelated test failures to reduce >> noise. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > append bug id being fixed rather than sorted insert Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Class::forName(String) defaults to system class loader if the caller is null [v5]
On Wed, 8 Jun 2022 02:40:15 GMT, Tim Prinzing wrote: >> The Class::forName behavior change to match JNI FindClass is a compatible >> change and seems pretty attractive as it would be expected that >> Class::forName would give the same behavior as FindClass which uses the >> system classloader. The test for 8281006 was enhanced to test for this >> change. Merged master to pick up fixes to unrelated test failures to reduce >> noise. > > Tim Prinzing has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - formatting improvement > - Merge branch 'master' into JDK-8281001 > - Fixed the build of the native c++ test NullCallerTest to specify >the c++ std library as part of the build. Changed the test to >use iostream instead of printf. Enabled the test for Class::forName >which is now located in test/jdk/jni/nullCaller (as part of the >merge of JDK-8287171). > - Merge branch 'master' into JDK-8281001 > - make javadoc consistent with other caller sensitve methods > - Added javadoc comment > - Merge branch 'master' into JDK-8281001 > - JDK-8281001 Examine the behavior of Class::forName if the caller is null Looks good. Thanks for refactoring the tests, making the addition of a new test case much cleaner. test/jdk/jni/nullCaller/NullCallerTest.java line 27: > 25: /** > 26: * @test > 27: * @bug 8280902 8281000 8281001 8281003 8281006 nit: append the bug rather than keeping the list in an increasing order. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8711
Re: CFV: new Core Libraries Group member: Roger Riggs
Vote: yes Mandy
Re: CVF: new Core Libraries Group member: Naoto Sato
Vote: yes Mandy
Integrated: JDK-8287867: Bad merge of jdk/test/lib/util/ForceGC.java causing test compilation error
On Mon, 6 Jun 2022 21:58:20 GMT, Mandy Chung wrote: > A typo in ForceGC.java causes several test failing due to compilation error. This pull request has now been integrated. Changeset: a50b06e8 Author: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/a50b06e85124f61b5133189a2a2e461753d5d9e7 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8287867: Bad merge of jdk/test/lib/util/ForceGC.java causing test compilation error Reviewed-by: dcubed - PR: https://git.openjdk.java.net/jdk/pull/9043
Re: RFR: JDK-8287867: Bad merge of jdk/test/lib/util/ForceGC.java causing test compilation error
On Mon, 6 Jun 2022 21:58:20 GMT, Mandy Chung wrote: > A typo in ForceGC.java causes several test failing due to compilation error. thanks for the prompt review. - PR: https://git.openjdk.java.net/jdk/pull/9043
RFR: JDK-8287867: Bad merge of jdk/test/lib/util/ForceGC.java causing test compilation error
A typo in ForceGC.java causes several test failing due to compilation error. - Commit messages: - JDK-8287867: Bad merge of jdk/test/lib/util/ForceGC.java causing compilation error Changes: https://git.openjdk.java.net/jdk/pull/9043/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9043=00 Issue: https://bugs.openjdk.org/browse/JDK-8287867 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/9043.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9043/head:pull/9043 PR: https://git.openjdk.java.net/jdk/pull/9043
Integrated: JDK-8287671: Adjust ForceGC to invoke System::gc fewer times for negative case
On Fri, 3 Jun 2022 18:05:52 GMT, Mandy Chung wrote: > This reapplies JDK-8287384 and adjust the number of GCs for negative case, > i.e. the condition is never met that is used to verify a reference is not > GC'ed. This pull request has now been integrated. Changeset: 2e332c27 Author: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/2e332c276052554540da0998316a5a99bc350cd6 Stats: 19 lines in 1 file changed: 11 ins; 0 del; 8 mod 8287671: Adjust ForceGC to invoke System::gc fewer times for negative case Reviewed-by: rriggs, bchristi, xuelei - PR: https://git.openjdk.java.net/jdk/pull/9021
Re: RFR: JDK-8287671: Adjust ForceGC to invoke System::gc fewer times for negative case
On Fri, 3 Jun 2022 18:05:52 GMT, Mandy Chung wrote: > This reapplies JDK-8287384 and adjust the number of GCs for negative case, > i.e. the condition is never met that is used to verify a reference is not > GC'ed. JDK-8287384 affects the tests using ForceGC for negative cases only. It will return with a fewer GC cycles of a shortened wait time between each cycle. It will be okay since a strongly reachable reference will never be GC'ed. - PR: https://git.openjdk.java.net/jdk/pull/9021
Re: RFR: 8287810: Reduce runtime of java.lang microbenchmarks
On Fri, 3 Jun 2022 15:05:33 GMT, Claes Redestad wrote: > - Reduce runtime by providing explicit configurations that try to run the > micros as fast as possible while giving enough time for warmup. > - Remove some excessive parameters > - Remove some benchmarks testing experimental features > (ObjectHashCode.mode_) or were simply dubious at large > (StringHttp) > > This reduces runtime of running `java.lang.[A-Z]` (only java.lang, not > java.lang.reflect etc..) from over 56 hours to 6:45. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/9015
Re: RFR: 8287785: Reduce runtime of java.lang.invoke microbenchmarks [v2]
On Fri, 3 Jun 2022 12:53:38 GMT, Claes Redestad wrote: >> - Add explicit run configurations to java.lang.invoke micros, aiming to >> reduce runtime while maintaining a decently high confidence that there's >> enough warmup to produce good enough data. >> >> - Remove several trivial baseline micros, mainly those that only return a >> static object: It's reasonable to have baseline microbenchmarks when the >> baseline op is complex and you're mostly interested in checking the overhead >> of doing the same thing via some MH API, but blackhole operations are now >> shortcutting very quickly and timings doesn't differ from one type of object >> to another, so we don't need a multitude of such baseline tests. >> >> Estimated runtime of `make test TEST=micro:java.lang.micro` (excluding >> build) drops from just above 28 to just above 3 hours. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unused cached field in LookupAcquire Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/9012
Re: RFR: 8287798: Reduce runtime of java.lang.reflect/runtime microbenchmarks
On Fri, 3 Jun 2022 12:46:31 GMT, Claes Redestad wrote: > Add explicit run configurations to java.lang.reflect and .runtime micros, > aiming to reduce runtime while maintaining a decently high confidence that > there's enough warmup to produce good enough data. > > Roughly halves runtime of running `java.lang.reflect|java.lang.runtime` from > 159 to 78 minutes. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/9013
RFR: JDK-8287671: Adjust ForceGC to invoke System::gc fewer times for negative case
This reapplies JDK-8287384 and adjust the number of GCs for negative case, i.e. the condition is never met that is used to verify a reference is not GC'ed. - Commit messages: - JDK-8287671: Adjust ForceGC to invoke System::gc fewer times for negative case Changes: https://git.openjdk.java.net/jdk/pull/9021/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9021=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287671 Stats: 19 lines in 1 file changed: 11 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/9021.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9021/head:pull/9021 PR: https://git.openjdk.java.net/jdk/pull/9021
Integrated: 8287746: ProblemList jni/nullCaller/NullCallerTest.java
On Thu, 2 Jun 2022 18:57:36 GMT, Mandy Chung wrote: > 8287746: ProblemList jni/nullCaller/NullCallerTest.java This pull request has now been integrated. Changeset: 37e1835b Author: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/37e1835be76f5f141ba0dc067578bfe767ca94ed Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8287746: ProblemList jni/nullCaller/NullCallerTest.java Reviewed-by: alanb, dcubed - PR: https://git.openjdk.java.net/jdk/pull/9002
Re: RFR: 8287746: ProblemList jni/nullCaller/NullCallerTest.java
On Thu, 2 Jun 2022 19:12:18 GMT, Daniel D. Daugherty wrote: >> 8287746: ProblemList jni/nullCaller/NullCallerTest.java > > test/jdk/ProblemList.txt line 500: > >> 498: java/lang/invoke/lambda/LambdaFileEncodingSerialization.java8249079 >> linux-x64 >> 499: java/lang/invoke/RicochetTest.java 8251969 >> generic-all >> 500: jni/nullCaller/NullCallerTest.java 8287745 >> generic-all > > Currently only fails on linux-x64, but that's in Tier1 so I don't know if it > will > fail on other configs in higher tiers. I tried to place safe to exclude it on all platforms. - PR: https://git.openjdk.java.net/jdk/pull/9002
RFR: 8287746: ProblemList jni/nullCaller/NullCallerTest.java
8287746: ProblemList jni/nullCaller/NullCallerTest.java - Commit messages: - 8287746: ProblemList jni/nullCaller/NullCallerTest.java Changes: https://git.openjdk.java.net/jdk/pull/9002/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9002=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287746 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/9002.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9002/head:pull/9002 PR: https://git.openjdk.java.net/jdk/pull/9002
Re: RFR: 8287522: StringConcatFactory: Add in prependers and mixers in batches [v4]
On Wed, 1 Jun 2022 13:31:27 GMT, Claes Redestad wrote: >> When generating `MethodHandle`-based concatenation expressions in >> `StringConcatFactory` we can reduce the number of classes generated at >> runtime by creating small batches of prependers and mixers before binding >> them into the root expression tree. >> >> Improvements on one-off tests are modest, while the improvement on >> bootstrapping stress tests can be substantial >> ([MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248)): >> >> | Build |# classes| Runtime | >> | --- | - | --- | >> | Baseline | 31119 | 2.942s | >> | Patch | 16208 | 1.958s | >> >> An earlier version of this patch saw a regression in the >> `StringConcatFactoryBootstraps` microbenchmark. After some refactoring along >> with the optimizations in #8881 and #8923 that is no longer the case, and >> allocation pressure is down slightly compared to the baseline on such a >> repeat-the-same-bootstrap stress test: >> >> Baseline: >> >> Benchmark Mode Cnt >> Score Error Units >> SCFB.makeConcatWithConstants avgt5 >> 2170.039 ? 117.441 ns/op >> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt5 >> 3538.020 ?4.558B/op >> >> This patch: >> >> Benchmark Mode Cnt >> Score Error Units >> SCFB.makeConcatWithConstants avgt5 >> 2144.807 ? 162.685 ns/op >> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt5 >> 3184.943 ?4.495B/op > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments from @ExE-Boss The reduction of the LF classes is promising. This looks okay with me. This PR would rely on testing to catch issues that are not easily caught through code review. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8855
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v2]
On Wed, 1 Jun 2022 21:07:16 GMT, Xue-Lei Andrew Fan wrote: >> This is a follow up update per comments in [JDK-8287384 >> PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in >> open part looks good to me. Please help to run Mach5 just case the closed >> test cases are impacted. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > back to wait 1 second No, it doesn't work. You can build a fastdebug build with `configure --enable-debug`. I reproduce it on macOS. If I restore to the previous version without 8287384, the test passes. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287522: StringConcatFactory: Add in prependers and mixers in batches [v2]
On Mon, 30 May 2022 20:23:40 GMT, Claes Redestad wrote:
>> When generating `MethodHandle`-based concatenation expressions in
>> `StringConcatFactory` we can reduce the number of classes generated at
>> runtime by creating small batches of prependers and mixers before binding
>> them into the root expression tree.
>>
>> Improvements on one-off tests are modest, while the improvement on
>> bootstrapping stress tests can be substantial
>> ([MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248)):
>>
>> | Build |# classes| Runtime |
>> | --- | - | --- |
>> | Baseline | 31119 | 2.942s |
>> | Patch | 16208 | 1.958s |
>>
>> An earlier version of this patch saw a regression in the
>> `StringConcatFactoryBootstraps` microbenchmark. After some refactoring along
>> with the optimizations in #8881 and #8923 that is no longer the case, and
>> allocation pressure is down slightly compared to the baseline on such a
>> repeat-the-same-bootstrap stress test:
>>
>> Baseline:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt5
>> 2170.039 ? 117.441 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt5
>> 3538.020 ?4.558B/op
>>
>> This patch:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt5
>> 2144.807 ? 162.685 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt5
>> 3184.943 ?4.495B/op
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Minor stylistic cleanup
src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 204:
> 202: static BasicType basicType(Class type) {
> 203: if (!type.isPrimitive()) return L_TYPE;
> 204: return
> basicType(Wrapper.forPrimitiveType(type).basicTypeChar());
Suggestion:
return basicType(Wrapper.basicTypeChar(type));
-
PR: https://git.openjdk.java.net/jdk/pull/8855
Re: RFR: 8287522: StringConcatFactory: Add in prependers and mixers in batches [v4]
On Wed, 1 Jun 2022 13:31:27 GMT, Claes Redestad wrote:
>> When generating `MethodHandle`-based concatenation expressions in
>> `StringConcatFactory` we can reduce the number of classes generated at
>> runtime by creating small batches of prependers and mixers before binding
>> them into the root expression tree.
>>
>> Improvements on one-off tests are modest, while the improvement on
>> bootstrapping stress tests can be substantial
>> ([MixedStringCombinations.java](https://gist.github.com/cl4es/08fb581dece3a73e89bfa52337bc4248)):
>>
>> | Build |# classes| Runtime |
>> | --- | - | --- |
>> | Baseline | 31119 | 2.942s |
>> | Patch | 16208 | 1.958s |
>>
>> An earlier version of this patch saw a regression in the
>> `StringConcatFactoryBootstraps` microbenchmark. After some refactoring along
>> with the optimizations in #8881 and #8923 that is no longer the case, and
>> allocation pressure is down slightly compared to the baseline on such a
>> repeat-the-same-bootstrap stress test:
>>
>> Baseline:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt5
>> 2170.039 ? 117.441 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt5
>> 3538.020 ?4.558B/op
>>
>> This patch:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt5
>> 2144.807 ? 162.685 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt5
>> 3184.943 ?4.495B/op
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments from @ExE-Boss
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 459:
> 457: String prefix = constants.get(0);
> 458: if (prefix == null) {
> 459: if (suffix == null) {
Is it possible for prefix or suffix an empty string (non-null)? The original
code checks `if (constant.isEmpty())`
-
PR: https://git.openjdk.java.net/jdk/pull/8855
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC
On Wed, 1 Jun 2022 19:08:03 GMT, Xue-Lei Andrew Fan wrote: > This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. JDK-8287384 causes `test/jdk/java/lang/invoke/defineHiddenClass/UnloadingTest.java` to timeout when running with fastdebug VM. I think this might be caused by more frequent GCs. I tried your patch and the test fails. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287171: Refactor null caller tests to a single directory [v8]
On Wed, 1 Jun 2022 19:27:33 GMT, Tim Prinzing wrote: >> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates >> a test module with some resources in it for the actual tests that occur at >> the native level. The native part was switched to c++ instead of c to make >> it easier to create helper objects that reduce the redundant code performing >> error checking. The two helper classes InstanceCall and StaticCall were >> placed in an include file called CallHelper.hpp. The main part of the tests >> are in exeNullCallerTest.cpp, and there is a separate function for each of >> the bug reports. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > comment changes Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8934
Re: RFR: 8287171: Refactor null caller tests to a single directory [v6]
On Wed, 1 Jun 2022 03:01:35 GMT, Tim Prinzing wrote: >> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates >> a test module with some resources in it for the actual tests that occur at >> the native level. The native part was switched to c++ instead of c to make >> it easier to create helper objects that reduce the redundant code performing >> error checking. The two helper classes InstanceCall and StaticCall were >> placed in an include file called CallHelper.hpp. The main part of the tests >> are in exeNullCallerTest.cpp, and there is a separate function for each of >> the bug reports. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > move jni/nullCaller tests to jdk_lang group test/jdk/jni/nullCaller/exeNullCallerTest.cpp line 27: > 25: > 26: /* Test for JDK-8280902 > 27: * ResourceBundle::getBundle may throw NPE when invoked by JNI code with > no caller frame This comment isn't correct. `ResourceBundle::getBundle` defaults to the system class loader if invoked by a null caller and I don't expect it may throw NPE. Looks like you copy the summary of the JBS issue here, which isn't helpful to the readers.I expect it a description of the test case. Same comments to all other test cases. - PR: https://git.openjdk.java.net/jdk/pull/8934
Re: RFR: 8287171: Refactor null caller tests to a single directory [v4]
On Wed, 1 Jun 2022 00:59:33 GMT, Tim Prinzing wrote: >> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates >> a test module with some resources in it for the actual tests that occur at >> the native level. The native part was switched to c++ instead of c to make >> it easier to create helper objects that reduce the redundant code performing >> error checking. The two helper classes InstanceCall and StaticCall were >> placed in an include file called CallHelper.hpp. The main part of the tests >> are in exeNullCallerTest.cpp, and there is a separate function for each of >> the bug reports. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > Moved the tests test/jdk/jni/nullCaller. Added to the jdk_other group > which runs as part of tier2. Made the other requested changes such as > not using the bugid as the function name for the test and using the > name of the main method being tested instead. jdk_other is for other modules. jdk_lang may be an option since this is to test when attached with JNI thread. - PR: https://git.openjdk.java.net/jdk/pull/8934
Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v2]
On Tue, 31 May 2022 15:39:39 GMT, Alan Bateman wrote: >> This patch adds an alternative virtual thread implementation where each >> virtual thread is backed by an OS thread. It doesn't scale but it can be >> used by ports that don't have continuations support in the VM. Aside from >> scalability, the lack of continuations support means: >> >> 1. JVM TI is not supported when running with --enable-preview (the JVM TI >> spec allows for this) >> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs >> and so needs JVM TI) >> >> The VM option "VMContinuations" is added as an experimental option so it can >> be used by tests. A number of tests are changed to re-run with >> -XX:-VMContinuations. A new jtreg property is added so that tests that need >> the underlying VM support to be present can use "@requires vm.continuations" >> in the test description. A follow-up change would be to add "@requires >> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with >> preview features enabled. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Allowing linking without intrinsic stub, contributed-by: rehn Looks okay to me. src/java.management/share/classes/sun/management/ThreadImpl.java line 586: > 584: > 585: /** > 586: * Returns the ThreadInfo objects from the given array that > coresspond to platform typo: coresspond -> correspond src/java.management/share/classes/sun/management/Util.java line 92: > 90: > 91: /** > 92: * Returns true if the given Thread is a virutal thread. typo: virutal -> virtual this typo appears in other comments too. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8939
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v6]
On Thu, 26 May 2022 23:20:27 GMT, liach wrote: >> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, >> the interfaces are iterated twice. The two passes can be merged into one, >> yielding the whole proxy definition context (module, package, whether >> there's package-private interface) when determining the module. >> >> Split from #8278. Helpful for moving proxies to hidden classes, but is a >> good cleanup on its own. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Fixes suggested by mandy Looks good. Thanks. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8287171: Refactor null caller tests to a single directory [v3]
On Mon, 30 May 2022 00:10:50 GMT, Tim Prinzing wrote:
>> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates
>> a test module with some resources in it for the actual tests that occur at
>> the native level. The native part was switched to c++ instead of c to make
>> it easier to create helper objects that reduce the redundant code performing
>> error checking. The two helper classes InstanceCall and StaticCall were
>> placed in an include file called CallHelper.hpp. The main part of the tests
>> are in exeNullCallerTest.cpp, and there is a separate function for each of
>> the bug reports.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> problem with iostream on Windows, use printf
test/jdk/jdk/nullCaller/CallHelper.hpp line 176:
> 174: }
> 175:
> 176: // call an method returning an object checking for exceptions and
`s/an method/a method/`
test/jdk/jdk/nullCaller/exeNullCallerTest.cpp line 29:
> 27: * ResourceBundle::getBundle may throw NPE when invoked by JNI code with
> no caller frame
> 28: */
> 29: void JDK_8280902(JNIEnv* env) {
A descriptive method name would be more helpful than the bug ID, for example,
`getResourceBundle` and `registerClassLoaderAsParallelCapable`
test/jdk/jdk/nullCaller/exeNullCallerTest.cpp line 49:
> 47:
> 48: // check the message
> 49: if (std::string("Hello!") != env->GetStringUTFChars(msg,NULL)) {
nit: a space after the comma `(msg, NULL)` consistent with the format in this
local file.
-
PR: https://git.openjdk.java.net/jdk/pull/8934
Re: RFR: 8287171: Refactor null caller tests to a single directory [v3]
On Mon, 30 May 2022 05:37:16 GMT, Alan Bateman wrote: > I don't think jdk/nullCaller is right location for this. Maybe jni/nullCaller > could work. You'll probably need to add the location to an appropriate > group/tier in test/jdk/TEST.groups, otherwise it won't be run. I also think `test/jdk/jni/nullCaller` is better. - PR: https://git.openjdk.java.net/jdk/pull/8934
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The ForceGC could be enhanced by using smaller wait/sleep time, and shared > cleaner. > > Thanks, > Xuelei Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 21:40:42 GMT, Xue-Lei Andrew Fan wrote: > Even using a Cleaner is a more overhead than necessary. > I would have skipped the overhead of a cleaner and Atomic classes with > something more self contained as a static method: Hmm... one benefit of Cleaner is the ease of use to avoid the need of managing the reference queue. If the performance of the Cleaner API is a concern, perhaps we should look into reducing its overhead? - PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Tue, 31 May 2022 13:26:17 GMT, Daniel Fuchs wrote:
>> Hi,
>>
>> May I have this test update reviewed?
>>
>> The ForceGC could be enhanced by using smaller wait/sleep time, and shared
>> cleaner.
>>
>> Thanks,
>> Xuelei
>
> test/lib/jdk/test/lib/util/ForceGC.java line 50:
>
>> 48: for (int i = 0; i < 100; i++) {
>> 49: System.gc();
>> 50: System.out.println("doIt() iter: " + iter + ", gc " + i);
>
> Maybe the trace should be printed only if `(i % 10 == 0) && (iter % 100 ==
> 0)` to avoid having too many traces in the log?
This log was added to help debugging some past issues. With this change, the
volume of this trace would not be that helpful. I think we can remove this
log at this time.
-
PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/3d558a34...05cf2d8b I think `Modifier` API is as good as it is right now. The question is whether we keep `Modifier` API as is for the existing usage or enhancing it for the new modifiers such as `value` and `primitive`. No benefit of replacing existing uses of `Modifier` to the new `AccessFlag` API as most existing use are simple tests to determine if it's public, static, final, non-public etc. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/43363358...05cf2d8b With the `AccessFlag` API, what is the role of the `Modifier` API going forward? [Value Objects JEP](https://openjdk.java.net/jeps/8277163) defines the new `identity` and `value` modifiers. [PR #698](https://github.com/openjdk/valhalla/pull/698) proposes to add `Modifier.IDENTITY` and `Modifier.VALUE` constants as the `identity` and `value` modifiers are encoded in a class file using the `ACC_IDENTITY` and `ACC_VALUE` flags. However, with the new improved `AccessFlag` API, the new flags will be defined in the `AccessFlag` API. I think we should avoid adding the new flags in `Modifier` and leave it for the existing usage. Use `AccessFlag` for new features. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy 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 32 additional commits since > the last revision: > > - Target JDK 20 rather than 19. > - Merge branch 'master' into JDK-8266670 > - Add mask values to constants' javadoc. > - Implement review feedback from mlchung. > - Fix type in @throws tag. > - Merge branch 'master' into JDK-8266670 > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - ... and 22 more: > https://git.openjdk.java.net/jdk/compare/2ff12e53...05cf2d8b I have the same view as Joe. `ACC_SUPER` has been specified in JVMS and set in class files of an older version. If `ACC_SUPER` would be removed from JVMS before this API becomes final, the `AccessFlag` API would be revised and not to define it at that time. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v5]
On Fri, 27 May 2022 00:16:00 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Doesn't hurt to be more specific Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v4]
On Thu, 26 May 2022 20:52:43 GMT, liach wrote:
>> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of
>> `Class.forName(String)`. `make test
>> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new
>> `LazyInitializationTest` failing the eager initialization check on the
>> baseline and passing with this patch.
>>
>> On a side note, this might reduce the number of methods that can be encoded
>> in a proxy due to code attribute size restrictions; we probably would
>> address that in another issue, as we never mandated a count of methods that
>> the proxy must be able to implement.
>>
>> Mandy, would you mind review this?
>
> liach 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 eight additional commits since
> the last revision:
>
> - Merge branch 'master' into proxy-class-forname
> - Move the try catch block as it doesn't throw checked exceptions
> - remove unused field
> - whitespace
> - Copyright year
> - typo
> - 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid
> unnecessary class initialization
> - Test for eager initialization
Thanks for doing this. It looks okay. JDK-7194006, having a variant of
`Class::forName` that loads the named class with the class loader of the
current class, would simplify this. This can be updated when such an API is
defined in the future.
test/jdk/java/lang/reflect/Proxy/LazyInitializationTest.java line 53:
> 51: new Class[]{ Intf.class },
> 52: (proxy, method, args) -> null);
> 53: Assert.assertFalse(initialized, "parameter type initialized
> eagerly");
This expects "parameter type initialized eagerly" to be false. This may cause
confusion to the reader. Maybe just simply "initialized expected: false" and a
comment would help too. Similarly for line 56.
test/jdk/java/lang/reflect/Proxy/LazyInitializationTest.java line 53:
> 51: new Class[]{ Intf.class },
> 52: (proxy, method, args) -> null);
> 53: Assert.assertFalse(initialized, "parameter type initialized
> eagerly");
This expects "parameter type initialized eagerly" to be false. This may cause
confusion to the reader. Maybe just simply "initialized expected: false" and a
comment would help too. Similarly for line 56.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8800
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v4]
On Thu, 26 May 2022 23:42:27 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Make primitive type info more reader friendly src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 979: > 977: unwrapMethodDesc = "()" + baseTypeString; > 978: this.loadOpcode = loadOpcode; > 979: this.returnOpcode = loadOpcode - ILOAD + IRETURN; This could do it. It would be more explicit to take the return opcode as an argument to the constructor. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 23:35:10 GMT, liach wrote: >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: >> >>> 967: // single-char BaseType descriptor (see JVMS section 4.3.2) >>> 968: String baseTypeString = wrapper.basicTypeString(); >>> 969: wrapperClassName = >>> dotToSlash(wrapper.wrapperType().getName()); >> >> Suggestion: >> >> wrapperClassName = wrapper.wrapperType().descriptorString(); >> >> >> It may worth to replace similar use of `dotToSlash(c.getName())` pattern. > > Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor > (`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor > construction. ah, you're right. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 20:55:53 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach has updated the pull request with a new target base due to a merge or a > rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains three additional commits since > the last revision: > > - Merge branch 'master' into proxy-primitivetypeinfo > - Convert PrimitiveTypeInfo to an enum > - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo Have you considered including the opcode for load and return instruction instead of `opcodeOffset`? src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: > 967: // single-char BaseType descriptor (see JVMS section 4.3.2) > 968: String baseTypeString = wrapper.basicTypeString(); > 969: wrapperClassName = > dotToSlash(wrapper.wrapperType().getName()); Suggestion: wrapperClassName = wrapper.wrapperType().descriptorString(); It may worth to replace similar use of `dotToSlash(c.getName())` pattern. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 986: > 984: if (cl == float.class) return FLOAT; > 985: if (cl == double.class) return DOUBLE; > 986: throw new AssertionError(); Suggestion: throw new AssertionError(cl); - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 22:01:56 GMT, liach wrote:
>> src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
>>
>>> 511:
>>> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
>>> 513: // Required for default method invocation
>>
>> Is this comment true?
>>
>> The module of the proxy class opens the package to `java.base` if the proxy
>> interface is non-public in a named module or if all proxy interfaces are
>> public but a proxy interface is not unconditionally exported.
>
> The original check and `Modules.addOpen` calls were added in
> [8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the
> `invokeDefault` support was added.
>
> See:
> https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667,
> or #313
Ah, I see. That assert was added in PR #313 as a clarification to the current
proxy implementation but not required by the `InvocationHandle::invokeDefault`.
It could have been added before the default method support.
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 20:53:29 GMT, liach wrote:
>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass,
>> the interfaces are iterated twice. The two passes can be merged into one,
>> yielding the whole proxy definition context (module, package, whether
>> there's package-private interface) when determining the module.
>>
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a
>> good cleanup on its own.
>
> liach 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:
>
> - Group proxy location validation all into the class context constructor
> - Merge branch 'master' into fix/proxy-single-pass
> - Update
> - Updates suggested by mandy
> - Merge branch 'master' into fix/proxy-single-pass
> - Don't need to complexify module cache
> - 8284942: Proxy building can just iterate superinterfaces once
src/java.base/share/classes/java/lang/reflect/Proxy.java line 498:
> 496: new ClassLoaderValue<>();
> 497:
> 498: private record ProxyClassContext(Module module, String pkg, int
> accessFlags) {
Suggestion:
private record ProxyClassContext(Module module, String packageName, int
accessFlags) {
src/java.base/share/classes/java/lang/reflect/Proxy.java line 499:
> 497:
> 498: private record ProxyClassContext(Module module, String pkg, int
> accessFlags) {
> 499: private ProxyClassContext {
We should validate the `accessFlags` value as it must be 0 or `PUBLIC`.
src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
> 511:
> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
> 513: // Required for default method invocation
Is this comment true?
The module of the proxy class opens the package to `java.base` if the proxy
interface is non-public in a named module or if all proxy interfaces are public
but a proxy interface is not unconditionally exported.
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently [v3]
On Wed, 25 May 2022 14:13:52 GMT, Claes Redestad wrote:
>> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey`
>> allows keys to be compacted when all byte values of the key fit in 4 bits,
>> otherwise a byte array is allocated and used. This means that all transforms
>> with a kind value above 15 will be forced to allocate and use array
>> comparisons.
>>
>> Removing unused and folding some transforms to ensure all existing kinds can
>> fit snugly within the 0-15 value range realize a minor improvement to
>> footprint, speed and allocation pressure of affected transforms, e.g.
>> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
>>
>> Baseline:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt 15
>> 2048.475 ? 69.887 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt 15
>> 3487.311 ? 80.385B/op
>>
>>
>> Patched:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt 15
>> 1961.985 ? 101.519 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt 15
>> 3156.478 ? 183.600B/op
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Missed correctly taking b1 into account in of(byte, int, int...)
> (java/lang/String/concat/ImplicitStringConcatShapes.java test failure)
LGTM with a couple of suggestions.
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 219:
> 217: return new TransformKey(fullBytes);
> 218: }
> 219: static TransformKey of(byte kind, int b1, int b2, int[] b3456) {
Nit: I can't figure out if `b3456` intends to be a different convention than
the others `b123` and `b234`. I would expect something like this:
Suggestion:
static TransformKey of(byte kind, int b1, int b2, int... b345) {
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 252:
> 250: if (!inRange(bitset))
> 251: return 0;
> 252: pb = pb | b2 << (2 * PACKED_BYTE_SIZE) | b1 <<
> PACKED_BYTE_SIZE | b0;
Suggestion:
pb = pb | packagedBytes(b0, b1, b2);
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 267:
> 265: if (!inRange(bitset))
> 266: return 0;
> 267: pb = pb | b1 << PACKED_BYTE_SIZE | b0;
Suggestion:
pb = pb | packagedBytes(b0, b1);
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8881
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v4]
On Fri, 20 May 2022 02:25:32 GMT, liach wrote: >> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, >> the interfaces are iterated twice. The two passes can be merged into one, >> yielding the whole proxy definition context (module, package, whether >> there's package-private interface) when determining the module. >> >> Split from #8278. Helpful for moving proxies to hidden classes, but is a >> good cleanup on its own. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Update src/java.base/share/classes/java/lang/reflect/Proxy.java line 514: > 512: // Per JLS 7.4.2, unnamed package can only exist in > unnamed modules. > 513: // This means a package-private superinterface exist in > the unnamed > 514: // package of a named module. With this patch, I think line 505-517 can turn into assert or internal error as you suggested. Probably can be moved to the `ProxyClassContext` constructor too. - PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v2]
On Tue, 17 May 2022 20:16:48 GMT, Tim Prinzing wrote: >> The Class::forName behavior change to match JNI FindClass is a compatible >> change and seems pretty attractive as it would be expected that >> Class::forName would give the same behavior as FindClass which uses the >> system classloader. The test for 8281006 was enhanced to test for this >> change. Merged master to pick up fixes to unrelated test failures to reduce >> noise. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > Added javadoc comment src/java.base/share/classes/java/lang/Class.java line 361: > 359: * > 360: * > 361: * This method is caller sensitive. In cases where this method is > called You can drop "This method is caller sensitive." sentence for consistency with other caller-sensitive methods that do not state that explicitly. The javadoc already specifies that it uses the class loader of the current class. - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: 8287206: Use WrongThreadException for confinement errors
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore wrote: > This patch tweaks the foreign API to use the newly added > `WrongThreadException` instead of `IllegalStateException` to report > confinement errors. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8865
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v3]
On Thu, 19 May 2022 23:44:12 GMT, liach wrote:
>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass,
>> the interfaces are iterated twice. The two passes can be merged into one,
>> yielding the whole proxy definition context (module, package, whether
>> there's package-private interface) when determining the module.
>>
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a
>> good cleanup on its own.
>
> liach has updated the pull request with a new target base due to a merge or a
> rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains four additional commits since the
> last revision:
>
> - Updates suggested by mandy
> - Merge branch 'master' into fix/proxy-single-pass
> - Don't need to complexify module cache
> - 8284942: Proxy building can just iterate superinterfaces once
Looks good with a couple comments.
src/java.base/share/classes/java/lang/reflect/Proxy.java line 509:
> 507: if (!m.isNamed())
> 508: throw new InternalError("unnamed module: " + m);
> 509: } else if (proxyPkg.isEmpty() && m.isNamed()) {
With the refactoring, it may be easier to understand to make this check for
both public and non-public access (i.e. drop the `else`) for clarity - a named
module can't have unnamed package.
src/java.base/share/classes/java/lang/reflect/Proxy.java line 534:
> 532: * Generate the specified proxy class.
> 533: */
> 534: accessFlags |= Modifier.FINAL;
It can be inlined in the call to `generateProxyClass`:
byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader,
proxyName, interfaces, accessFlags | Modifier.FINAL);
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v2]
On Thu, 21 Apr 2022 03:44:18 GMT, liach wrote:
>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass,
>> the interfaces are iterated twice. The two passes can be merged into one,
>> yielding the whole proxy definition context (module, package, whether
>> there's package-private interface) when determining the module.
>>
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a
>> good cleanup on its own.
>
> liach has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Don't need to complexify module cache
src/java.base/share/classes/java/lang/reflect/Proxy.java line 498:
> 496: new ClassLoaderValue<>();
> 497:
> 498: private record ProxyContext(Module module, String pkg, boolean
> packagePrivate) {}
This represents the context for a proxy class. It seems `ProxyClassContext`
would be clearer. I suggest the context to have the accessFlags for a proxy
class rather than the boolean whether it's package private. The constructor
should check that it must be 0 or `Modifier.PUBLIC`. `FINAL` will be added to
the access flags when it generates the proxy class.
src/java.base/share/classes/java/lang/reflect/Proxy.java line 766:
> 764: * Reads edge and qualified exports are added for dynamic module
> to access.
> 765: */
> 766: private static ProxyContext setupContext(ClassLoader loader,
Perhaps name this method `proxyClassContext` that returns `ProxyClassContext`.
src/java.base/share/classes/java/lang/reflect/Proxy.java line 831:
> 829: // All proxy interfaces are public. So maps to a dynamic
> proxy module
> 830: // and add reads edge and qualified exports, if necessary
> 831: var context = getDynamicModuleContext(loader, nonExported);
I suggest to keep the `getDynamicModule` method as creating a dynamic module of
a given class loader is a clear function. The context can be created in this
method body.
var targetModule = getDynamicModule(loader);
var pkgName = nonExported ? PROXY_PACKAGE_PREFIX + '.' +
targetModule.getName()
: targetModule.getName();
var context = new ProxyClassContext(targetModule, pkgName,
Modifier.PUBLIC);
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8286858: Remove dead code in sun.reflect.misc.MethodUtil
On Sun, 15 May 2022 12:47:10 GMT, Andrey Turbanov wrote: > They are unused and leftover since JDK 7. It's good to remove them. Looks good. Do you know why there are test failures (they look unrelated)? - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8715
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
On Tue, 17 May 2022 16:55:14 GMT, Tim Prinzing wrote: > I like the idea of moving all the null caller tests to a clearly named > directory to avoid duplication. +1. You can do this refactoring in a separate JBS issue and then update this PR when the tests are moved. - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null
On Sat, 14 May 2022 00:30:00 GMT, Tim Prinzing wrote: > The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. `Class::forName(String)` javadoc should specify which class loader to use when invoked with null caller similar to the other APIs you fixed for null callers. I think this new test case does not belong to `NullCallerGetResource.java` which is a test for `Module::getResource`.It's better to be placed under the `test/jdk/java/lang/Class/forName` directory that makes it clear it's a test for `Class::forName`. Alternatively, we could move all the tests for null caller under a new clearly-named directory, maybe `test/jdk/jdk/nullCaller`.This may allow to do some refactoring and remove the duplicated code in these test cases. What do you think? src/java.base/share/classes/java/lang/Class.java line 385: > 383: ClassLoader loader = (caller == null) ? > 384: ClassLoader.getSystemClassLoader() : > 385: ClassLoader.getClassLoader(caller); Formatting nit: ClassLoader loader = (caller == null) ? ClassLoader.getSystemClassLoader() : ClassLoader.getClassLoader(caller); test/jdk/java/lang/module/exeNullCallerGetResource/NullCallerGetResource.java line 28: > 26: * @test > 27: * @bug 8281006 > 28: * @bug 8281001 Suggestion: * @bug 8281006 8281001 `@bug` can have one or more bug IDs - PR: https://git.openjdk.java.net/jdk/pull/8711
Re: Unused method sun.reflect.misc.MethodUtil.getPublicMethods
They are unused and leftover since JDK 7. It's good to remove them. Thanks Mandy On 4/30/22 7:48 AM, Andrey Turbanov wrote: Hello. I found a few methods in MethodUtil class which seem unused to me: getPublicMethods, getInterfaceMethods, getInternalPublicMethods ,addMethod. Are they somehow used by VM, or is it just leftovers from some refactorings? I wonder if we can drop them. Andrey Turbanov
Re: RFR: 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure [v2]
On Wed, 4 May 2022 09:46:00 GMT, Сергей Цыпанов wrote: >> `Class.getInterfaces(false)` does not clone underlying array and can be used >> in cases when the returned array is only read from. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282701: Revert some irrelevant changes Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7714
Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]
On Tue, 10 May 2022 14:58:15 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvement eliminates >> some of the redundant `--enable-preview` clauses from the Sealed Classes >> tests, since Sealed Classes have been graduated from preview in JDK 17. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8627
Re: RFR: 8286473: Drop --enable-preview from Record related tests [v2]
On Tue, 10 May 2022 14:54:39 GMT, Aleksey Shipilev wrote: >> There are plenty of tests failing on many architectures due to >> `--enable-preview` VM code introduced by Loom. This improvements eliminates >> some of the redundant `--enable-preview` clauses from the Record tests, >> since Records have been graduated from preview in JDK 16. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, affected tests still pass >> - [x] Linux x86_32 fastdebug, affected tests start to pass > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Review comments Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8626
Re: RFR: 8286298: Remove unused methods in sun.invoke.util.VerifyType [v2]
On Mon, 9 May 2022 10:12:36 GMT, Claes Redestad wrote: >> A few untested and unused methods in `VerifyType` which can be removed. >> (Possibly used by native JSR 292 implementations in JDK 7). > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Inline isNullReferenceConversion at sole call site and remove it as well Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8570
Re: RFR: JDK-8286347: incorrect use of `{@link}`
On Fri, 6 May 2022 23:30:44 GMT, Jonathan Gibbons wrote:
> Please review a small doc fix to update incorrect use of `{@link}` on arrays
> of primitive types.
Marked as reviewed by mchung (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8584
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v41]
On Fri, 6 May 2022 16:48:11 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
On Thu, 5 May 2022 20:54:53 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI > * Tweak restricted method check to work when there's no caller class src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 161: > 159: ClassLoader.getSystemClassLoader(); > 160: MemorySession loaderSession = (loader == null) ? > 161: MemorySession.global() : // boot loader never goes away The other built-in class loaders (`ClassLoaders::appClassLoader` and `ClassLoaders::platformClassLoader` are both instance of `BuiltinClassLoader`) will never be unloaded. Should they use the globel memory session? src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 120: > 118: // if there is no caller class, act as if the call came from an > unnamed module > 119: Module module = currentClass != null ? > 120: currentClass.getModule() : Holder.FALLBACK_MODULE; This can be simplified to: Module module = currentClass != null ? currentClass.getModule() : ClassLoader::getSystemClassLoader().getUnnamedModule(); No need to have the Holder class. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Thu, 5 May 2022 16:22:41 GMT, Mandy Chung wrote: >> Looking deeper, `System::loadLibrary` seems to search the boot loader >> libraries when invoked with a `null` caller class, so replicating that >> behavior should be safe. > > `BootLoader` is what you want in this case - it defines the static methods to > access resources, packages etc defined to the boot loader (aka null > `ClassLoader`). > > To find a symbol defined in the native libraries loaded by the boot loader, > you can call `BootLoader.getNativeLibraries().find(name)`. When a caller-sensitive method is invoked from a thread attached via JNI, the caller class returned from `Reflection::getCallerClass` is `null`.I would recommend the default to be a class in the unnamed module defined to the system class loader; hence it defaults to the system class loader. This is consistent with JNI `FindClass` when invoked with no caller frame. It's an open issue [1] to revisit the default for `System::load` and `System::loadLibrary` when invoked with null caller class. However, the existing behavior may likely be unchanged because of the compatibility risk. [1] https://bugs.openjdk.java.net/browse/JDK-8281001 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:44:08 GMT, Maurizio Cimadamore wrote: >> Another option would be to treat calls to `ensureNativeAccess` with `null` >> caller class as coming from unnamed module. > > Looking deeper, `System::loadLibrary` seems to search the boot loader > libraries when invoked with a `null` caller class, so replicating that > behavior should be safe. `BootLoader` is what you want in this case - it defines the static methods to access resources, packages etc defined to the boot loader (aka null `ClassLoader`). To find a symbol defined in the native libraries loaded by the boot loader, you can call `BootLoader.getNativeLibraries().find(name)`. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure
On Sat, 5 Mar 2022 13:07:56 GMT, Сергей Цыпанов wrote: > `Class.getInterfaces(false)` does not clone underlying array and can be used > in cases when the returned array is only read from. For the `checkPackageAccess` case, I don't think it worths fixing; not only that security manager is deprecated for removal but also when security manager is enabled, there are lots of allocations for other security checks. So the `getInterface(boolean)` method can be kept private. - PR: https://git.openjdk.java.net/jdk/pull/7714
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote:
>> This is an early review of changes to better model JVM access flags, that is
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>>
>> Language level modifiers and JVM level access flags are closely related, but
>> distinct. There are concepts that overlap in the two domains (public,
>> private, etc.), others that only have a language-level modifier (sealed),
>> and still others that only have an access flag (synthetic).
>>
>> The existing java.lang.reflect.Modifier class is inadequate to model these
>> subtleties. For example, the bit positions used by access flags on different
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for
>> methods. Just having a raw integer does not provide sufficient context to
>> decode the corresponding language-level string. Methods like
>> Modifier.methodModifiers() were introduced to cope with this situation.
>>
>> With additional modifiers and flags on the horizon with projects like
>> Valhalla, addressing the existent modeling deficiency now ahead of time is
>> reasonable before further strain is introduced.
>>
>> This PR in its current form is meant to give the overall shape of the API.
>> It is missing implementations to map from, say, method modifiers to access
>> flags, taking into account overlaps in bit positions.
>>
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in
>> once the API is further along.
>
> Joe Darcy 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 26 additional commits since
> the last revision:
>
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - Typo fix; add implSpec to Executable.
> - Appease jcheck.
> - Fix some bugs found by inspection, docs cleanup.
> - Merge branch 'master' into JDK-8266670
> - Initial support for accessFlags methods
> - Add mask to access flag functionality.
> - ... and 16 more:
> https://git.openjdk.java.net/jdk/compare/7ac698ba...14980605
I took a closer look at the proposed APIs. I think it's in a good state to
target for 19. I skimmed on the existing JDK usage of `getModifiers` other
than a trivial test e.g. is public, final, etc and the proposed API works well
(btw no plan to convert them and just use those cases for validation).
The value of `ACC_SUPER` and `ACC_STRICT` could possibly be reused for new
access flags. It may be useful to document when the flag becomes obsolete.
Nit: the enum constants are listed in the order of the mask value, which I
like. Some enum constants reference the `Modifer` constants but I think it'd
help to see the mask value here consistently for all entries. One go-to place
in the source if I want to find the value of different flags.
src/java.base/share/classes/java/lang/Class.java line 1328:
> 1326: * @since 19
> 1327: */
> 1328: public Set accessFlags() {
The access flags of a hidden class has no difference than that of a normal
class. A hidden class is created with normal `ClassFile` except that it's
created via `Lookup::defineHiddenClass` API.
I think the `Class::accessFlags` method should specify the access flags for
primitive type, void, and array classes as `Class::getModifiers` specified.
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 216:
> 214:
> 215: /**
> 216: * {@return an unmodifiable set of the module {@linkplain
> AccessFlag
Suggestion:
* {@return a possibly-empty unmodifiable set of the module {@linkplain
AccessFlag
as specified in the `@return` of the `modifiers()` method. Same comment
applies to the `accessFlags` method in `ModuleDescriptor.Opens` and other
classes.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 44:
> 42: * not have an access flag, such as {@code sealed} (JVMS
> 43: * {@jvms 4.7.31}) and some access flags have no corresponding
> 44: * modifier, such as {@linkplain SYNTHETIC synthetic}.
Suggestion:
* modifier, such as {@linkplain #SYNTHETIC synthetic}.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 186:
> 184: /**
> 185: * The access flag {@code ACC_ABSTRACT}, corresponding to the
> 186: * source modifier {@code link Modifier#ABSTRACT abstract}.
Suggestion:
* source modifier {@link Modifier#ABSTRACT abstract}.
src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 306:
> 304: * Note that since these locations represent class file structures
> 305: * rather than language structures many language structures, such
> 306: * as constructors and interfaces, are not present.
missing `@since 19`
-
PR:
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 18:24:33 GMT, Andrey Turbanov wrote:
>> Joe Darcy 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:
>>
>> - Update copyright years.
>> - Merge branch 'master' into JDK-8285676
>> - Respond to more review feedback.
>> - Respond to more review feedback.
>> - Respond to review feedback.
>> - Merge branch 'master' into JDK-8285676
>> - JDK-8285676: Add missing @param tags for type parameters on classes and
>> interfaces
>
> src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
>
>> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
>> 47: * whether some object is the referent of a phantom reference.
>> 48: * @param the type of the referent
>
> Shouldn't there be a space after `@param` ?
Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
On Thu, 28 Apr 2022 16:58:40 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e I reviewed most of the source files in `java.base` except `java.util.concurrent`. I also reviewed `java.logging`, `java.management` and `jdk.management` modules. The changes are easy to follow and clean. Looks good to me. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
On Wed, 27 Apr 2022 18:19:57 GMT, Kim Barrett wrote:
>> test/jdk/java/lang/ref/ReferenceClone.java line 52:
>>
>>> 50: } catch (CloneNotSupportedException e) {
>>> 51: throw new RuntimeException("CloneableReference::clone
>>> should not throw CloneNotSupportedException");
>>> 52: }
>>
>> Alternatively, it could simply let CNSE propagate.
>>
>>
>> CloneableReference ref = new CloneableReference(o);
>> ref.clone();
>>
>>
>> `test()` and `main` will need to declare this checked exception.
>
> That was my initial thought, but it doesn't work - CNSE is a checked
> exception so must be handled.
> test() and main will need to declare this checked exception.
diff --git a/test/jdk/java/lang/ref/ReferenceClone.java
b/test/jdk/java/lang/ref/ReferenceClone.java
index bd1ead81bec..2f9386b81e4 100644
--- a/test/jdk/java/lang/ref/ReferenceClone.java
+++ b/test/jdk/java/lang/ref/ReferenceClone.java
@@ -31,12 +31,12 @@ import java.lang.ref.*;
public class ReferenceClone {
private static final ReferenceQueue QUEUE = new ReferenceQueue<>();
-public static void main(String... args) {
+public static void main(String... args) throws Exception {
ReferenceClone refClone = new ReferenceClone();
refClone.test();
}
-public void test() {
+public void test() throws CloneNotSupportedException {
// test Reference::clone that throws CNSE
Object o = new Object();
assertCloneNotSupported(new SoftRef(o));
@@ -45,9 +45,7 @@ public class ReferenceClone {
// Reference subclass may override the clone method
CloneableReference ref = new CloneableReference(o);
-try {
ref.clone();
-} catch (CloneNotSupportedException e) {}
}
private void assertCloneNotSupported(CloneableRef ref) {
```
-
PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 18:13:15 GMT, Paul Sandoz wrote: >> src/java.base/share/classes/java/lang/PinnedThreadPrinter.java line 58: >> >>> 56: >>> 57: // maps a class to the hashes of stack traces pinned by that code >>> in that class >>> 58: private static final Map, Hashes> classToHashes = new >>> WeakHashMap<>(); >> >> Can you use `ClassValue` in this case? > > I was wondering that too, but held off commenting since it's not super > performance critical given `classToHashes` is synchronized on. However, it > does make the code a little clearer. It's not critical and no problem if this is cleaned up as a follow-up. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
src/java.management/share/classes/sun/management/ThreadImpl.java line 447:
> 445: if (threads != null) {
> 446: long[] tids = Stream.of(threads)
> 447: .filter(t -> !(t instanceof
> jdk.internal.misc.CarrierThread))
Returning an array of thread IDs of just the platform threads is good. The
javadoc needs to be updated to say "Returns an array of thread identifiers for
the platform threads"
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java line 122: > 120: */ > 121: > 122: public static final int SCOPED_CACHE_SHIFT; I can't find this constant being used. If added for future, maybe keep `UnsafeConstants` class and this static field package-private for now. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e src/java.base/share/classes/java/lang/PinnedThreadPrinter.java line 58: > 56: > 57: // maps a class to the hashes of stack traces pinned by that code in > that class > 58: private static final Map, Hashes> classToHashes = new > WeakHashMap<>(); Can you use `ClassValue` in this case? - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285690: CloneableReference subtest should not throw CloneNotSupportedException [v2]
On Wed, 27 Apr 2022 10:11:15 GMT, Kim Barrett wrote:
>> Please review this fix to test/jdk/java/lang/ref/ReferenceClone.java. It was
>> passing if CloneableReference::clone were to throw
>> CloneNotSupportedException.
>> That should be a failure.
>>
>> Testing:
>> Locally (linux-x64) verified test still passes. Temporarily changed
>> CloneableReference::clone to throw and verified the expected failure gets
>> reported, unlike before.
>
> Kim Barrett has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyright, @bug list
Marked as reviewed by mchung (Reviewer).
test/jdk/java/lang/ref/ReferenceClone.java line 52:
> 50: } catch (CloneNotSupportedException e) {
> 51: throw new RuntimeException("CloneableReference::clone should
> not throw CloneNotSupportedException");
> 52: }
Alternatively, it could simply let CNSE propagate.
CloneableReference ref = new CloneableReference(o);
ref.clone();
`test()` and `main` will need to declare this checked exception.
-
PR: https://git.openjdk.java.net/jdk/pull/8418
Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v5]
On Wed, 20 Apr 2022 01:03:23 GMT, Tim Prinzing wrote:
>> Created a test called NullCallerGetResource to test
>> Module::getResourceAsStream and Class::getResourceAsStream from the native
>> level.
>>
>> At the java level the test builds a test module called 'n' which opens the
>> package 'open' to everyone. There is also a package 'closed' which is
>> neither opened or exported. Both packages have a text file called
>> 'test.txt'. The open package has a class called OpenResources and the closed
>> package has a class called ClosedResources. The native test is launched with
>> the test module n added.
>>
>> At the native level the test tries to read both the open and closed resource
>> from both the classes and the module. It performs the equivalent of the
>> following java operations:
>>
>> Class c = open.OpenResources.fetchClass();
>> InputStream in = c.getResourceAsStream("test.txt");
>> assert(in != null); in.close();
>>
>> Module n = c.getModule();
>> in = n.getResourceAsStream("open/test.txt");
>> assert(in != null); in.close();
>>
>> Class closed = closed.ClosedResources.fetchClass();
>> assert(closedsStream("test.txt") == null);
>> assert(n.getResourceAsStream("closed/test.txt") == null);
>>
>> The test initially threw an NPE when trying to fetch the open resource. The
>> Module class was fixed by removing the fragment with the (caller == null)
>> test in getResourceAsStream, and changing the call to isOpen(String,Module)
>> to use EVERYONE_MODULE if the caller module is null.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> some cleanup of the test
Thanks for the test update. Looks good.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8134
Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v4]
On Mon, 11 Apr 2022 00:48:34 GMT, Tim Prinzing wrote:
>> Created a test called NullCallerGetResource to test
>> Module::getResourceAsStream and Class::getResourceAsStream from the native
>> level.
>>
>> At the java level the test builds a test module called 'n' which opens the
>> package 'open' to everyone. There is also a package 'closed' which is
>> neither opened or exported. Both packages have a text file called
>> 'test.txt'. The open package has a class called OpenResources and the closed
>> package has a class called ClosedResources. The native test is launched with
>> the test module n added.
>>
>> At the native level the test tries to read both the open and closed resource
>> from both the classes and the module. It performs the equivalent of the
>> following java operations:
>>
>> Class c = open.OpenResources.fetchClass();
>> InputStream in = c.getResourceAsStream("test.txt");
>> assert(in != null); in.close();
>>
>> Module n = c.getModule();
>> in = n.getResourceAsStream("open/test.txt");
>> assert(in != null); in.close();
>>
>> Class closed = closed.ClosedResources.fetchClass();
>> assert(closedsStream("test.txt") == null);
>> assert(n.getResourceAsStream("closed/test.txt") == null);
>>
>> The test initially threw an NPE when trying to fetch the open resource. The
>> Module class was fixed by removing the fragment with the (caller == null)
>> test in getResourceAsStream, and changing the call to isOpen(String,Module)
>> to use EVERYONE_MODULE if the caller module is null.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> requested changes
Marked as reviewed by mchung (Reviewer).
test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c
line 95:
> 93: jclass class_OpenResources = (*env)->FindClass(env,
> "open/OpenResources");
> 94: assert(class_OpenResources != NULL);
> 95: jmethodID mid_OpenResources_fetchClass =
> (*env)->GetStaticMethodID(env, class_OpenResources, "fetchClass",
> "()Ljava/lang/Class;" );
It seems that invoking `fetchClass` isn't necessary since you can simply use
`class_OpenResources`. Similarly for `class_ClosedResources`
test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c
line 183:
> 181: exit(-1);
> 182: }
> 183: assert(in == NULL);
assert is typically used for sanity test. As part of the test validation,
e.g. in this case `in == NULL` or `in != NULL` in line 157, it may be clearer
if it's an explicit check and throw exception to indicate test failure
especially in case `#undef NDEBUG` may not be set in the test.
-
PR: https://git.openjdk.java.net/jdk/pull/8134
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE [v2]
On Tue, 19 Apr 2022 17:29:12 GMT, Claes Redestad wrote: >> Trivially fix the resolution of the `NF_UNSAFE` named function to use the >> appropriate lookup mode. >> >> In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we >> changed from regular reflection to use a `NamedFunction` for this field, but >> it appears the lookup mode came out wrong. This mismatch appears to be >> benign and ignored by HotSpot, but a user implementing an experimental JVM >> ran into some issues (and additionally noticed the resolve throws the wrong >> exception). >> >> This patch is a point fix to this particular issue, but I think we should >> consider following up with a stronger assertion or test for proper >> staticness of fields somewhere when resolving fields via >> `MemberName.getFactory().resolveOrNull(..)`. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Missed a spot! Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v2]
On Thu, 14 Apr 2022 13:59:57 GMT, Claes Redestad wrote: >> This patch examines and optimizes `Wrapper` lookups. >> >> First wrote a few simple microbenchmarks to verify there are actual speedups >> from using perfect hash tables in `sun.invoke.util.Wrapper` compared to >> simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ >> a speed-up for the case of `char` -> `Wrapper`, but not when mapping from >> `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case >> didn't use the `FROM_CHAR` table for some reason, which is remedied. >> >> Micros show benefits across the board for warmed up case: >> >> >> Baseline, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op >> Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op >> Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op >> Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op >> >> Patch, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op >> Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op >> Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op >> Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op >> >> >> For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when >> spinning up of MHs) there are decent or even great wins in all cases but >> `forPrimitiveType` - which was changed from a simple switch to use the hash >> lookup. Since the interpreter penalty is small in absolute terms and the win >> on JITed code is significant this seems like a reasonable trade-off: >> >> >> Baseline, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op >> Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op >> Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op >> Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op >> >> Patch, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op >> Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op >> Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op >> Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add line break in make/test/BuildMicrobenchmark.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> Looks good.The copyright header end-year needs update before you push. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8242
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE
On Tue, 19 Apr 2022 13:19:31 GMT, Claes Redestad wrote: > Trivially fix the resolution of the `NF_UNSAFE` named function to use the > appropriate lookup mode. > > In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we changed > from regular reflection to use a `NamedFunction` for this field, but it > appears the lookup mode came out wrong. This mismatch appears to be benign > and ignored by HotSpot, but a user implementing an experimental JVM ran into > some issues (and additionally noticed the resolve throws the wrong > exception). > > This patch is a point fix to this particular issue, but I think we should > consider following up with a stronger assertion or test for proper staticness > of fields somewhere when resolving fields via > `MemberName.getFactory().resolveOrNull(..)`. src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 901: > 899: MemberName member = new > MemberName(MethodHandleStatics.class, "UNSAFE", Unsafe.class, REF_getStatic); > 900: return new NamedFunction( > 901: > MemberName.getFactory().resolveOrFail(REF_getField, member, `REF_getField` passed to `resolveOrFail` should also be corrected? - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]
On Tue, 19 Apr 2022 10:49:14 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 61:
>>
>>> 59: private final Condition notEmpty;
>>> 60:
>>> 61: void signal() { notEmpty.signalAll(); }
>>
>> `signal()`, `await()` and `await(long)` methods are only used by
>> `ReferenceQueue`. Good to make them private.
>
> They are overridden so can't be private.
That's right, I missed it.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]
On Sun, 17 Apr 2022 16:03:30 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh
src/java.base/share/classes/java/lang/System.java line 2173:
> 2171:
> 2172: // start Finalizer and Reference Handler threads
> 2173: SharedSecrets.getJavaLangRefAccess().startThreads();
I think it would avoid any confusion if `VM.initLevel(1)` is the last step in
`initPhase1`, i.e. call after this line. Previously, the finalizer starts
very early and it has to wait until initLevel is set to level 1 which
guarantees that `JavaLangAccess` is available. With this change,
`JavaLangAccess` is already set.
src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 61:
> 59: private final Condition notEmpty;
> 60:
> 61: void signal() { notEmpty.signalAll(); }
`signal()`, `await()` and `await(long)` methods are only used by
`ReferenceQueue`. Good to make them private.
src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
line 138:
> 136: *
> 137: * @param outputFile the path to the file to create
> 138: * @param format the format to use (TEXT_PLAIN or JSON)
It's useful to link to `ThreadDumpFormat#TEXT_PLAN` and `ThreadDumpFormat#JSON`
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]
On Sun, 17 Apr 2022 16:03:30 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh
src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 206:
> 204: throws IllegalArgumentException, InterruptedException {
> 205: if (timeout < 0) throw new IllegalArgumentException("Negative
> timeout value");
> 206: if (timeout == 0) return remove();
Nit: use the same formatting as in `NativeReferenceQueue::remove` and in this
file.
if (timeout < 0)
throw new IllegalArgumentException("Negative timeout value");
if (timeout == 0)
return remove();
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]
On Sun, 17 Apr 2022 16:03:30 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh src/java.management/share/classes/java/lang/management/ThreadMXBean.java line 655: > 653: * synchronization control. It might be an expensive operation. > 654: * Its behavior with cycles involving virtual threads is not defined > 655: * in this release. What does the current implementation return if the virtual threads are involved in a deadlock?The class spec says "ThreadMXBean does not support monitoring or management of virtual threads" while this javadoc says it's undefined.I wonder if it's helpful to include `@implNote` if appropriate. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On Sun, 17 Apr 2022 16:17:30 GMT, liach wrote:
> Convert dynamic proxies to hidden classes. Modifies the serialization of
> proxies (requires change in "Java Object Serialization Specification"). Makes
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
>
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of
> the class is allocated... The contents restored appropriately" section, I
> propose explicitly state that handling of proxies are unspecified as to allow
> implementation freedom, though I've seen deliberate attempts for proxies to
> implement interfaces with `readResolve` in order to control their
> serialization behavior.
>- This is for the existing generated constructor accessor is
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization
> constructor calls `invokespecial` on the closest serializable superclass'
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and
> should be discouraged in the long term. Calling the existing constructor with
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
> implemented by the lambda, will not appear in the stack trace, and isn't too
> problematic.
>
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into
> one. This reduced a lot of code and allowed anchor class (for hidden class
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of
> `java.base`. This API is intended for implementation of legacy (pre
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
> passes methods computed by proxy generator as class data to the hidden proxy
> class to reduce generated proxy class size and improve performance.
>
> In addition, since this change is somewhat large, should we keep the old
> proxy generator as well and have it toggled through a command-line flag (like
> the old v49 proxy generator or the old reflection implementation)?
>
> Please feel free to comment or review. This change definitely requires a CSR,
> but I have yet to determine what specifications should be changed.
It's good to see more experiment and prototype for this issue. Like Alan
said, the spec change, compatibility risks and security issues in particular on
the serialization spec/impl change require lot of discussions and also
prototyping of other alternatives. A new API may also be one alternative to
consider.
The current spec of Proxy class is defined with null protection domain (same
protection domain as the bootstrap class loader). Lookup::defineHiddenClass
defines the hidden class in the same protection domain as the defining class
loader. This would become a non-issue when the security manager is removed.
However, before the removal of security manager, we still need to look into the
compatibility risks and what and how applications/libraries depend on this
behavior.
During the development of JEP 371, I had a prototype of converting dynamic
proxies to hidden class by adding a shim class (that's what your prototype
does). That raises the question "how to get a Lookup on a package for the
dynamic proxy class to be injected in without injecting a shim class (i.e. your
anchor class)?" This functionality could be considered as a separate RFE.
However, frameworks don't always have the access to inject a class in a package
defined to a class loader. This is something worth looking into.
-
PR: https://git.openjdk.java.net/jdk/pull/8278
Re: RFR: 8284579: Improve VarHandle checks for interpreter [v3]
On Mon, 11 Apr 2022 10:13:40 GMT, Claes Redestad wrote: >> A few additional enhancements aiming to improve VH performance in the >> interpreter: >> >> - Flatten `TypeAndInvokers`: adds a pointer to `VarHandle` (a small increase >> 40->48) but removes an object and an indirection on any instance actually >> used - and might avoid allocating the `MethodHandle[]` unnecessarily on some >> instances >> - Have `checkExactAccessMode` return the directness of the `VarHandle` so >> that we can avoid some `isDirect` method calls. >> >> Baseline, `-Xint` >> >> Benchmark Mode CntScore Error Units >> VarHandleExact.exact_exactInvocation avgt 30 478.324 ? 5.762 ns/op >> VarHandleExact.generic_exactInvocationavgt 30 392.114 ? 1.644 ns/op >> VarHandleExact.generic_genericInvocation avgt 30 822.484 ? 1.865 ns/op >> >> >> Patched, `-Xint` >> >> Benchmark Mode CntScore Error Units >> VarHandleExact.exact_exactInvocation avgt 30 437.704 ? 5.320 ns/op >> VarHandleExact.generic_exactInvocationavgt 30 374.512 ? 3.154 ns/op >> VarHandleExact.generic_genericInvocation avgt 30 757.054 ? 1.237 ns/op >> >> >> No significant performance difference in normal mode. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > checkExactAccessMode -> checkAccessModeThenIsDirect, privatize throw > method, cleanup code generator Dropping `Exact` from `checkExactAccessMode` is good with me and it'd help future readers if you can add a javadoc what this method does. - PR: https://git.openjdk.java.net/jdk/pull/8160
Re: RFR: 8284579: Improve VarHandle checks for interpreter [v3]
On Mon, 11 Apr 2022 10:13:40 GMT, Claes Redestad wrote: >> A few additional enhancements aiming to improve VH performance in the >> interpreter: >> >> - Flatten `TypeAndInvokers`: adds a pointer to `VarHandle` (a small increase >> 40->48) but removes an object and an indirection on any instance actually >> used - and might avoid allocating the `MethodHandle[]` unnecessarily on some >> instances >> - Have `checkExactAccessMode` return the directness of the `VarHandle` so >> that we can avoid some `isDirect` method calls. >> >> Baseline, `-Xint` >> >> Benchmark Mode CntScore Error Units >> VarHandleExact.exact_exactInvocation avgt 30 478.324 ? 5.762 ns/op >> VarHandleExact.generic_exactInvocationavgt 30 392.114 ? 1.644 ns/op >> VarHandleExact.generic_genericInvocation avgt 30 822.484 ? 1.865 ns/op >> >> >> Patched, `-Xint` >> >> Benchmark Mode CntScore Error Units >> VarHandleExact.exact_exactInvocation avgt 30 437.704 ? 5.320 ns/op >> VarHandleExact.generic_exactInvocationavgt 30 374.512 ? 3.154 ns/op >> VarHandleExact.generic_genericInvocation avgt 30 757.054 ? 1.237 ns/op >> >> >> No significant performance difference in normal mode. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > checkExactAccessMode -> checkAccessModeThenIsDirect, privatize throw > method, cleanup code generator Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8160
Re: RFR: 8284579: Improve VarHandle checks for interpreter [v2]
On Fri, 8 Apr 2022 12:20:32 GMT, Claes Redestad wrote: >> A few additional enhancements aiming to improve VH performance in the >> interpreter: >> >> - Flatten `TypeAndInvokers`: adds a pointer to `VarHandle` (a small increase >> 40->48) but removes an object and an indirection on any instance actually >> used - and might avoid allocating the `MethodHandle[]` unnecessarily on some >> instances >> - Have `checkExactAccessMode` return the directness of the `VarHandle` so >> that we can avoid some `isDirect` method calls. >> >> Baseline, `-Xint` >> >> Benchmark Mode CntScore Error Units >> VarHandleExact.exact_exactInvocation avgt 30 478.324 ? 5.762 ns/op >> VarHandleExact.generic_exactInvocationavgt 30 392.114 ? 1.644 ns/op >> VarHandleExact.generic_genericInvocation avgt 30 822.484 ? 1.865 ns/op >> >> >> Patched, `-Xint` >> >> Benchmark Mode CntScore Error Units >> VarHandleExact.exact_exactInvocation avgt 30 437.704 ? 5.320 ns/op >> VarHandleExact.generic_exactInvocationavgt 30 374.512 ? 3.154 ns/op >> VarHandleExact.generic_genericInvocation avgt 30 757.054 ? 1.237 ns/op >> >> >> No significant performance difference in normal mode. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplified as suggested by @ExE-Boss The change looks fine. A `checkXXX` method if returning a boolean would typically indicate if it succeeds or not.`checkExactAccessMode` returns the directness of this VarHandle which is not obvious from its method name. It'd be better if the method name can be explicit that it returns its directness after the exact access mode check. But I can't think of a good method name though. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8160
Re: RFR: 8284361: Updating ASM to 9.3 for JDK 19
On Thu, 7 Apr 2022 03:33:02 GMT, Vicente Romero wrote: > We recently updated our ASM version to 9.2 and just this week version 9.3 was > announced with support for JDK19 so it makes sense to update to this last > version. > > Thanks in advance for the reviews, > Vicente Looks okay. Most of the changes is in javadoc and rename `var` to `varIndex`. The change in ASMifier looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8135
Integrated: 8283470: Update java.lang.invoke.VarHandle to use sealed classes
On Wed, 23 Mar 2022 16:27:29 GMT, Mandy Chung wrote: > This patch changes VarHandle and its implementation hierarchy to use sealed > classes. All VarHandle permitted classes are package-private and either > final or sealed abstract classes. > > Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8283540 This pull request has now been integrated. Changeset: e61ccfba Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/e61ccfba7fa747c24e34a7539871a34630693af5 Stats: 58 lines in 5 files changed: 42 ins; 5 del; 11 mod 8283470: Update java.lang.invoke.VarHandle to use sealed classes Reviewed-by: darcy, psandoz - PR: https://git.openjdk.java.net/jdk/pull/7926
Re: RFR: 8283060: RawNativeLibraries should allow multiple clients to load/unload the same library
On Wed, 30 Mar 2022 13:21:41 GMT, Jaikiran Pai wrote:
>> A small improvement to `RawNativeLibraries`. `RawNativeLibraries::load`
>> returns the same `NativeLibrary` instance of a given path if it's called
>> multiple times. This means that multiple clients have to coordinate that the
>> last one using the loaded library is the one to close the library by calling
>> `RawNativeLibraries::unload`; otherwise, an exception may be thrown.
>>
>> This patch changes `RawNativeLibraries::load` to delegate to the underlying
>> platform-specific library loading mechanism e.g. dlopen/dlclose that keeps
>> the reference count. So each call to `RawNativeLibraries::load` and
>> `unload` is simply a call to dlopen and dlclose respectively. Each client
>> of calling `RawNativeLibraries::load` is responsible for calling
>> `RawNativeLibraries::unload`. This will be consistent with the current
>> behavior when you call `load` and `unload` with a different
>> `RawNativeLibraries` instance.
>
> src/java.base/share/classes/jdk/internal/loader/RawNativeLibraries.java line
> 49:
>
>> 47: public final class RawNativeLibraries {
>> 48: final Set libraries =
>> ConcurrentHashMap.newKeySet();
>> 49: final Class caller;
>
> Hello Mandy,
> Apart from the `caller` being used for checks while constructing a
> `RawNativeLibraries` in `RawNativeLibraries.newInstance(MethodHandles.Lookup
> trustedCaller)`, I don't see this instance member being used anywhere. Do you
> think we need to store this as an instance member?
I keep it as a record and for debugging use in case.
-
PR: https://git.openjdk.java.net/jdk/pull/8022
RFR: 8283060: RawNativeLibraries should allow multiple clients to load/unload the same library
A small improvement to `RawNativeLibraries`. `RawNativeLibraries::load` returns the same `NativeLibrary` instance of a given path if it's called multiple times. This means that multiple clients have to coordinate that the last one using the loaded library is the one to close the library by calling `RawNativeLibraries::unload`; otherwise, an exception may be thrown. This patch changes `RawNativeLibraries::load` to delegate to the underlying platform-specific library loading mechanism e.g. dlopen/dlclose that keeps the reference count. So each call to `RawNativeLibraries::load` and `unload` is simply a call to dlopen and dlclose respectively. Each client of calling `RawNativeLibraries::load` is responsible for calling `RawNativeLibraries::unload`. This will be consistent with the current behavior when you call `load` and `unload` with a different `RawNativeLibraries` instance. - Commit messages: - 8283060: RawNativeLibraries should allow multiple clients to load/unload the same library Changes: https://git.openjdk.java.net/jdk/pull/8022/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8022=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283060 Stats: 66 lines in 3 files changed: 43 ins; 10 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/8022.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8022/head:pull/8022 PR: https://git.openjdk.java.net/jdk/pull/8022
Integrated: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver
On Wed, 9 Mar 2022 22:52:41 GMT, Mandy Chung wrote: > A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. This pull request has now been integrated. Changeset: 489b27d2 Author: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/489b27d2c0284f9248bfb0448950698a3f9dee36 Stats: 16 lines in 1 file changed: 10 ins; 0 del; 6 mod 8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v5]
> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. Mandy Chung 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 six additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8282776 - per feedback - Add exception message - Move the null check after isInstance check - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8282776 - JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver - Changes: - all: https://git.openjdk.java.net/jdk/pull/7766/files - new: https://git.openjdk.java.net/jdk/pull/7766/files/55c38cf8..5c1d777b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7766=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7766=03-04 Stats: 124526 lines in 1656 files changed: 92323 ins; 27724 del; 4479 mod Patch: https://git.openjdk.java.net/jdk/pull/7766.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7766/head:pull/7766 PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Looks good. Thanks for improving the error reporting in CheckCSMs test. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283846: Remove unused jdk.internal.reflect.SignatureIterator
On Tue, 29 Mar 2022 09:15:01 GMT, Andrey Turbanov wrote: > It was no longer used due to JDK-4479184 long ago. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8013
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes [v2]
On Thu, 24 Mar 2022 18:58:43 GMT, Joe Darcy wrote: >> Small refactoring to use sealed classes in the MethodHandle implementation >> hierarchy. >> >> DelegatingMethodHandle is non-sealed rather than sealed since it has two >> subclasses, one defined as a nested class and only a separate type in the >> same package. The permits clause does not allow listed those two kinds of >> subclasses. >> >> Please also review the corresponding CSR >> https://bugs.openjdk.java.net/browse/JDK-8283434 > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains three additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8283416 > - JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v3]
On Wed, 23 Mar 2022 03:37:01 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line
>> 424:
>>
>>> 422: throw new IncompatibleClassChangeError(msg);
>>> 423: } else {
>>> 424: throw new NullPointerException("Cannot invoke " +
>>> member + " with null receiver");
>>
>> Suggestion:
>>
>> String msg = String.format("Cannot invoke %s with null
>> receiver",
>>member);
>> throw new NullPointerException(msg);
>
> Good catch. Will update it.
I now reminded myself that java.base is built with -XDstringConcat=inline to
avoid the bootstrap issue. So the original patch should have no issue. In
any case, I updated the patch to use `String.format` to be consistent with the
ICCE message formatting.
-
PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v4]
> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: per feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/7766/files - new: https://git.openjdk.java.net/jdk/pull/7766/files/62f74c70..55c38cf8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7766=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7766=02-03 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7766.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7766/head:pull/7766 PR: https://git.openjdk.java.net/jdk/pull/7766
