Re: RFR: 8264109: Add vectorized implementation for VectorMask.andNot() [v2]
On Thu, 1 Apr 2021 15:13:31 GMT, Paul Sandoz wrote: >> Xiaohong Gong has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move the changing to AbstractMask.andNot and revert changes in VectorMask >> >> Change-Id: Ie3ec8f53cb24526c8e1ccc68038355d024910818 >> CustomizedGitHooks: yes > > Looks good. Thanks for your review @PaulSandoz ! - PR: https://git.openjdk.java.net/jdk/pull/3211
Re: RFR: 8256245: AArch64: Implement Base64 decoding intrinsic [v3]
> In JDK-8248188, IntrinsicCandidate and API is added for Base64 decoding. > Base64 decoding can be improved on aarch64 with ld4/tbl/tbx/st3, a basic idea > can be found at > http://0x80.pl/articles/base64-simd-neon.html#encoding-quadwords. > > Patch passed jtreg tier1-3 tests with linux-aarch64-server-fastdebug build. > Tests in `test/jdk/java/util/Base64/` and > `compiler/intrinsics/base64/TestBase64.java` runned specially for the > correctness of the implementation. > > There can be illegal characters at the start of the input if the data is MIME > encoded. > It would be no benefits to use SIMD for this case, so the stub use no-simd > instructions for MIME encoded data now. > > A JMH micro, Base64Decode.java, is added for performance test. > With different input length (upper-bounded by parameter `maxNumBytes` in the > JMH micro), > we witness ~2.5x improvements with long inputs and no regression with short > inputs for raw base64 decodeing, minor improvements (~10.95%) for MIME on > Kunpeng916. > > The Base64Decode.java JMH micro-benchmark results: > > Benchmark (lineSize) (maxNumBytes) Mode Cnt > Score Error Units > > # Kunpeng916, intrinsic > Base64Decode.testBase64Decode 4 1 avgt5 > 48.614 ± 0.609 ns/op > Base64Decode.testBase64Decode 4 3 avgt5 > 58.199 ± 1.650 ns/op > Base64Decode.testBase64Decode 4 7 avgt5 > 69.400 ± 0.931 ns/op > Base64Decode.testBase64Decode 4 32 avgt5 > 96.818 ± 1.687 ns/op > Base64Decode.testBase64Decode 4 64 avgt5 > 122.856 ± 9.217 ns/op > Base64Decode.testBase64Decode 4 80 avgt5 > 130.935 ± 1.667 ns/op > Base64Decode.testBase64Decode 4 96 avgt5 > 143.627 ± 1.751 ns/op > Base64Decode.testBase64Decode 4112 avgt5 > 152.311 ± 1.178 ns/op > Base64Decode.testBase64Decode 4512 avgt5 > 342.631 ± 0.584 ns/op > Base64Decode.testBase64Decode 4 1000 avgt5 > 573.635 ± 1.050 ns/op > Base64Decode.testBase64Decode 4 2 avgt5 > 9534.136 ±45.172 ns/op > Base64Decode.testBase64Decode 4 5 avgt5 > 22718.726 ± 192.070 ns/op > Base64Decode.testBase64MIMEDecode 4 1 avgt 10 > 63.558 ±0.336 ns/op > Base64Decode.testBase64MIMEDecode 4 3 avgt 10 > 82.504 ±0.848 ns/op > Base64Decode.testBase64MIMEDecode 4 7 avgt 10 > 120.591 ±0.608 ns/op > Base64Decode.testBase64MIMEDecode 4 32 avgt 10 > 324.314 ±6.236 ns/op > Base64Decode.testBase64MIMEDecode 4 64 avgt 10 > 532.678 ±4.670 ns/op > Base64Decode.testBase64MIMEDecode 4 80 avgt 10 > 678.126 ±4.324 ns/op > Base64Decode.testBase64MIMEDecode 4 96 avgt 10 > 771.603 ±6.393 ns/op > Base64Decode.testBase64MIMEDecode 4112 avgt 10 > 889.608 ± 0.759 ns/op > Base64Decode.testBase64MIMEDecode 4512 avgt 10 > 3663.557 ±3.422 ns/op > Base64Decode.testBase64MIMEDecode 4 1000 avgt 10 > 7017.784 ±9.128 ns/op > Base64Decode.testBase64MIMEDecode 4 2 avgt 10 > 128670.660 ± 7951.521 ns/op > Base64Decode.testBase64MIMEDecode 4 5 avgt 10 > 317113.667 ± 161.758 ns/op > > # Kunpeng916, default > Base64Decode.testBase64Decode 4 1 avgt5 > 48.455 ± 0.571 ns/op > Base64Decode.testBase64Decode 4 3 avgt5 > 57.937 ± 0.505 ns/op > Base64Decode.testBase64Decode 4 7 avgt5 > 73.823 ± 1.452 ns/op > Base64Decode.testBase64Decode 4 32 avgt5 > 106.484 ± 1.243 ns/op > Base64Decode.testBase64Decode 4 64 avgt5 > 141.004 ± 1.188 ns/op > Base64Decode.testBase64Decode 4 80 avgt5 > 156.284 ± 0.572 ns/op > Base64Decode.testBase64Decode 4 96 avgt5 > 174.137 ± 0.177 ns/op > Base64Decode.testBase64Decode 4112 avgt5 > 188.445 ± 0.572 ns/op > Base64Decode.testBase64Decode 4512 avgt5 > 610.847 ± 1.559 ns/op > Base64Decode.testBase64Decode 4 1000 avgt5 > 1155.368 ± 0.813 ns/op > Base64Decode.testBase64Decode 4 2 avgt5 > 19751.477 ± 24.669 ns/op >
Integrated: 8205502: Make exception message from AnnotationInvocationHandler more informative
On Thu, 1 Apr 2021 22:15:01 GMT, Joe Darcy wrote: > Simple change to make the exception/error messages more informative for > various malformed annotation situations. This pull request has now been integrated. Changeset: 66d9961c Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/66d9961c Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod 8205502: Make exception message from AnnotationInvocationHandler more informative Reviewed-by: bpb, naoto, iris - PR: https://git.openjdk.java.net/jdk/pull/3317
Re: RFR: 8205502: Make exception message from AnnotationInvocationHandler more informative
On Thu, 1 Apr 2021 22:15:01 GMT, Joe Darcy wrote: > Simple change to make the exception/error messages more informative for > various malformed annotation situations. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3317
Re: RFR: 8264544: Case-insensitive comparison issue with supplementary characters.
On Thu, 1 Apr 2021 03:24:04 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. Thanks to the contribution by > Chris Johnson. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3300
Re: RFR: 8264544: Case-insensitive comparison issue with supplementary characters.
On Thu, 1 Apr 2021 03:24:04 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. Thanks to the contribution by > Chris Johnson. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3300
Re: RFR: 8205502: Make exception message from AnnotationInvocationHandler more informative
On Thu, 1 Apr 2021 22:15:01 GMT, Joe Darcy wrote: > Simple change to make the exception/error messages more informative for > various malformed annotation situations. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3317
Re: RFR: 8205502: Make exception message from AnnotationInvocationHandler more informative
On Thu, 1 Apr 2021 22:15:01 GMT, Joe Darcy wrote: > Simple change to make the exception/error messages more informative for > various malformed annotation situations. Looks fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3317
RFR: 8205502: Make exception message from AnnotationInvocationHandler more informative
Simple change to make the exception/error messages more informative for various malformed annotation situations. - Commit messages: - Appease jcheck. - 8205502: Make exception message from AnnotationInvocationHandler more informative Changes: https://git.openjdk.java.net/jdk/pull/3317/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3317=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8205502 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3317.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3317/head:pull/3317 PR: https://git.openjdk.java.net/jdk/pull/3317
Re: RFR: 8037397: Lost nested character class after intersection && [v2]
> Bug fix with the intersection `&&` operator in regex patterns. In > JDK-8037397, some character classes on the right hand side of the operator > are dropped in cases where nested `[..]` classes are used with non "nested" > ones. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Fixing missing space - Changes: - all: https://git.openjdk.java.net/jdk/pull/3291/files - new: https://git.openjdk.java.net/jdk/pull/3291/files/76cf0be2..2a783a1e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3291=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3291=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3291/head:pull/3291 PR: https://git.openjdk.java.net/jdk/pull/3291
RFR: 8264544: Case-insensitive comparison issue with supplementary characters.
Please review the fix to the subject issue. Thanks to the contribution by Chris Johnson. - Commit messages: - 8264544: Case-insensitive comparison issue with supplementary characters. Changes: https://git.openjdk.java.net/jdk/pull/3300/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3300=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264544 Stats: 11 lines in 3 files changed: 3 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/3300.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3300/head:pull/3300 PR: https://git.openjdk.java.net/jdk/pull/3300
Integrated: JDK-8264403: [macos]: App names containing '.' characters results in an error message when launching
On Wed, 31 Mar 2021 19:30:10 GMT, Andy Herrick wrote: > Deriving the cfg file name is broken on mac and linux when the application > name has a "." in it. This pull request has now been integrated. Changeset: 04f24fe9 Author:Andy Herrick URL: https://git.openjdk.java.net/jdk/commit/04f24fe9 Stats: 81 lines in 5 files changed: 78 ins; 1 del; 2 mod 8264403: [macos]: App names containing '.' characters results in an error message when launching Reviewed-by: asemenyuk - PR: https://git.openjdk.java.net/jdk/pull/3288
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v41]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Fix NotCompliantCauseTest to not rely on Random not being a bean - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/5a23b4f1..c5fb77f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=40 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=39-40 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Integrated: 8264609 : Number.{byteValue, shortValue} spec should use @implSpec
On Thu, 1 Apr 2021 18:12:43 GMT, Joe Darcy wrote:
> Please review this small code change and its accompanying CSR:
>
> https://bugs.openjdk.java.net/browse/JDK-8264610
This pull request has now been integrated.
Changeset: b953386d
Author:Joe Darcy
URL: https://git.openjdk.java.net/jdk/commit/b953386d
Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod
8264609: Number.{byteValue, shortValue} spec should use @implSpec
Reviewed-by: psandoz, bpb
-
PR: https://git.openjdk.java.net/jdk/pull/3314
Re: RFR: 8264288: Performance issue with MethodHandle.asCollector
On Thu, 1 Apr 2021 19:09:53 GMT, John R Rose wrote:
>> This patch speeds up MethodHandle.asCollector handles where the array type
>> is not Object[], as well as speeding up all collectors where the arity is
>> greater than 10.
>>
>> The old code is creating a collector handle by combining a set of hard coded
>> methods for collecting arguments into an Object[], up to a maximum of ten
>> elements, and then copying this intermediate array into a final array.
>>
>> In principle, it shouldn't matter how slow (or fast) this handle is, because
>> it should be replaced by existing bytecode intrinsification which does the
>> right thing. But, through investigation it turns out that the
>> intrinsification is only being applied in a very limited amount of cases:
>> Object[] with max 10 elements only, only for the intermediate collector
>> handles. Every other collector shape incurs overhead because it essentially
>> ends up calling the ineffecient fallback handle.
>>
>> Rather than sticking on a band aid (I tried this, but it turned out to be
>> very tricky to untangle the existing code), the new code replaces the
>> existing implementation with a collector handle implemented using a
>> LambdaForm, which removes the need for intrinsification, and also greatly
>> reduces code-complexity of the implementation. (I plan to expose this
>> collector using a public API in the future as well, so users don't have to
>> go through MHs::identity to make a collector).
>>
>> The old code was also using a special lambda form transform for collecting
>> arguments into an array. I believe this was done to take advantage of the
>> existing-but-limited bytecode intrinsification, at least for Object[] with
>> less than 10 elements.
>>
>> The new code just uses the existing collect arguments transform with the
>> newly added collector handle as filter, and this works just as well for the
>> existing case, but as a bonus is also much simpler, since no separate
>> transform is needed. Using the collect arguments transform should also
>> improve sharing.
>>
>> As a result of these changes a lot of code was unused and has been removed
>> in this patch.
>>
>> Testing: tier 1-3, benchmarking using TypedAsCollector (part of the patch),
>> as well as another variant of the benchmark that used a declared static
>> identity method instead of MHs::identity (not included). Before/after
>> comparison of MethodHandleAs* benchmarks (no differences there).
>>
>> Here are some numbers from the added benchmark:
>>
>> Before:
>> BenchmarkMode CntScoreError
>> Units
>> TypedAsCollector.testIntCollect avgt 30 189.156 � 1.796
>> ns/op
>> TypedAsCollector.testIntCollectHighArity avgt 30 660.549 � 10.159
>> ns/op
>> TypedAsCollector.testObjectCollect avgt 307.092 � 0.042
>> ns/op
>> TypedAsCollector.testObjectCollectHighArity avgt 30 65.225 � 0.546
>> ns/op
>> TypedAsCollector.testStringCollect avgt 30 28.511 � 0.243
>> ns/op
>> TypedAsCollector.testStringCollectHighArity avgt 30 57.054 � 0.635
>> ns/op
>> (as you can see, just the Object[] with arity less than 10 case is fast here)
>> After:
>> BenchmarkMode Cnt Score Error Units
>> TypedAsCollector.testIntCollect avgt 30 6.569 � 0.131 ns/op
>> TypedAsCollector.testIntCollectHighArity avgt 30 8.923 � 0.066 ns/op
>> TypedAsCollector.testObjectCollect avgt 30 6.813 � 0.035 ns/op
>> TypedAsCollector.testObjectCollectHighArity avgt 30 9.718 � 0.066 ns/op
>> TypedAsCollector.testStringCollect avgt 30 6.737 � 0.016 ns/op
>> TypedAsCollector.testStringCollectHighArity avgt 30 9.618 � 0.052 ns/op
>>
>> Thanks,
>> Jorn
>
> src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1903:
>
>> 1901: }
>> 1902:
>> 1903: private static LambdaForm makeCollectorForm(MethodType basicType,
>> MethodHandle storeFunc) {
>
> This reads very well. It is the right way to do this job with LFs.
> There is a slight risk that the JIT will fail to inline the tiny MHs
> that (a) construct arrays and (b) poke elements into them.
> If that happens, then perhaps the bytecode generator can
> treat them as intrinsics. Maybe that's worth a comment in
> the code, to help later readers?
Good idea, I'll leave some notes.
-
PR: https://git.openjdk.java.net/jdk/pull/3306
Re: RFR: 8264610: Number.{byteValue, shortValue} spec should use @implSpec
On Thu, 1 Apr 2021 18:12:43 GMT, Joe Darcy wrote: > Please review this small code change and its accompanying CSR: > > https://bugs.openjdk.java.net/browse/JDK-8264610 CSR also reviewed. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3314
Re: RFR: 8264288: Performance issue with MethodHandle.asCollector
On Thu, 1 Apr 2021 19:09:14 GMT, John R Rose wrote: >> This patch speeds up MethodHandle.asCollector handles where the array type >> is not Object[], as well as speeding up all collectors where the arity is >> greater than 10. >> >> The old code is creating a collector handle by combining a set of hard coded >> methods for collecting arguments into an Object[], up to a maximum of ten >> elements, and then copying this intermediate array into a final array. >> >> In principle, it shouldn't matter how slow (or fast) this handle is, because >> it should be replaced by existing bytecode intrinsification which does the >> right thing. But, through investigation it turns out that the >> intrinsification is only being applied in a very limited amount of cases: >> Object[] with max 10 elements only, only for the intermediate collector >> handles. Every other collector shape incurs overhead because it essentially >> ends up calling the ineffecient fallback handle. >> >> Rather than sticking on a band aid (I tried this, but it turned out to be >> very tricky to untangle the existing code), the new code replaces the >> existing implementation with a collector handle implemented using a >> LambdaForm, which removes the need for intrinsification, and also greatly >> reduces code-complexity of the implementation. (I plan to expose this >> collector using a public API in the future as well, so users don't have to >> go through MHs::identity to make a collector). >> >> The old code was also using a special lambda form transform for collecting >> arguments into an array. I believe this was done to take advantage of the >> existing-but-limited bytecode intrinsification, at least for Object[] with >> less than 10 elements. >> >> The new code just uses the existing collect arguments transform with the >> newly added collector handle as filter, and this works just as well for the >> existing case, but as a bonus is also much simpler, since no separate >> transform is needed. Using the collect arguments transform should also >> improve sharing. >> >> As a result of these changes a lot of code was unused and has been removed >> in this patch. >> >> Testing: tier 1-3, benchmarking using TypedAsCollector (part of the patch), >> as well as another variant of the benchmark that used a declared static >> identity method instead of MHs::identity (not included). Before/after >> comparison of MethodHandleAs* benchmarks (no differences there). >> >> Here are some numbers from the added benchmark: >> >> Before: >> BenchmarkMode CntScoreError >> Units >> TypedAsCollector.testIntCollect avgt 30 189.156 � 1.796 >> ns/op >> TypedAsCollector.testIntCollectHighArity avgt 30 660.549 � 10.159 >> ns/op >> TypedAsCollector.testObjectCollect avgt 307.092 � 0.042 >> ns/op >> TypedAsCollector.testObjectCollectHighArity avgt 30 65.225 � 0.546 >> ns/op >> TypedAsCollector.testStringCollect avgt 30 28.511 � 0.243 >> ns/op >> TypedAsCollector.testStringCollectHighArity avgt 30 57.054 � 0.635 >> ns/op >> (as you can see, just the Object[] with arity less than 10 case is fast here) >> After: >> BenchmarkMode Cnt Score Error Units >> TypedAsCollector.testIntCollect avgt 30 6.569 � 0.131 ns/op >> TypedAsCollector.testIntCollectHighArity avgt 30 8.923 � 0.066 ns/op >> TypedAsCollector.testObjectCollect avgt 30 6.813 � 0.035 ns/op >> TypedAsCollector.testObjectCollectHighArity avgt 30 9.718 � 0.066 ns/op >> TypedAsCollector.testStringCollect avgt 30 6.737 � 0.016 ns/op >> TypedAsCollector.testStringCollectHighArity avgt 30 9.618 � 0.052 ns/op >> >> Thanks, >> Jorn > > src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1342: > >> 1340: } >> 1341: >> 1342: // Array identity function (used as >> getConstantHandle(MH_arrayIdentity)). > > If this is no longer used, remove it. Yes, it looks like this can be removed. Good catch! - PR: https://git.openjdk.java.net/jdk/pull/3306
Re: RFR: 8264288: Performance issue with MethodHandle.asCollector
On Thu, 1 Apr 2021 19:12:42 GMT, Paul Sandoz wrote:
>> This patch speeds up MethodHandle.asCollector handles where the array type
>> is not Object[], as well as speeding up all collectors where the arity is
>> greater than 10.
>>
>> The old code is creating a collector handle by combining a set of hard coded
>> methods for collecting arguments into an Object[], up to a maximum of ten
>> elements, and then copying this intermediate array into a final array.
>>
>> In principle, it shouldn't matter how slow (or fast) this handle is, because
>> it should be replaced by existing bytecode intrinsification which does the
>> right thing. But, through investigation it turns out that the
>> intrinsification is only being applied in a very limited amount of cases:
>> Object[] with max 10 elements only, only for the intermediate collector
>> handles. Every other collector shape incurs overhead because it essentially
>> ends up calling the ineffecient fallback handle.
>>
>> Rather than sticking on a band aid (I tried this, but it turned out to be
>> very tricky to untangle the existing code), the new code replaces the
>> existing implementation with a collector handle implemented using a
>> LambdaForm, which removes the need for intrinsification, and also greatly
>> reduces code-complexity of the implementation. (I plan to expose this
>> collector using a public API in the future as well, so users don't have to
>> go through MHs::identity to make a collector).
>>
>> The old code was also using a special lambda form transform for collecting
>> arguments into an array. I believe this was done to take advantage of the
>> existing-but-limited bytecode intrinsification, at least for Object[] with
>> less than 10 elements.
>>
>> The new code just uses the existing collect arguments transform with the
>> newly added collector handle as filter, and this works just as well for the
>> existing case, but as a bonus is also much simpler, since no separate
>> transform is needed. Using the collect arguments transform should also
>> improve sharing.
>>
>> As a result of these changes a lot of code was unused and has been removed
>> in this patch.
>>
>> Testing: tier 1-3, benchmarking using TypedAsCollector (part of the patch),
>> as well as another variant of the benchmark that used a declared static
>> identity method instead of MHs::identity (not included). Before/after
>> comparison of MethodHandleAs* benchmarks (no differences there).
>>
>> Here are some numbers from the added benchmark:
>>
>> Before:
>> BenchmarkMode CntScoreError
>> Units
>> TypedAsCollector.testIntCollect avgt 30 189.156 � 1.796
>> ns/op
>> TypedAsCollector.testIntCollectHighArity avgt 30 660.549 � 10.159
>> ns/op
>> TypedAsCollector.testObjectCollect avgt 307.092 � 0.042
>> ns/op
>> TypedAsCollector.testObjectCollectHighArity avgt 30 65.225 � 0.546
>> ns/op
>> TypedAsCollector.testStringCollect avgt 30 28.511 � 0.243
>> ns/op
>> TypedAsCollector.testStringCollectHighArity avgt 30 57.054 � 0.635
>> ns/op
>> (as you can see, just the Object[] with arity less than 10 case is fast here)
>> After:
>> BenchmarkMode Cnt Score Error Units
>> TypedAsCollector.testIntCollect avgt 30 6.569 � 0.131 ns/op
>> TypedAsCollector.testIntCollectHighArity avgt 30 8.923 � 0.066 ns/op
>> TypedAsCollector.testObjectCollect avgt 30 6.813 � 0.035 ns/op
>> TypedAsCollector.testObjectCollectHighArity avgt 30 9.718 � 0.066 ns/op
>> TypedAsCollector.testStringCollect avgt 30 6.737 � 0.016 ns/op
>> TypedAsCollector.testStringCollectHighArity avgt 30 9.618 � 0.052 ns/op
>>
>> Thanks,
>> Jorn
>
> src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1277:
>
>> 1275: if (intrinsicName == Intrinsic.IDENTITY) {
>> 1276: MethodType resultType =
>> type().asCollectorType(arrayType, type().parameterCount() - 1, arrayLength);
>> 1277: MethodHandle collector =
>> MethodHandleImpl.makeCollector(arrayType, arrayLength);
>
> Should `MethodHandleImpl.varargsArray` still be used here since it performs
> arity checking and caching?
>
> Maybe the arity checks are performed by `asCollectorType`, but that would
> still leave the caching.
Ah right, good catch! This is a leftover from moving things around and should
indeed use `varargsArray`.
-
PR: https://git.openjdk.java.net/jdk/pull/3306
Re: RFR: 8264610: Number.{byteValue, shortValue} spec should use @implSpec
On Thu, 1 Apr 2021 18:12:43 GMT, Joe Darcy wrote: > Please review this small code change and its accompanying CSR: > > https://bugs.openjdk.java.net/browse/JDK-8264610 Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3314
Re: RFR: 8264288: Performance issue with MethodHandle.asCollector
On Thu, 1 Apr 2021 13:25:05 GMT, Jorn Vernee wrote:
> This patch speeds up MethodHandle.asCollector handles where the array type is
> not Object[], as well as speeding up all collectors where the arity is
> greater than 10.
>
> The old code is creating a collector handle by combining a set of hard coded
> methods for collecting arguments into an Object[], up to a maximum of ten
> elements, and then copying this intermediate array into a final array.
>
> In principle, it shouldn't matter how slow (or fast) this handle is, because
> it should be replaced by existing bytecode intrinsification which does the
> right thing. But, through investigation it turns out that the
> intrinsification is only being applied in a very limited amount of cases:
> Object[] with max 10 elements only, only for the intermediate collector
> handles. Every other collector shape incurs overhead because it essentially
> ends up calling the ineffecient fallback handle.
>
> Rather than sticking on a band aid (I tried this, but it turned out to be
> very tricky to untangle the existing code), the new code replaces the
> existing implementation with a collector handle implemented using a
> LambdaForm, which removes the need for intrinsification, and also greatly
> reduces code-complexity of the implementation. (I plan to expose this
> collector using a public API in the future as well, so users don't have to go
> through MHs::identity to make a collector).
>
> The old code was also using a special lambda form transform for collecting
> arguments into an array. I believe this was done to take advantage of the
> existing-but-limited bytecode intrinsification, at least for Object[] with
> less than 10 elements.
>
> The new code just uses the existing collect arguments transform with the
> newly added collector handle as filter, and this works just as well for the
> existing case, but as a bonus is also much simpler, since no separate
> transform is needed. Using the collect arguments transform should also
> improve sharing.
>
> As a result of these changes a lot of code was unused and has been removed in
> this patch.
>
> Testing: tier 1-3, benchmarking using TypedAsCollector (part of the patch),
> as well as another variant of the benchmark that used a declared static
> identity method instead of MHs::identity (not included). Before/after
> comparison of MethodHandleAs* benchmarks (no differences there).
>
> Here are some numbers from the added benchmark:
>
> Before:
> BenchmarkMode CntScoreError
> Units
> TypedAsCollector.testIntCollect avgt 30 189.156 � 1.796
> ns/op
> TypedAsCollector.testIntCollectHighArity avgt 30 660.549 � 10.159
> ns/op
> TypedAsCollector.testObjectCollect avgt 307.092 � 0.042
> ns/op
> TypedAsCollector.testObjectCollectHighArity avgt 30 65.225 � 0.546
> ns/op
> TypedAsCollector.testStringCollect avgt 30 28.511 � 0.243
> ns/op
> TypedAsCollector.testStringCollectHighArity avgt 30 57.054 � 0.635
> ns/op
> (as you can see, just the Object[] with arity less than 10 case is fast here)
> After:
> BenchmarkMode Cnt Score Error Units
> TypedAsCollector.testIntCollect avgt 30 6.569 � 0.131 ns/op
> TypedAsCollector.testIntCollectHighArity avgt 30 8.923 � 0.066 ns/op
> TypedAsCollector.testObjectCollect avgt 30 6.813 � 0.035 ns/op
> TypedAsCollector.testObjectCollectHighArity avgt 30 9.718 � 0.066 ns/op
> TypedAsCollector.testStringCollect avgt 30 6.737 � 0.016 ns/op
> TypedAsCollector.testStringCollectHighArity avgt 30 9.618 � 0.052 ns/op
>
> Thanks,
> Jorn
That's an elegant solution.
At first i thought it might unduly perturb lambda form generation and caching.
but you slotted a different lambda form implementation underneath the varargs
implementation.
src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1277:
> 1275: if (intrinsicName == Intrinsic.IDENTITY) {
> 1276: MethodType resultType =
> type().asCollectorType(arrayType, type().parameterCount() - 1, arrayLength);
> 1277: MethodHandle collector =
> MethodHandleImpl.makeCollector(arrayType, arrayLength);
Should `MethodHandleImpl.varargsArray` still be used here since it performs
arity checking and caching?
Maybe the arity checks are performed by `asCollectorType`, but that would still
leave the caching.
-
PR: https://git.openjdk.java.net/jdk/pull/3306
Re: RFR: 8264288: Performance issue with MethodHandle.asCollector
On Thu, 1 Apr 2021 13:25:05 GMT, Jorn Vernee wrote:
> This patch speeds up MethodHandle.asCollector handles where the array type is
> not Object[], as well as speeding up all collectors where the arity is
> greater than 10.
>
> The old code is creating a collector handle by combining a set of hard coded
> methods for collecting arguments into an Object[], up to a maximum of ten
> elements, and then copying this intermediate array into a final array.
>
> In principle, it shouldn't matter how slow (or fast) this handle is, because
> it should be replaced by existing bytecode intrinsification which does the
> right thing. But, through investigation it turns out that the
> intrinsification is only being applied in a very limited amount of cases:
> Object[] with max 10 elements only, only for the intermediate collector
> handles. Every other collector shape incurs overhead because it essentially
> ends up calling the ineffecient fallback handle.
>
> Rather than sticking on a band aid (I tried this, but it turned out to be
> very tricky to untangle the existing code), the new code replaces the
> existing implementation with a collector handle implemented using a
> LambdaForm, which removes the need for intrinsification, and also greatly
> reduces code-complexity of the implementation. (I plan to expose this
> collector using a public API in the future as well, so users don't have to go
> through MHs::identity to make a collector).
>
> The old code was also using a special lambda form transform for collecting
> arguments into an array. I believe this was done to take advantage of the
> existing-but-limited bytecode intrinsification, at least for Object[] with
> less than 10 elements.
>
> The new code just uses the existing collect arguments transform with the
> newly added collector handle as filter, and this works just as well for the
> existing case, but as a bonus is also much simpler, since no separate
> transform is needed. Using the collect arguments transform should also
> improve sharing.
>
> As a result of these changes a lot of code was unused and has been removed in
> this patch.
>
> Testing: tier 1-3, benchmarking using TypedAsCollector (part of the patch),
> as well as another variant of the benchmark that used a declared static
> identity method instead of MHs::identity (not included). Before/after
> comparison of MethodHandleAs* benchmarks (no differences there).
>
> Here are some numbers from the added benchmark:
>
> Before:
> BenchmarkMode CntScoreError
> Units
> TypedAsCollector.testIntCollect avgt 30 189.156 � 1.796
> ns/op
> TypedAsCollector.testIntCollectHighArity avgt 30 660.549 � 10.159
> ns/op
> TypedAsCollector.testObjectCollect avgt 307.092 � 0.042
> ns/op
> TypedAsCollector.testObjectCollectHighArity avgt 30 65.225 � 0.546
> ns/op
> TypedAsCollector.testStringCollect avgt 30 28.511 � 0.243
> ns/op
> TypedAsCollector.testStringCollectHighArity avgt 30 57.054 � 0.635
> ns/op
> (as you can see, just the Object[] with arity less than 10 case is fast here)
> After:
> BenchmarkMode Cnt Score Error Units
> TypedAsCollector.testIntCollect avgt 30 6.569 � 0.131 ns/op
> TypedAsCollector.testIntCollectHighArity avgt 30 8.923 � 0.066 ns/op
> TypedAsCollector.testObjectCollect avgt 30 6.813 � 0.035 ns/op
> TypedAsCollector.testObjectCollectHighArity avgt 30 9.718 � 0.066 ns/op
> TypedAsCollector.testStringCollect avgt 30 6.737 � 0.016 ns/op
> TypedAsCollector.testStringCollectHighArity avgt 30 9.618 � 0.052 ns/op
>
> Thanks,
> Jorn
So many red deletion lines; I'm looking at a beautiful sunset!
It's a sunset for some very old code, some of the first code
I wrote for method handles, long before Lambda Forms
made this sort of task easier.
Thanks very much for cleaning this out. See a few
minor comments on some diff lines.
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 651:
> 649: }
> 650:
> 651: LambdaForm collectArgumentArrayForm(int pos, MethodHandle
> arrayCollector) {
It's counter-intuitive that removing a LFE tactic would be harmless.
Each LFE tactic is a point where LFs can be shared, reducing footprint.
But in this case `collectArgumentArrayForm` is always paired with
`collectArgumentsForm`, so the latter takes care of sharing.
The actual code which makes the arrays is also shared, via
`Makers.TYPED_COLLECTORS` (unchanged).
src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1342:
> 1340: }
> 1341:
> 1342: // Array identity function (used as
> getConstantHandle(MH_arrayIdentity)).
If this is no longer used, remove it.
src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1903:
> 1901: }
> 1902:
> 1903: private static LambdaForm
Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
On Thu, 1 Apr 2021 18:19:35 GMT, Alan Bateman wrote: >> We should never close the jimage since java threads can still be running >> after a hard exit(). > > src/java.base/share/native/libjimage/imageFile.cpp line 219: > >> 217: // WARNING: Should never close the jimage file. >> 218: // Threads may still be running at shutdown. >> 219: #if 0 > > Are you keeping the code in order to re-visit it again? Just wondering why > it's not deleted. Leaving the code as an example of what is required to close in case the topic gets revisited in the future and I get hit by a bus or dementia. - PR: https://git.openjdk.java.net/jdk/pull/3304
Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
On Thu, 1 Apr 2021 11:48:21 GMT, Jim Laskey wrote: > We should never close the jimage since java threads can still be running > after a hard exit(). src/java.base/share/native/libjimage/imageFile.cpp line 219: > 217: // WARNING: Should never close the jimage file. > 218: // Threads may still be running at shutdown. > 219: #if 0 Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted. - PR: https://git.openjdk.java.net/jdk/pull/3304
RFR: 8264610: Number.{byteValue, shortValue} spec should use @implSpec
Please review this small code change and its accompanying CSR: https://bugs.openjdk.java.net/browse/JDK-8264610 - Commit messages: - Update second method. - 8264609: Number.shortValue spec should use @implSpec Changes: https://git.openjdk.java.net/jdk/pull/3314/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3314=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264610 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3314.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3314/head:pull/3314 PR: https://git.openjdk.java.net/jdk/pull/3314
Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
On Thu, 1 Apr 2021 11:48:21 GMT, Jim Laskey wrote: > We should never close the jimage since java threads can still be running > after a hard exit(). LGTM. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3304
RFR: 8264288: Performance issue with MethodHandle.asCollector
This patch speeds up MethodHandle.asCollector handles where the array type is not Object[], as well as speeding up all collectors where the arity is greater than 10. The old code is creating a collector handle by combining a set of hard coded methods for collecting arguments into an Object[], up to a maximum of ten elements, and then copying this intermediate array into a final array. In principle, it shouldn't matter how slow (or fast) this handle is, because it should be replaced by existing bytecode intrinsification which does the right thing. But, through investigation it turns out that the intrinsification is only being applied in a very limited amount of cases: Object[] with max 10 elements only, only for the intermediate collector handles. Every other collector shape incurs overhead because it essentially ends up calling the ineffecient fallback handle. Rather than sticking on a band aid, the new code replaces the existing implementation with a collector handle implemented using a LambdaForm, which removes the need for intrinsification, and also greatly reduces code-complexity of the implementation. (I plan to expose this collector using a public API in the future as well, so users don't have to go through MHs::identity to make a collector). The old code was also using a special lambda form transform for collecting arguments into an array. I believe this was done to take advantage of the existing-but-limited bytecode intrinsification, at least for Object[] with less than 10 elements. The new code just uses the existing collect arguments transform with the newly added collector handle as filter, and this works just as well for the existing case, but as a bonus is also much simpler, since no separate transform is needed. Using the collect arguments transform should also improve sharing. As a result of these changes a lot of code was unused and has been removed in this patch. Testing: tier 1-3, benchmarking using TypedAsCollector (part of the patch), as well as another variant of the benchmark that used a declared static identity method instead of MHs::identity (not included). Before/after comparison of MethodHandleAs* benchmarks (no differences there). Here are some numbers from the added benchmark: Before: BenchmarkMode CntScoreError Units TypedAsCollector.testIntCollect avgt 30 189.156 � 1.796 ns/op TypedAsCollector.testIntCollectHighArity avgt 30 660.549 � 10.159 ns/op TypedAsCollector.testObjectCollect avgt 307.092 � 0.042 ns/op TypedAsCollector.testObjectCollectHighArity avgt 30 65.225 � 0.546 ns/op TypedAsCollector.testStringCollect avgt 30 28.511 � 0.243 ns/op TypedAsCollector.testStringCollectHighArity avgt 30 57.054 � 0.635 ns/op (as you can see, just the Object[] with arity less than 10 case is fast here) After: BenchmarkMode Cnt Score Error Units TypedAsCollector.testIntCollect avgt 30 6.569 � 0.131 ns/op TypedAsCollector.testIntCollectHighArity avgt 30 8.923 � 0.066 ns/op TypedAsCollector.testObjectCollect avgt 30 6.813 � 0.035 ns/op TypedAsCollector.testObjectCollectHighArity avgt 30 9.718 � 0.066 ns/op TypedAsCollector.testStringCollect avgt 30 6.737 � 0.016 ns/op TypedAsCollector.testStringCollectHighArity avgt 30 9.618 � 0.052 ns/op Thanks, Jorn - Commit messages: - WIP - clean up - Completely implement collect in lambdaform - WIP - fix MH collector perf, with jumbo arity bandaid Changes: https://git.openjdk.java.net/jdk/pull/3306/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3306=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264288 Stats: 513 lines in 8 files changed: 154 ins; 337 del; 22 mod Patch: https://git.openjdk.java.net/jdk/pull/3306.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3306/head:pull/3306 PR: https://git.openjdk.java.net/jdk/pull/3306
Integrated: 8169629: Annotations with lambda expressions cause AnnotationFormatError
On Wed, 31 Mar 2021 21:32:35 GMT, Joe Darcy wrote: > The stricter checks added by > 8035781: Improve equality for annotations > in creating the proxy objects used to implement annotations has an unintended > by-catch of rejecting annotation's whose type has, say, a field initialized > with a lambda expression. While uncommon, it is legal code to have a field in > an annotation type. > > The updated checks skip over the sort of synthetic method used for the > initialization. > > Some different compilation tactics were used before and after nest mates, so > the test includes compilation and testing under both situations. This pull request has now been integrated. Changeset: 328e9514 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/328e9514 Stats: 27 lines in 2 files changed: 23 ins; 0 del; 4 mod 8169629: Annotations with lambda expressions cause AnnotationFormatError Reviewed-by: jfranck - PR: https://git.openjdk.java.net/jdk/pull/3294
Re: RFR: 8169629: Annotations with lambda expressions cause AnnotationFormatError [v2]
> The stricter checks added by > 8035781: Improve equality for annotations > in creating the proxy objects used to implement annotations has an unintended > by-catch of rejecting annotation's whose type has, say, a field initialized > with a lambda expression. While uncommon, it is legal code to have a field in > an annotation type. > > The updated checks skip over the sort of synthetic method used for the > initialization. > > Some different compilation tactics were used before and after nest mates, so > the test includes compilation and testing under both situations. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3294/files - new: https://git.openjdk.java.net/jdk/pull/3294/files/818df21d..db79e3c8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3294=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3294=00-01 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3294.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3294/head:pull/3294 PR: https://git.openjdk.java.net/jdk/pull/3294
Re: RFR: 8169629: Annotations with lambda expressions cause AnnotationFormatError [v2]
On Thu, 1 Apr 2021 12:36:56 GMT, Joel Borggrén-Franck
wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Respond to review feedback.
>
> src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
> line 497:
>
>> 495: Method currentMethod = null;
>> 496: for(Method method : memberMethods) {
>> 497: currentMethod = method;
>
> I can't see any use of currentMethod, am I missing something?
Remnant of a second fix I pulled out for separate handling; I'll remove before
pushing. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/3294
Re: RFR: 8203359: Container level resources events
On Fri, 26 Mar 2021 11:05:56 GMT, Severin Gehwolf wrote: >> Does each getter call result in parsing /proc, or do things aggregated over >> several calls or hooks? >> >> Do you have any data how expensive the invocations are? >> >> You could for example try to measure it by temporary making the events >> durational, and fetch the values between begin() and end(), and perhaps show >> a 'jfr print --events Container* recording.jfr' printout. >> >> If possible, it would be interesting to get some idea about the startup cost >> as well >> >> If not too much overhead, I think it would be nice to skip the "flag" in the >> .jfcs, and always record the events in a container environment. >> >> I know there is a way to test JFR using Docker, maybe @mseledts could >> provide information? Some sanity tests would be good to have. > >> Does each getter call result in parsing /proc, or do things aggregated over >> several calls or hooks? > > From the looks of it the event emitting code uses `Metrics.java` interface > for retrieving the info. Each call to a method exposed by Metrics result in > file IO on some cgroup (v1 or v2) interface file(s) in `/sys/fs/...`. I don't > see any aggregation being done. > > On the hotspot side, we implemented some caching for frequent calls > (JDK-8232207, JDK-8227006), but we didn't do that yet for the Java side since > there wasn't any need (so far). If calls are becoming frequent with this it > should be reconsidered. > > So +1 on getting some data on what the perf penalty of this is. Thanks to all for chiming in! I have added the tests to `test/hotspot/jtreg/containers/docker/TestJFREvents.java` where there already were some templates for the container event data. As for the performance - as expected, extracting the data from `/proc` is not exactly cheap. On my test c5.4xlarge instance I am getting an average wall-clock time to generate the usage/throttling events (one instance of each) of ~15ms. I would argue that 15ms per 30s (the default emission period for those events) might be acceptable to start with. Caching of cgroups parsed data would help if the emission period is shorter than the cache TTL. This is exacerbated by the fact that (almost) each container event type requires data from a different cgroups control file - hence the data will not be shared between the event type instances even if cached. Realistically, caching benefits would become visible only for sub-second emission periods. If the caching is still required I would suggest having a follow up ticket just for that - it will require setting up some benchmarks to justify the changes that would need to be done in the metrics implementation. - PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8264048: Fix caching in Jar URL connections when an entry is missing [v2]
On Wed, 31 Mar 2021 12:47:50 GMT, Aleksei Efimov wrote: >> Current fix tries to tackle an issue with URL connection referencing >> non-existing Jar file entries: >> If an entry that doesn't exist is specified in an URL connection the >> underlying Jar file is still cached even if an exception is thrown after >> that. Such behavior prevents the caller, for instance, a `URLClassLoader`, >> from closing a Jar file. >> >> The proposed fix checks if entry exists before caching a Jar file (only for >> cases with enabled caching): >> - If entry exists - jar file is cached if it wasn't cached before >> - If entry doesn't exist and jar file wasn't cached before - jar file is >> closed and exception is thrown >> - If entry doesn't exist and jar file was cached before - jar file is kept >> cached and exception is thrown >> >> >> The following tests have been used to verify the fix: >> - New regression tests >> - ``:jdk_core:`` tests >> - `api/java_util`,`api/java_net` JCK tests > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8264048: Remove old version of RemoveJar test Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3263
Re: RFR: 8203359: Container level resources events [v4]
> With this change it becomes possible to surface various cgroup level metrics > (available via `jdk.internal.platform.Metrics`) as JFR events. > > Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is > turned into JFR events to start with. > * CPU related metrics > * Memory related metrics > * I/O related metrics > > For each of those subsystems a configuration data will be emitted as well. > The initial proposal is to emit the configuration data events at least once > per chunk and the metrics values at 30 seconds interval. > By using these values the emitted events seem to contain useful information > without increasing overhead (the metrics values are read from `/proc` > filesystem so that should not be done too frequently). Jaroslav Bachorik has updated the pull request incrementally with two additional commits since the last revision: - Remove unused test files - Initial test support for JFR container events - Changes: - all: https://git.openjdk.java.net/jdk/pull/3126/files - new: https://git.openjdk.java.net/jdk/pull/3126/files/82570022..79c91f57 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3126=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3126=02-03 Stats: 117 lines in 4 files changed: 99 ins; 6 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/3126.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3126/head:pull/3126 PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8264109: Add vectorized implementation for VectorMask.andNot() [v2]
On Thu, 1 Apr 2021 03:39:43 GMT, Xiaohong Gong wrote:
>> Currently "VectorMask.andNot()" is not vectorized:
>> public VectorMask andNot(VectorMask m) {
>> // FIXME: Generate good code here.
>> return bOp(m, (i, a, b) -> a && !b);
>> }
>> This can be implemented with` "and(m.not())" `directly. Since
>> `"VectorMask.and()/not()" `have been vectorized in hotspot, `"andNot"`
>> can be vectorized as well by calling them.
>>
>> The performance gain is >100% for such a simple JMH:
>> @Benchmark
>> public Object andNot(Blackhole bh) {
>> boolean[] mask = fm.apply(SPECIES.length());
>> boolean[] r = fmt.apply(SPECIES.length());
>> VectorMask rm = VectorMask.fromArray(SPECIES, r, 0);
>>
>> for (int ic = 0; ic < INVOC_COUNT; ic++) {
>> for (int i = 0; i < mask.length; i += SPECIES.length()) {
>> VectorMask vmask = VectorMask.fromArray(SPECIES, mask, i);
>> rm = rm.andNot(vmask);
>> }
>> }
>> return rm;
>> }
>
> Xiaohong Gong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move the changing to AbstractMask.andNot and revert changes in VectorMask
>
> Change-Id: Ie3ec8f53cb24526c8e1ccc68038355d024910818
> CustomizedGitHooks: yes
Looks good.
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3211
Re: RFR: 8173970: jar tool should have a way to extract to a directory [v3]
On Wed, 31 Mar 2021 17:22:55 GMT, Lance Andersen wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Alan's review feedback for -C help text
>> - Keep -xfP backward compatible but don't allow -C/--dir with -xfP
>
> Some additional comments basically suggesting that we test --extract in
> addition to -x
Thank you for the reviews, Lance. The latest version of this PR has taken into
account most of these comments. There's one review comment which hasn't
resulted in a change and I've added a reply to that review comment explaining
my thoughts.
> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1427:
>
>> 1425: return rc;// leading '/' or 'dot-dot' only path
>> 1426: }
>> 1427: File f = new File(xdestDir, name.replace('/',
>> File.separatorChar));
>
> Could you please add a comment regarding xdestDir and also correct the typo
> on line 1418 'requres' -> 'requires'
Fixed the typo and also added code comment about the `xdestDir` usage and
semantics. If the new comment needs clarification/changes, do let me know.
> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 62:
>
>> 60: Could not create a temporary file
>> 61: error.extract.multiple.dest.dir=\
>> 62: You may not specify more than one directory for extracting the
>> jar
>
> Perhaps something similar to:
>
> You may not specify the '-C' or '--dir' option more than once with the '-x'
> option
Done
> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 64:
>
>> 62: You may not specify more than one directory for extracting the
>> jar
>> 63: error.extract.pflag.not.allowed=\
>> 64: -P option cannot be used when extracting a jar to a specific
>> location
>
> Perhaps something similar to
>
> You may not specify '-Px' with the '-C' or '--dir' options
Makes sense. I've updated the PR with this change.
> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 167:
>
>> 165: (in = {0}) (out= {1})
>> 166: out.extract.dir=\
>> 167: extracting to {0}
>
> Perhaps 'Extracting to directory: {0}'
Updated the message to say `extracting to directory: {0}`. I decided to use
lower case for `extracting` to keep it consistent with other similar messages
that get logged here.
> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 249:
>
>> 247: \ -C DIR Change to the specified directory and
>> include the\n\
>> 248: \ following file. When used in extract
>> mode, extracts\n\
>> 249: \ the jar to the specified directory
>
> Should this be updated on line 187 as well in the compatibility mode section?
Updated in latest version of the PR.
> test/jdk/tools/jar/JarExtractTest.java line 152:
>
>> 150: return abs;
>> 151: }
>> 152:
>
> Please add a comment to each test giving a high level overview of what it
> does as it will help future maintainers
Done in latest update to this PR
> test/jdk/tools/jar/JarExtractTest.java line 175:
>
>> 173: final String dest = "foo-bar";
>> 174: System.out.println("Extracting " + testJarPath + " to " + dest);
>> 175: final int exitCode = JAR_TOOL.run(System.out, System.err, "-x",
>> "-f", testJarPath.toString(),
>
> Perhaps add a DataProvider so you can test --extract as well?
Not a data provider but the latest version of PR tests --extract as well
> test/jdk/tools/jar/JarExtractTest.java line 216:
>
>> 214: final Path jarPath = createJarWithPFlagSemantics();
>> 215: // extract with -P flag without any explicit destination
>> directory (expect the extraction to work fine)
>> 216: final String[] args = new String[]{"-xvfP", jarPath.toString()};
>
> Perhaps add a DataProvider so you can test --extract as well?
Not a data provider but the latest version of PR tests --extract as well
> test/jdk/tools/jar/JarExtractTest.java line 239:
>
>> 237: */
>> 238: @Test
>> 239: public void testExtractWithDirPFlagNotAllowed() throws Exception {
>
> I would include --extract in your testing options
Done in latest version of the PR
> test/jdk/tools/jar/JarExtractTest.java line 240:
>
>> 238: @Test
>> 239: public void testExtractWithDirPFlagNotAllowed() throws Exception {
>> 240: final String expectedErrMsg = "-P option cannot be used when
>> extracting a jar to a specific location";
>
> Probably need to add a comment that this must match the entry in
> jar.properties
Added a comment mentioning where this message is sourced from.
> test/jdk/tools/jar/JarExtractTest.java line 247:
>
>> 245: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(),
>> "-P", "-C", "."});
>> 246: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(),
>> "-P", "--dir", "."});
>>
Re: RFR: 8173970: jar tool should have a way to extract to a directory [v4]
> Can I please get a review for this patch which proposes to implement the > enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970? > > The commit in this PR introduces the `-o` and `--output-dir` option to the > `jar` command. The option takes a path to a destination directory as a value > and extracts the contents of the jar into that directory. This is an optional > option and the changes in the commit continue to maintain backward > compatibility where the jar is extracted into current directory, if no `-o` > or `--output-dir` option has been specified. > > As far as I know, there hasn't been any discussion on what the name of this > new option should be. I was initially thinking of using `-d` but that is > currently used by the `jar` command for a different purpose. So I decided to > use `-o` and `--output-dir`. This is of course open for change depending on > any suggestions in this PR. > > The commit in this PR also updates the `jar.properties` file which contains > the English version of the jar command's `--help` output. However, no changes > have been done to the internationalization files corresponding to this one > (for example: `jar_de.properties`), because I don't know what process needs > to be followed to have those files updated (if at all they need to be > updated). > > The commit also includes a jtreg testcase which verifies the usage of this > new option. Jaikiran Pai has updated the pull request incrementally with seven additional commits since the last revision: - Lance's review - include tests for --extract long form option - cleanup after each test - Adjust test for new error messages - Lance's review - add a code comment for xdestDir - Lance's review - updates to the help messages in jar.properties - Lance's review - add comment to the magic number 6 in the tests - Lance's review - add comments to test methods - Changes: - all: https://git.openjdk.java.net/jdk/pull/2752/files - new: https://git.openjdk.java.net/jdk/pull/2752/files/3df602d2..b5de6e3d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2752=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2752=02-03 Stats: 139 lines in 3 files changed: 98 ins; 0 del; 41 mod Patch: https://git.openjdk.java.net/jdk/pull/2752.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2752/head:pull/2752 PR: https://git.openjdk.java.net/jdk/pull/2752
Re: RFR: JDK-8264403: [macos]: App names containing '.' characters results in an error message when launching [v3]
On Thu, 1 Apr 2021 12:19:21 GMT, Andy Herrick wrote: >> Deriving the cfg file name is broken on mac and linux when the application >> name has a "." in it. > > Andy Herrick has updated the pull request incrementally with one additional > commit since the last revision: > > Fix trailing whitespace Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3288
Re: RFR: 8037397: Lost nested character class after intersection &
On Wed, 31 Mar 2021 20:38:33 GMT, Ian Graves wrote: > Bug fix with the intersection `&&` operator in regex patterns. In > JDK-8037397, some character classes on the right hand side of the operator > are dropped in cases where nested `[..]` classes are used with non "nested" > ones. Please add a test. - PR: https://git.openjdk.java.net/jdk/pull/3291
Re: RFR: 8169629: Annotations with lambda expressions cause AnnotationFormatError
On Wed, 31 Mar 2021 21:32:35 GMT, Joe Darcy wrote:
> The stricter checks added by
> 8035781: Improve equality for annotations
> in creating the proxy objects used to implement annotations has an unintended
> by-catch of rejecting annotation's whose type has, say, a field initialized
> with a lambda expression. While uncommon, it is legal code to have a field in
> an annotation type.
>
> The updated checks skip over the sort of synthetic method used for the
> initialization.
>
> Some different compilation tactics were used before and after nest mates, so
> the test includes compilation and testing under both situations.
Other than the potentially unused var, looks good to me.
src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
line 497:
> 495: Method currentMethod = null;
> 496: for(Method method : memberMethods) {
> 497: currentMethod = method;
I can't see any use of currentMethod, am I missing something?
-
Marked as reviewed by jfranck (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3294
RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
We should never close the jimage since java threads can still be running after a hard exit(). - Commit messages: - 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28 Changes: https://git.openjdk.java.net/jdk/pull/3304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3304=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8166727 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3304/head:pull/3304 PR: https://git.openjdk.java.net/jdk/pull/3304
Re: RFR: JDK-8264403: [macos]: App names containing '.' characters results in an error message when launching [v3]
> Deriving the cfg file name is broken on mac and linux when the application > name has a "." in it. Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: Fix trailing whitespace - Changes: - all: https://git.openjdk.java.net/jdk/pull/3288/files - new: https://git.openjdk.java.net/jdk/pull/3288/files/25102702..b83578c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3288=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3288=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3288.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3288/head:pull/3288 PR: https://git.openjdk.java.net/jdk/pull/3288
Re: RFR: JDK-8264403: [macos]: App names containing '.' characters results in an error message when launching [v2]
> Deriving the cfg file name is broken on mac and linux when the application > name has a "." in it. Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8264403: [macos]: App names containing '.' characters results in an error message when launching - Changes: - all: https://git.openjdk.java.net/jdk/pull/3288/files - new: https://git.openjdk.java.net/jdk/pull/3288/files/1fe2699e..25102702 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3288=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3288=00-01 Stats: 12 lines in 4 files changed: 0 ins; 2 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3288.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3288/head:pull/3288 PR: https://git.openjdk.java.net/jdk/pull/3288
Re: RFR: JDK-8264403: [macos]: App names containing '.' characters results in an error message when launching [v2]
On Wed, 31 Mar 2021 21:59:55 GMT, Alexey Semenyuk wrote:
>> Andy Herrick has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> JDK-8264403: [macos]: App names containing '.' characters results in an
>> error message when launching
>
> src/jdk.jpackage/share/native/common/FileUtils.h line 76:
>
>> 74:
>> 75: // extract the name from the launcher path
>> 76: tstring extractName(const tstring& path);
>
> Function name seems to be misleading.
> If it specifically designed to be applied to executables, I'd name it
> `stripExecutableSuffix()`. On Unix implementation would return passed in
> string as is without calling `basename()` and on Windows would just strip
> ".exe" suffix not calling `basename()` either.
> The use would be: `FileUtils::mkpath() << appDirPath <<
> (FileUtils::stripExecutableSuffix(FileUtils::basename(launcherPath)) +
> _T(".cfg"));`
makes sense - updated as requested
-
PR: https://git.openjdk.java.net/jdk/pull/3288
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v40]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 76 commits: - Merge branch 'master' into 8248862 - Correct return type of RandomGeneratorFactory.of - Merge branch 'master' into 8248862 - CSR review revisions - CSR review updates - Removed @since from nextDouble ThreadLocalRandom - RandomGeneratorFactory::all(Class category) @implNote was out of date - Clarify all() - Cleaned up ints(), longs(), doubles() - Merge branch 'master' into 8248862 - ... and 66 more: https://git.openjdk.java.net/jdk/compare/011f6d13...5a23b4f1 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=39 Stats: 12968 lines in 26 files changed: 11202 ins; 1588 del; 178 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: Proposal for Decimal64 and Decimal128 value-based classes
On 31/03/2021 23:54, Dan Smith wrote: On Mar 31, 2021, at 3:27 PM, Maurizio Cimadamore wrote: What I'd be curious though, is if the @ValueBased annotation could be enhanced to say _which_ primitive class default you want (and then javac could enforce extra checks if you pick the "val" default). If something like this was feasible (cc'ing Dan), maybe some of the friction here could be removed? You mean annotate a class with "pretend this class's name represents a value type" and then implement the associated null checks in javac, even though we don't actually have value types yet? I'd expect that to run into a number of problems related to the fact that the language model hasn't actually been updated to include primitive classes or value types yet. Plus the lack of features like reference types (Foo.ref) would be limiting for programmers who need them. Plus binary incompatibility—value types need special encoding in class files, and those class files aren't legal yet; when they are, you risk mismatches. In this case I think the straightforward approach of just completing and delivering the Valhalla features is better than trying to spin off a small taste of them early. You are right - there are more aspects to this than just null checks - binary compatibility seems especially problematic. Thanks for the explanation! Maurizio
