Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 22:44:10 GMT, David Holmes wrote: > > I think this is not deliberate. Since `rawAnnotations` may end up having > > any of the following: > > - `RuntimeVisibleAnnotations` (when there were just `RUNTIME` annotation > > usages compiled into the class or `-XX+PreserveAllAnnotations` was not used > > at runtime) > > - `RuntimeInvisibleAnnotations` (when there were just `CLASS` annotation > > usages compiled into the class and `-XX+PreserveAllAnnotations` was used at > > runtime) > > - `RuntimeVisibleAnnotations + RuntimeInvisibleAnnotations` (when there > > were `RUNTIME` and `CLASS` annotation usages compiled into the class and > > `-XX+PreserveAllAnnotations` was used at runtime) > > So why would `RuntimeInvisibleAnnotations` be skipped in 3rd case but not > > in 2nd case? > > Well in the second case there is no way to know it is looking only at > invisible annotations, so it has to read them and then discard them as > they were in fact CLASS retention. The skipping could be seen as an > optimization. Not all annotations in the 2nd case need to be CLASS retention. They were CLASS retention when the class that uses them was compiled, but at runtime, the retention may be different (separate compilation). So they are actually returned by the reflection in that case. > > The code is confusing because it gives no indication that it is aware > that runtime invisible annotations could be present: > > /** > * Parses the annotations described by the specified byte array. > * resolving constant references in the specified constant pool. > * The array must contain an array of annotations as described > * in the RuntimeVisibleAnnotations_attribute: > > but at the same time it checks for the RUNTIME retention, which means it > must have recognised the possibility there could be something else > there. Yes, there could be a CLASS retention annotation there (even though `-XX+PreserveAllAnnotations` was not used at runtime, so rawAnnotations contains the content of `RuntimeVisibleAnnotations` only). Again, such annotation was RUNTIME retention when its use was compiled into a class, but at runtime such annotation may be updated to have CLASS or even SOURCE retention. Such annotation use is filtered out. > That check is day 2 code though, on day 1 there was this comment: > > /* XXX Uncomment when this meta-attribute on meta-attributes (Neal's > putback) > if (type.retention() == VisibilityLevel.RUNTIME) > XXX */ I guess this was just code in creation before release. At some point, the `RetentionPolicy` enum was added, but above comment refers to it by the name `VisibilityLevel` > > But the end result is that the code in AnnotationParser correctly > filters out all non-RUNTIME annotations, either directly, or by skipping > them in the stream in the first place. So in that sense there is no bug, > but the code could do with some additional comments. The problem is that it sometimes skips RUNTIME annotations too. I consider this a bug. > > Cheers, > David Regard, Peter - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: 8267630: Start of release updates for JDK 18
On Mon, 24 May 2021 22:35:04 GMT, Joe Darcy wrote: > 8267630: Start of release updates for JDK 18 Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4175
Re: RFR: 8267630: Start of release updates for JDK 18
On Mon, 24 May 2021 22:35:04 GMT, Joe Darcy wrote: > 8267630: Start of release updates for JDK 18 JDK 18 is right around the corner, time for the usual start-of-release changeset for review. Typical structure for these kinds of changes. I'll update the symbol file information of JDK 17 b25 after that build is promoted. Usual updates to SourceVersion, javac, HotSpot, and version-oriented tests. Please also review the related CSRs: JDK-8268156: Add SourceVersion.RELEASE_18 https://bugs.openjdk.java.net/browse/JDK-8268156 JDK-8268157: Add source 18 and target 18 to javac https://bugs.openjdk.java.net/browse/JDK-8268157 - PR: https://git.openjdk.java.net/jdk/pull/4175
RFR: 8267630: Start of release updates for JDK 18
8267630: Start of release updates for JDK 18 - Commit messages: - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Merge branch 'master' into 8267630 - Clean langtools test run. - ... and 1 more: https://git.openjdk.java.net/jdk/compare/ef01e478...9c7c88bf Changes: https://git.openjdk.java.net/jdk/pull/4175/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4175&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267630 Stats: 4946 lines in 62 files changed: 4904 ins; 0 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/4175.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4175/head:pull/4175 PR: https://git.openjdk.java.net/jdk/pull/4175
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v11]
On Wed, 2 Jun 2021 02:32:54 GMT, Yi Yang wrote: >> The JDK codebase re-created many variants of checkIndex(`grep -I -r >> 'cehckIndex' jdk/`). A notable variant is java.nio.Buffer.checkIndex, which >> annotated with @IntrinsicCandidate and it only has a corresponding C1 >> intrinsic version. >> >> In fact, there is an utility method >> `jdk.internal.util.Preconditions.checkIndex`(wrapped by >> java.lang.Objects.checkIndex) that behaves the same as these variants of >> checkIndex, we can replace these re-created variants of checkIndex by >> Objects.checkIndex, it would significantly reduce duplicated code and enjoys >> performance improvement because Preconditions.checkIndex is >> @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot. >> >> But, the problem is currently HotSpot only implements the C2 version of >> Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we >> can firstly implement its C1 counterpart. There are also a few kinds of >> stuff we can do later: >> >> 1. Replace all variants of checkIndex by Objects.checkIndex in the whole JDK >> codebase. >> 2. Remove Buffer.checkIndex and obsolete/deprecate InlineNIOCheckIndex flag >> >> Testing: cds, compiler and jdk > > Yi Yang has updated the pull request with a new target base due to a merge or > a rebase. The pull request now contains eight commits: > > - x86_32 fails > - build failed > - cmp clobbers its left argument on x86_32 > - better check1-4 > - AssertionError when expected exception was not thrown > - remove InlineNIOCheckIndex flag > - remove java_nio_Buffer in javaClasses.hpp > - consolidate `test/jdk/java/util/Objects/CheckIndex.java` and `test/jdk/java/util/Objects/CheckLongIndex.java` started failing. Please take a look. test CheckIndex.testCheckIndex(0, -2147483647, false): failure java.lang.AssertionError: Index 0 is out of bounds of [0, -2147483647), but was reported to be within bounds at org.testng.Assert.fail(Assert.java:94) at CheckIndex.lambda$testCheckIndex$3(CheckIndex.java:103) at CheckIndex.testCheckIndex(CheckIndex.java:117) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:85) at org.testng.internal.Invoker.invokeMethod(Invoker.java:639) at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:821) at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1131) at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:124) at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108) at org.testng.TestRunner.privateRun(TestRunner.java:773) at org.testng.TestRunner.run(TestRunner.java:623) at org.testng.SuiteRunner.runTest(SuiteRunner.java:357) at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:352) at org.testng.SuiteRunner.privateRun(SuiteRunner.java:310) at org.testng.SuiteRunner.run(SuiteRunner.java:259) at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52) at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86) at org.testng.TestNG.runSuitesSequentially(TestNG.java:1185) at org.testng.TestNG.runSuitesLocally(TestNG.java:1110) at org.testng.TestNG.run(TestNG.java:1018) at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298) at java.base/java.lang.Thread.run(Thread.java:831) test CheckIndex.testCheckIndex(0, -2147483648, false): failure java.lang.AssertionError: Index 0 is out of bounds of [0, -2147483648), but was reported to be within bounds at org.testng.Assert.fail(Assert.java:94) at CheckIndex.lambda$testCheckIndex$3(CheckIndex.java:103) at CheckIndex.testCheckIndex(CheckIndex.java:120) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImp
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Thu, 3 Jun 2021 03:23:13 GMT, Brian Goetz wrote: > reasonable options are to either leave it alone, deprecate it, or engage > in a much more deliberate redesign.? My favorite option is to leave it alone and test it a bit with the test already provided in this PR. That however requires one approval, me to type `/integrate` and someone to type `/sponsor` - if my understanding of the skara bot is correct. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: 8268077: java.util.List missing from Collections Framework Overview
On Tue, 1 Jun 2021 19:51:39 GMT, Raffaello Giulietti wrote: > Trivial changes to this non-normative document. Marked as reviewed by smarks (Reviewer). Looks good. Thanks for noticing and fixing this! - PR: https://git.openjdk.java.net/jdk/pull/4289
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v2]
On Thu, 3 Jun 2021 03:09:58 GMT, Nick Gasson wrote: >> macOS on Apple silicon uses slightly different ABI conventions to the >> standard AArch64 ABI. The differences are outlined in [1]. In >> particular in the standard (AAPCS) ABI, variadic arguments may be passed >> in either registers or on the stack following the normal calling >> convention. To handle this, va_list is a struct containing separate >> pointers for arguments located in integer registers, floating point >> registers, and on the stack. Apple's ABI simplifies this by passing all >> variadic arguments on the stack and the va_list type becomes a simple >> char* pointer. >> >> This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to >> represent the new ABI variant on macOS. StackVaList is based on >> WinVaList lightly modified to handle the different TypeClasses on >> AArch64. The original AArch64Linker is renamed to AapcsLinker and is >> currently used for all non-Mac platforms. I think we also need to add a >> WinAArch64 CABI but I haven't yet been able to test on a Windows system >> so will do that later. >> >> The macOS ABI also uses a different method of spilling arguments to the >> stack (the standard ABI pads each argument to a multiple of 8 byte stack >> slots, but the Mac ABI packs arguments according to their natural >> alignment). None of the existing tests exercise this so I'll open a new >> JBS issue and work on that separately. >> >> Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. >> >> [1] >> https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms > > Nick Gasson has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Fixes after JEP integratioN > >Change-Id: Iaa13b3869522c8814c3f7ef4c1eac8e8267657e6 >CustomizedGitHooks: yes > - merge master > >Change-Id: Ic06fec084099ff2053dd251a255cbbf4a64a59d7 >CustomizedGitHooks: yes > - 8263512: [macos_aarch64] issues with calling va_args functions from > invoke_native > >macOS on Apple silicon uses slightly different ABI conventions to the >standard AArch64 ABI. The differences are outlined in [1]. In >particular in the standard (AAPCS) ABI, variadic arguments may be passed >in either registers or on the stack following the normal calling >convention. To handle this, va_list is a struct containing separate >pointers for arguments located in integer registers, floating point >registers, and on the stack. Apple's ABI simplifies this by passing all >variadic arguments on the stack and the va_list type becomes a simple >char* pointer. > >This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to >represent the new ABI variant on macOS. StackVaList is based on >WinVaList lightly modified to handle the different TypeClasses on >AArch64. The original AArch64Linker is renamed to AapcsLinker and is >currently used for all non-Mac platforms. I think we also need to add a >WinAArch64 CABI but I haven't yet been able to test on a Windows system >so will do that later. > >The macOS ABI also uses a different method of spilling arguments to the >stack (the standard ABI pads each argument to a multiple of 8 byte stack >slots, but the Mac ABI packs arguments according to their natural >alignment). None of the existing tests exercise this so I'll open a new >JBS issue and work on that separately. > >Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. I've rebased this on the latest master after the JEP integration and fixed the issues above. Should be good to re-review. I've done the suggested renaming to LinuxAArch64Linker, MacOsAArch64Linker, etc. - PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v3]
> macOS on Apple silicon uses slightly different ABI conventions to the > standard AArch64 ABI. The differences are outlined in [1]. In > particular in the standard (AAPCS) ABI, variadic arguments may be passed > in either registers or on the stack following the normal calling > convention. To handle this, va_list is a struct containing separate > pointers for arguments located in integer registers, floating point > registers, and on the stack. Apple's ABI simplifies this by passing all > variadic arguments on the stack and the va_list type becomes a simple > char* pointer. > > This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to > represent the new ABI variant on macOS. StackVaList is based on > WinVaList lightly modified to handle the different TypeClasses on > AArch64. The original AArch64Linker is renamed to AapcsLinker and is > currently used for all non-Mac platforms. I think we also need to add a > WinAArch64 CABI but I haven't yet been able to test on a Windows system > so will do that later. > > The macOS ABI also uses a different method of spilling arguments to the > stack (the standard ABI pads each argument to a multiple of 8 byte stack > slots, but the Mac ABI packs arguments according to their natural > alignment). None of the existing tests exercise this so I'll open a new > JBS issue and work on that separately. > > Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. > > [1] > https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms Nick Gasson has updated the pull request incrementally with one additional commit since the last revision: No variadic upcalls Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5 CustomizedGitHooks: yes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3617/files - new: https://git.openjdk.java.net/jdk/pull/3617/files/645ccb3d..251aae68 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3617&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3617&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3617/head:pull/3617 PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
It seems pretty clear that this "feature" is a leftover from an early implementation, doesn't clearly say what it is supposed to do, is more complicated than it looks, and is buggily implemented. While I understand the temptation to "fix" it, at this point we'd realistically be adding a basically entirely new reflection feature that hasn't gone through any sort of design analysis -- we'd just be guessing about what the original intent of this vestigial feature is. It seems the reasonable options are to either leave it alone, deprecate it, or engage in a much more deliberate redesign. (And given the fact that I can think of at least 1,000 things that are higher priority, I'd not be inclined to pursue the third option.) On 6/2/2021 11:06 PM, Jaroslav Tulach wrote: On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. FYI: The current test is a "mirror" copy of [AnnotationTypeRuntimeAssumptionTest.java](https://github.com/openjdk/jdk/blob/0f689ea8f29be113cc8a917a86fd9e0f85f921fc/test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeRuntimeAssumptionTest.java) with adjustments to cover the `CLASS`->`RUNTIME` migration. What I would do is to add a patch for the parser bug That's an interesting discovery! I see a patch and I can include it as your commit in this PR, if there is a test... and then extend the test in 3 dimensions: ...or just take my PR and expand it. Also the checkbox _"Allow edits and access to secrets by maintainers"_ is on - e.g. this PR is open for modifications. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v2]
> macOS on Apple silicon uses slightly different ABI conventions to the > standard AArch64 ABI. The differences are outlined in [1]. In > particular in the standard (AAPCS) ABI, variadic arguments may be passed > in either registers or on the stack following the normal calling > convention. To handle this, va_list is a struct containing separate > pointers for arguments located in integer registers, floating point > registers, and on the stack. Apple's ABI simplifies this by passing all > variadic arguments on the stack and the va_list type becomes a simple > char* pointer. > > This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to > represent the new ABI variant on macOS. StackVaList is based on > WinVaList lightly modified to handle the different TypeClasses on > AArch64. The original AArch64Linker is renamed to AapcsLinker and is > currently used for all non-Mac platforms. I think we also need to add a > WinAArch64 CABI but I haven't yet been able to test on a Windows system > so will do that later. > > The macOS ABI also uses a different method of spilling arguments to the > stack (the standard ABI pads each argument to a multiple of 8 byte stack > slots, but the Mac ABI packs arguments according to their natural > alignment). None of the existing tests exercise this so I'll open a new > JBS issue and work on that separately. > > Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. > > [1] > https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms Nick Gasson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Fixes after JEP integratioN Change-Id: Iaa13b3869522c8814c3f7ef4c1eac8e8267657e6 CustomizedGitHooks: yes - merge master Change-Id: Ic06fec084099ff2053dd251a255cbbf4a64a59d7 CustomizedGitHooks: yes - 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native macOS on Apple silicon uses slightly different ABI conventions to the standard AArch64 ABI. The differences are outlined in [1]. In particular in the standard (AAPCS) ABI, variadic arguments may be passed in either registers or on the stack following the normal calling convention. To handle this, va_list is a struct containing separate pointers for arguments located in integer registers, floating point registers, and on the stack. Apple's ABI simplifies this by passing all variadic arguments on the stack and the va_list type becomes a simple char* pointer. This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to represent the new ABI variant on macOS. StackVaList is based on WinVaList lightly modified to handle the different TypeClasses on AArch64. The original AArch64Linker is renamed to AapcsLinker and is currently used for all non-Mac platforms. I think we also need to add a WinAArch64 CABI but I haven't yet been able to test on a Windows system so will do that later. The macOS ABI also uses a different method of spilling arguments to the stack (the standard ABI pads each argument to a multiple of 8 byte stack slots, but the Mac ABI packs arguments according to their natural alignment). None of the existing tests exercise this so I'll open a new JBS issue and work on that separately. Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. - Changes: https://git.openjdk.java.net/jdk/pull/3617/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3617&range=01 Stats: 777 lines in 14 files changed: 571 ins; 115 del; 91 mod Patch: https://git.openjdk.java.net/jdk/pull/3617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3617/head:pull/3617 PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: > There doesn't seem to be much support for the complete changes in #4245. To > get at least something useful from that endeavor I have extracted the test > for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it > in this pull request without any changes to the JVM behavior. FYI: The current test is a "mirror" copy of [AnnotationTypeRuntimeAssumptionTest.java](https://github.com/openjdk/jdk/blob/0f689ea8f29be113cc8a917a86fd9e0f85f921fc/test/jdk/java/lang/annotation/AnnotationType/AnnotationTypeRuntimeAssumptionTest.java) with adjustments to cover the `CLASS`->`RUNTIME` migration. > What I would do is to add a patch for the parser bug That's an interesting discovery! I see a patch and I can include it as your commit in this PR, if there is a test... > and then extend the test in 3 dimensions: ...or just take my PR and expand it. Also the checkbox _"Allow edits and access to secrets by maintainers"_ is on - e.g. this PR is open for modifications. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: 8265518: C1: Intrinsic support for Preconditions.checkIndex [v6]
On Tue, 1 Jun 2021 17:43:45 GMT, Igor Veresov wrote: >> Thank you @veresov! >> >> I'm glad to have more comments from hotspot-compiler group. >> >> Later, I'd like to integrate it if there are no more comments/objections. >> >> Thanks! >> Yang > > @kelthuzadx Sorry about the delay. Could you please rebase this to the > current master and I'll push it. Thanks! @veresov I've rebased to the latest commit and resolved all conflicts. Please take a look at this. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3615
Re: RFR: 8268151: Vector API toShuffle optimization
On Thu, 3 Jun 2021 00:29:00 GMT, Sandhya Viswanathan
wrote:
> The Vector API toShuffle method can be optimized using existing vector
> conversion intrinsic.
>
> The following changes are made:
> 1) vector.toShuffle java implementation is changed to call
> VectorSupport.convert.
> 2) The conversion intrinsic (inline_vector_convert()) in vectorIntrinsics.cpp
> is changed to allow shuffle as a destination type.
> 3) The shuffle.toVector intrinsic (inline_vector_shuffle_to_vector()) in
> vectorIntrinsics.cpp now explicitly generates conversion node instead of
> performing conversion during unbox. This is to remove unnecessary boxing
> during back to back vector.toShuffle and shuffle.toVector calls.
>
> Best Regards,
> Sandhya
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Byte128Vector.java
line 335:
> 333: @ForceInline
> 334: private final
> 335: VectorShuffle toShuffleTemplate(AbstractSpecies dsp) {
Is it better to move this template method to the super class like other APIs?
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Byte128Vector.java
line 350:
> 348: Byte128Shuffle.class, byte.class,
> VLENGTH,
> 349: this, VSPECIES,
> 350: Byte128Vector::toShuffleTemplate);
ditto
-
PR: https://git.openjdk.java.net/jdk/pull/4326
Re: RFR: 8268151: Vector API toShuffle optimization
On Thu, 3 Jun 2021 00:29:00 GMT, Sandhya Viswanathan
wrote:
> The Vector API toShuffle method can be optimized using existing vector
> conversion intrinsic.
>
> The following changes are made:
> 1) vector.toShuffle java implementation is changed to call
> VectorSupport.convert.
> 2) The conversion intrinsic (inline_vector_convert()) in vectorIntrinsics.cpp
> is changed to allow shuffle as a destination type.
> 3) The shuffle.toVector intrinsic (inline_vector_shuffle_to_vector()) in
> vectorIntrinsics.cpp now explicitly generates conversion node instead of
> performing conversion during unbox. This is to remove unnecessary boxing
> during back to back vector.toShuffle and shuffle.toVector calls.
>
> Best Regards,
> Sandhya
src/hotspot/share/opto/vectornode.cpp line 1246:
> 1244: return new VectorLoadMaskNode(value, out_vt);
> 1245: } else if (is_vector_shuffle) {
> 1246: if (!is_shuffle_to_vector()) {
Hi @sviswa7 , thanks for this change! I'm just curious whether
`is_shuffle_to_vector()` is still needed for `VectorUnboxNode` with this
change? It seems this flag can be removed, doesn't it?
-
PR: https://git.openjdk.java.net/jdk/pull/4326
Re: RFR: 8267969: Add vectorized implementation for VectorMask.eq() [v2]
On Wed, 2 Jun 2021 07:48:47 GMT, Nils Eliasson wrote: > Please wait until you have two reviewers before integrating. Sure! Thanks so much for looking at this PR! - PR: https://git.openjdk.java.net/jdk/pull/4272
RFR: 8268151: Vector API toShuffle optimization
The Vector API toShuffle method can be optimized using existing vector conversion intrinsic. The following changes are made: 1) vector.toShuffle java implementation is changed to call VectorSupport.convert. 2) The conversion intrinsic (inline_vector_convert()) in vectorIntrinsics.cpp is changed to allow shuffle as a destination type. 3) The shuffle.toVector intrinsic (inline_vector_shuffle_to_vector()) in vectorIntrinsics.cpp now explicitly generates conversion node instead of performing conversion during unbox. This is to remove unnecessary boxing during back to back vector.toShuffle and shuffle.toVector calls. Best Regards, Sandhya - Commit messages: - toShuffle optimization Changes: https://git.openjdk.java.net/jdk/pull/4326/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4326&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268151 Stats: 393 lines in 34 files changed: 314 ins; 42 del; 37 mod Patch: https://git.openjdk.java.net/jdk/pull/4326.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4326/head:pull/4326 PR: https://git.openjdk.java.net/jdk/pull/4326
Integrated: JDK-8268150: tier2: test/jdk/tools/jpackage/junit/junit.java needs updating for jtreg 6
On Thu, 3 Jun 2021 00:47:45 GMT, Jonathan Gibbons wrote: > Please review a small test fix, to include hamcrest.jar, as required by the > latest version of JUnit in jtreg 6. This pull request has now been integrated. Changeset: ef01e478 Author:Jonathan Gibbons URL: https://git.openjdk.java.net/jdk/commit/ef01e478586c5676747195ea67c1864639305c0f Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8268150: tier2: test/jdk/tools/jpackage/junit/junit.java needs updating for jtreg 6 Reviewed-by: almatvee - PR: https://git.openjdk.java.net/jdk/pull/4328
Re: RFR: JDK-8268150: tier2: test/jdk/tools/jpackage/junit/junit.java needs updating for jtreg 6
On Thu, 3 Jun 2021 00:47:45 GMT, Jonathan Gibbons wrote: > Please review a small test fix, to include hamcrest.jar, as required by the > latest version of JUnit in jtreg 6. Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4328
RFR: JDK-8268150: tier2: test/jdk/tools/jpackage/junit/junit.java needs updating for jtreg 6
Please review a small test fix, to include hamcrest.jar, as required by the latest version of JUnit in jtreg 6. - Commit messages: - JDK-8268150: tier2: test/jdk/tools/jpackage/junit/junit.java needs updating for jtreg 6 Changes: https://git.openjdk.java.net/jdk/pull/4328/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4328&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268150 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4328.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4328/head:pull/4328 PR: https://git.openjdk.java.net/jdk/pull/4328
Re: RFR: 8267939: Clarify the specification of iterator and spliterator forEachRemaining [v2]
On Wed, 2 Jun 2021 22:22:55 GMT, Paul Sandoz wrote: >> The specification of `forEachRemaining`, accepting a primitive functional >> interface, on the primitive iterators is updated to be the same as for >> `Iterator.forEachRemaining`, specifically the following is added: >> >> >> * >> * Subsequent behavior of an iterator is unspecified if the action >> throws an >> * exception. >> >> >> In addition the specification of `tryAdvance` and `forEachRemaining` on >> `Spliterator` and the primitive specializations are also updated to include >> a similar statement: >> >> >> * Subsequent behavior of a spliterator is unspecified if the action >> throws >> * an exception. > > Paul Sandoz has updated the pull request incrementally with one additional > commit since the last revision: > > Use source of elements. Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4290
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
On Wed, 2 Jun 2021 18:07:55 GMT, Daniel Fuchs wrote:
>> Stuart Marks has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Tiny editorial tweaks.
>
> src/java.base/share/classes/java/util/Map.java line 396:
>
>> 394:
>> 395: /**
>> 396: * A map entry (key-value pair). The Entry may be unmodifiable, or
>> the
>
> In that case then maybe it should be `{@code Entry}` in both places where
> `entry` was replaced with `Entry` ?
Maybe. We're not terribly consistent about this. A fair amount of the docs just
uses a plain-text, capitalized form of a class name, as opposed to the code
font. Using the code font everywhere adds clutter to both the markup and to the
rendered output. In this case I wanted to distinguish the generic mention of an
entry in a map from an instance of an Entry object. In cases where it needs to
be absolutely clear, I used `Map.Entry` (the qualified name, in code font).
Doing that here seems like it would add too much clutter though.
-
PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 3/06/2021 2:54 am, Joe Darcy wrote: If the reflection runtime doesn't implement the semantics of -XX+PreserveAllAnnotations, I suggest deprecating/obsoleting/etc. that option now. I have to agree with Joe now. I mistakenly thought the raw annotation stream was exposed to some parts of reflection, but now I see that it all goes through AnnotationParser, which strips out all non-Runtime retention annotations. So the flag is useless as the data it causes to be passed to the JDK is thrown away anyway. Cheers, David -Joe On 6/2/2021 7:48 AM, Peter Levart wrote: On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. What I would do is to add a patch for the parser bug and then extend the test in 3 dimensions: - run it with and without the `-XX+PreserveAllAnnotations` option (adapt expected results accordingly) - test annotation use when besides the annotation that changes retention from CLASS -> RUNTIME there is another RUNTIME annotation present on the annotated element or not (this would fail before the bugfix) - test with different annotated elements (Class, Method, Field, Constructor, Parameter) to exercise different code paths in the VM. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 3/06/2021 1:38 am, Peter Levart wrote: On Wed, 2 Jun 2021 13:24:10 GMT, David Holmes wrote: Sorry now I see what happens. We aren't combining two arrays of annotations we're concatenating two streams of byes, each of which represents a set of annotations, starting with the length. The code that receives this on the JDK side doesn't seem to understand that this is a possibility. Though maybe this isn't a bug, maybe the AnnotationParser is deliberately ignoring the second byte stream. (Though if it were deliberate there should be some commentary to that affect!) David I think this is not deliberate. Since `rawAnnotations` may end up having any of the following: - `RuntimeVisibleAnnotations` (when there were just `RUNTIME` annotation usages compiled into the class or `-XX+PreserveAllAnnotations` was not used at runtime) - `RuntimeInvisibleAnnotations` (when there were just `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) - `RuntimeVisibleAnnotations + RuntimeInvisibleAnnotations` (when there were `RUNTIME` and `CLASS` annotation usages compiled into the class and `-XX+PreserveAllAnnotations` was used at runtime) So why would `RuntimeInvisibleAnnotations` be skipped in 3rd case but not in 2nd case? Well in the second case there is no way to know it is looking only at invisible annotations, so it has to read them and then discard them as they were in fact CLASS retention. The skipping could be seen as an optimization. The code is confusing because it gives no indication that it is aware that runtime invisible annotations could be present: /** * Parses the annotations described by the specified byte array. * resolving constant references in the specified constant pool. * The array must contain an array of annotations as described * in the RuntimeVisibleAnnotations_attribute: but at the same time it checks for the RUNTIME retention, which means it must have recognised the possibility there could be something else there. That check is day 2 code though, on day 1 there was this comment: /* XXX Uncomment when this meta-attribute on meta-attributes (Neal's putback) if (type.retention() == VisibilityLevel.RUNTIME) XXX */ But the end result is that the code in AnnotationParser correctly filters out all non-RUNTIME annotations, either directly, or by skipping them in the stream in the first place. So in that sense there is no bug, but the code could do with some additional comments. Cheers, David - - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8268147: need to update reference to testng module for jtreg6
On Wed, 2 Jun 2021 22:22:22 GMT, Jonathan Gibbons wrote: > Please review a test fix to refer to the new name of the TestNG module. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4325
Integrated: JDK-8268147: need to update reference to testng module for jtreg6
On Wed, 2 Jun 2021 22:22:22 GMT, Jonathan Gibbons wrote: > Please review a test fix to refer to the new name of the TestNG module. This pull request has now been integrated. Changeset: d46a2c8e Author:Jonathan Gibbons URL: https://git.openjdk.java.net/jdk/commit/d46a2c8ecfac785ae2c935a507c3bcae2e76aba9 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8268147: need to update reference to testng module for jtreg6 Reviewed-by: dholmes, psandoz, naoto - PR: https://git.openjdk.java.net/jdk/pull/4325
Re: RFR: JDK-8268147: need to update reference to testng module for jtreg6
On Wed, 2 Jun 2021 22:22:22 GMT, Jonathan Gibbons wrote: > Please review a test fix to refer to the new name of the TestNG module. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4325
Re: RFR: JDK-8268147: need to update reference to testng module for jtreg6
On Wed, 2 Jun 2021 22:22:22 GMT, Jonathan Gibbons wrote: > Please review a test fix to refer to the new name of the TestNG module. Looks good and trivial. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4325
RFR: JDK-8268147: need to update reference to testng module for jtreg6
Please review a test fix to refer to the new name of the TestNG module. - Commit messages: - JDK-8268147: need to update reference to testng module for jtreg6 Changes: https://git.openjdk.java.net/jdk/pull/4325/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4325&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268147 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4325.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4325/head:pull/4325 PR: https://git.openjdk.java.net/jdk/pull/4325
Re: RFR: 8267939: Clarify the specification of iterator and spliterator forEachRemaining [v2]
> The specification of `forEachRemaining`, accepting a primitive functional > interface, on the primitive iterators is updated to be the same as for > `Iterator.forEachRemaining`, specifically the following is added: > > > * > * Subsequent behavior of an iterator is unspecified if the action throws > an > * exception. > > > In addition the specification of `tryAdvance` and `forEachRemaining` on > `Spliterator` and the primitive specializations are also updated to include a > similar statement: > > > * Subsequent behavior of a spliterator is unspecified if the action > throws > * an exception. Paul Sandoz has updated the pull request incrementally with one additional commit since the last revision: Use source of elements. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4290/files - new: https://git.openjdk.java.net/jdk/pull/4290/files/fa786a40..97d2e741 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4290&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4290&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4290/head:pull/4290 PR: https://git.openjdk.java.net/jdk/pull/4290
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On 3/06/2021 12:39 am, Peter Levart wrote: On Wed, 2 Jun 2021 05:59:05 GMT, David Holmes wrote: Sorry Jaroslav but I don't really see this test as a basic functional test of the PreserveAllAnnotations flag. There is no need for any dynamic retention mode switch. All you need as I've said previously is a class with all the CLASS retention annotations of interest (8 IIRC) applied and a programs that reads them, and either expects to find them or not, based on the PreserveAllAnnotations flag. `CLASS` retention annotations are never returned by reflection regardless of whether the `PreserveAllAnnotations` option was used Sorry yes - my bad. I missed that getRawAnnotations() output was then fed through the AnnotationsParser. Thanks, David - or not. The only way (apart from hacking into encapsulated state and doing your own parsing) is to compile a class that uses an annotation with `CLASS` retention and then run a test that looks up the annotation on this class after the annotation has been changed to have `RUNTIME` retention, but the class that uses it has not been recompiled. `AltClassLoader` does the trick which replaces the class file of _v1 class with the class file of _v2 class when loading the class which simulates this change and recompilation of the annotation without changing anything else. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
On Wed, 2 Jun 2021 17:54:06 GMT, Stuart Marks wrote: >> I'm fixing this along with a couple intertwined issues. >> >> 1. Add Map.Entry::copyOf as an idempotent copy operation. >> >> 2. Clarify that AbstractMap.SimpleImmutableEntry is itself unmodifiable (not >> really immutable) but that subclasses can be modifiable. >> >> 3. Clarify some confusing, historical wording about Map.Entry instances >> being obtainable only via iteration of a Map's entry-set view. This was >> never really true, since anyone could implement the Map.Entry interface, and >> it certainly was not true since JDK 1.6 when SimpleEntry and >> SimpleImmutableEntry were added. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Tiny editorial tweaks. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: JDK-8267764: jpackage cannot handle window screensaver files when EXE renamed as SCR
On Wed, 2 Jun 2021 13:48:47 GMT, Andy Herrick wrote: > JDK-8267764: jpackage cannot handle window screensaver files when EXE renamed > as SCR Marked as reviewed by almatvee (Reviewer). Can we add a simple test for .scr executables? - PR: https://git.openjdk.java.net/jdk/pull/4306
Re: RFR: 8267939: Clarify the specification of iterator and spliterator forEachRemaining
On Tue, 1 Jun 2021 23:37:10 GMT, Stuart Marks wrote:
>> Yeah, well spotted. I agree it's awkward. How about we lean on the behavior
>> of the boxed counterpart:
>>
>> /**
>> * Performs the given action for each remaining element until all
>> elements
>> * have been processed or the action throws an exception. Actions are
>> * performed in the order of iteration, if that order is specified.
>> *
>> * This primitive-based method conforms to the same behavior as its
>> * boxed counterpart with regards to the action's behavior.
>> ?
>
> Referring to the "boxed counterpart" is a bit too oblique, I think. The
> specification of Iterator itself isn't all that good to begin with. It's
> written as if the only possible source of an Iterator is a collection (which
> might have been true at first, but it isn't true any longer). But the rest of
> the Iterator spec refers to "the collection" so it kind of makes sense in
> that context.
>
> Maybe just making a minimal change from "collection" to "source of elements"
> would make the most sense.
>
> * The behavior of an iterator is unspecified if the action modifies the
> source of
> * elements in any way (even by calling the {@link #remove remove} method
Ok, that works for me.
-
PR: https://git.openjdk.java.net/jdk/pull/4290
RFR: 8267939: Clarify the specification of iterator and spliterator forEachRemaining
The specification of `forEachRemaining`, accepting a primitive functional interface, on the primitive iterators is updated to be the same as for `Iterator.forEachRemaining`, specifically the following is added: * * Subsequent behavior of an iterator is unspecified if the action throws an * exception. In addition the specification of `tryAdvance` and `forEachRemaining` on `Spliterator` and the primitive specializations are also updated to include a similar statement: * Subsequent behavior of a spliterator is unspecified if the action throws * an exception. - Commit messages: - 8267939: Clarifiy the specification of iterator and spliterator forEachRemaining Changes: https://git.openjdk.java.net/jdk/pull/4290/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4290&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267939 Stats: 48 lines in 2 files changed: 20 ins; 21 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/4290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4290/head:pull/4290 PR: https://git.openjdk.java.net/jdk/pull/4290
Re: RFR: 8267939: Clarify the specification of iterator and spliterator forEachRemaining
On Tue, 1 Jun 2021 21:34:10 GMT, Stuart Marks wrote:
>> The specification of `forEachRemaining`, accepting a primitive functional
>> interface, on the primitive iterators is updated to be the same as for
>> `Iterator.forEachRemaining`, specifically the following is added:
>>
>>
>> *
>> * Subsequent behavior of an iterator is unspecified if the action
>> throws an
>> * exception.
>>
>>
>> In addition the specification of `tryAdvance` and `forEachRemaining` on
>> `Spliterator` and the primitive specializations are also updated to include
>> a similar statement:
>>
>>
>> * Subsequent behavior of a spliterator is unspecified if the action
>> throws
>> * an exception.
>
> src/java.base/share/classes/java/util/PrimitiveIterator.java line 77:
>
>> 75: *
>> 76: * The behavior of an iterator is unspecified if the action modifies
>> the
>> 77: * collection in any way (even by calling the {@link #remove remove}
>> method
>
> It's kind of odd to mention "the collection" here. Iterator is defined as
> being over a collection, but strictly speaking this isn't true; it can be an
> iterator over anything. PrimitiveIterator doesn't say anything more specific
> than this, just that it's a base for iterating primitives... and collections
> cannot contain primitives.
>
> I'm not sure what a better term for this is. Something like, "the underlying
> source of the Iterator"?
Yeah, well spotted. I agree it's awkward. How about we lean on the behavior of
the boxed counterpart:
/**
* Performs the given action for each remaining element until all elements
* have been processed or the action throws an exception. Actions are
* performed in the order of iteration, if that order is specified.
*
* This primitive-based method conforms to the same behavior as its
* boxed counterpart with regards to the action's behavior.
?
-
PR: https://git.openjdk.java.net/jdk/pull/4290
Re: RFR: 8267939: Clarify the specification of iterator and spliterator forEachRemaining
On Tue, 1 Jun 2021 22:40:53 GMT, Paul Sandoz wrote:
>> src/java.base/share/classes/java/util/PrimitiveIterator.java line 77:
>>
>>> 75: *
>>> 76: * The behavior of an iterator is unspecified if the action
>>> modifies the
>>> 77: * collection in any way (even by calling the {@link #remove
>>> remove} method
>>
>> It's kind of odd to mention "the collection" here. Iterator is defined as
>> being over a collection, but strictly speaking this isn't true; it can be an
>> iterator over anything. PrimitiveIterator doesn't say anything more specific
>> than this, just that it's a base for iterating primitives... and collections
>> cannot contain primitives.
>>
>> I'm not sure what a better term for this is. Something like, "the underlying
>> source of the Iterator"?
>
> Yeah, well spotted. I agree it's awkward. How about we lean on the behavior
> of the boxed counterpart:
>
> /**
> * Performs the given action for each remaining element until all elements
> * have been processed or the action throws an exception. Actions are
> * performed in the order of iteration, if that order is specified.
> *
> * This primitive-based method conforms to the same behavior as its
> * boxed counterpart with regards to the action's behavior.
> ?
Referring to the "boxed counterpart" is a bit too oblique, I think. The
specification of Iterator itself isn't all that good to begin with. It's
written as if the only possible source of an Iterator is a collection (which
might have been true at first, but it isn't true any longer). But the rest of
the Iterator spec refers to "the collection" so it kind of makes sense in that
context.
Maybe just making a minimal change from "collection" to "source of elements"
would make the most sense.
* The behavior of an iterator is unspecified if the action modifies the
source of
* elements in any way (even by calling the {@link #remove remove} method
-
PR: https://git.openjdk.java.net/jdk/pull/4290
Re: RFR: 8267939: Clarify the specification of iterator and spliterator forEachRemaining
On Tue, 1 Jun 2021 19:54:59 GMT, Paul Sandoz wrote:
> The specification of `forEachRemaining`, accepting a primitive functional
> interface, on the primitive iterators is updated to be the same as for
> `Iterator.forEachRemaining`, specifically the following is added:
>
>
> *
> * Subsequent behavior of an iterator is unspecified if the action throws
> an
> * exception.
>
>
> In addition the specification of `tryAdvance` and `forEachRemaining` on
> `Spliterator` and the primitive specializations are also updated to include a
> similar statement:
>
>
> * Subsequent behavior of a spliterator is unspecified if the action
> throws
> * an exception.
Generally, adding the "unspecified after an exception" clause in those places
seems to be the correct thing to do here.
Marked as reviewed by smarks (Reviewer).
src/java.base/share/classes/java/util/PrimitiveIterator.java line 77:
> 75: *
> 76: * The behavior of an iterator is unspecified if the action modifies
> the
> 77: * collection in any way (even by calling the {@link #remove remove}
> method
It's kind of odd to mention "the collection" here. Iterator is defined as being
over a collection, but strictly speaking this isn't true; it can be an iterator
over anything. PrimitiveIterator doesn't say anything more specific than this,
just that it's a base for iterating primitives... and collections cannot
contain primitives.
I'm not sure what a better term for this is. Something like, "the underlying
source of the Iterator"?
-
Changes requested by smarks (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4290
Re: Integrated: 8268146: fix for JDK-8266254 fails validate-source
On Wed, 2 Jun 2021 21:50:24 GMT, Bradford Wetmore wrote: >> A trivial copyright fix. > > LGTM @bradfordwetmore - Thanks for the review. - PR: https://git.openjdk.java.net/jdk/pull/4323
Integrated: 8268146: fix for JDK-8266254 fails validate-source
On Wed, 2 Jun 2021 21:39:15 GMT, Daniel D. Daugherty wrote: > A trivial copyright fix. This pull request has now been integrated. Changeset: 76fdf2c8 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/76fdf2c89bb7df9140438fcbaf16ea5fda024551 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8268146: fix for JDK-8266254 fails validate-source Reviewed-by: psandoz, wetmore - PR: https://git.openjdk.java.net/jdk/pull/4323
Re: Integrated: 8268146: fix for JDK-8266254 fails validate-source
On Wed, 2 Jun 2021 21:39:15 GMT, Daniel D. Daugherty wrote: > A trivial copyright fix. LGTM - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4323
Re: Integrated: 8268146: fix for JDK-8266254 fails validate-source
On Wed, 2 Jun 2021 21:40:59 GMT, Paul Sandoz wrote: >> A trivial copyright fix. > > Marked as reviewed by psandoz (Reviewer). @PaulSandoz - Thanks for the fast review! - PR: https://git.openjdk.java.net/jdk/pull/4323
Re: Integrated: 8268146: fix for JDK-8266254 fails validate-source
On Wed, 2 Jun 2021 21:39:15 GMT, Daniel D. Daugherty wrote: > A trivial copyright fix. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4323
Integrated: 8268146: fix for JDK-8266254 fails validate-source
A trivial copyright fix. - Commit messages: - 8268146: fix for JDK-8266254 fails validate-source Changes: https://git.openjdk.java.net/jdk/pull/4323/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4323&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268146 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4323.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4323/head:pull/4323 PR: https://git.openjdk.java.net/jdk/pull/4323
Re: RFR: 8268131: 2 java/foreign tests timed out
Can the test cases be broken into pieces or otherwise decomposed into several shorter-running tests? -Joe On 6/2/2021 2:35 PM, Maurizio Cimadamore wrote: This patch increases time out for both TestUpcall and TestDowncall. These tests were already long-running, but with JEP-412, they were beefed up even more, so now they time out on some debug builds. This patch also address a minor issue in TestResourceScope which can sometimes lead to intermittent failure, depending on how threads are scheduled. - Commit messages: - * Increase timeout for TestUpcall/Downcall Changes: https://git.openjdk.java.net/jdk/pull/4321/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4321&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268131 Stats: 10 lines in 3 files changed: 7 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4321.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4321/head:pull/4321 PR: https://git.openjdk.java.net/jdk/pull/4321
RFR: 8268131: 2 java/foreign tests timed out
This patch increases time out for both TestUpcall and TestDowncall. These tests were already long-running, but with JEP-412, they were beefed up even more, so now they time out on some debug builds. This patch also address a minor issue in TestResourceScope which can sometimes lead to intermittent failure, depending on how threads are scheduled. - Commit messages: - * Increase timeout for TestUpcall/Downcall Changes: https://git.openjdk.java.net/jdk/pull/4321/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4321&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268131 Stats: 10 lines in 3 files changed: 7 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4321.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4321/head:pull/4321 PR: https://git.openjdk.java.net/jdk/pull/4321
Withdrawn: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to an explicit `org.testng`. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
On Wed, 2 Jun 2021 16:56:07 GMT, Joe Darcy wrote: > If the reflection runtime doesn't implement the semantics of > -XX+PreserveAllAnnotations, I suggest deprecating/obsoleting/etc. that > option now. > > -Joe What is the -XX+PreserveAllAnnotations semantics in terms of reflection? The VM spec allows it, but reflection spec doesn't mention it or anything close to it. It doesn't violate the reflection spec. It actually makes reflection behave more correctly in some cases. Without it, reflection sometimes doesn't return annotations with RUNTIME retention. It does implement a useful semantics in terms of late binding. It's just buggy in corner case, which apparently hasn't bothered anyone, since nobody officially complained. That doesn't mean it hasn't been or is not used in the way where it behaves in a useful way and it doesn't mean it won't be needed in the future. Jaroslav gave a perfect example of the case where it is needed now. Removing this option means that migrating an annotation from CLASS retention to RUNTIME becomes almost impossible thing in the real world where no single person controls the whole codebase. Peter - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v4]
> This patch overhauls the library loading mechanism used by the Foreign Linker > API. We realized that, while handy, the *default* lookup abstraction > (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > abstraction, a functional interface. Crucially, `SymbolLookup` does not > concern with library loading, only symbol lookup. For this reason, two > factories are added: > > * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to > lookup symbols in libraries loaded by current loader > * `CLinker::systemLookup` - a more stable replacement for the *default* > lookup, which looks for symbols in libc.so (or its equivalent in other > platforms). The contents of this lookup are unspecified. > > Both factories are *restricted*, so they can only be called when > `--enable-native-access` is set. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4316/files - new: https://git.openjdk.java.net/jdk/pull/4316/files/41ab74b6..7be87eae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=02-03 Stats: 42 lines in 13 files changed: 6 ins; 6 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/4316.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4316/head:pull/4316 PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v3]
> This patch overhauls the library loading mechanism used by the Foreign Linker > API. We realized that, while handy, the *default* lookup abstraction > (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > abstraction, a functional interface. Crucially, `SymbolLookup` does not > concern with library loading, only symbol lookup. For this reason, two > factories are added: > > * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to > lookup symbols in libraries loaded by current loader > * `CLinker::systemLookup` - a more stable replacement for the *default* > lookup, which looks for symbols in libc.so (or its equivalent in other > platforms). The contents of this lookup are unspecified. > > Both factories are *restricted*, so they can only be called when > `--enable-native-access` is set. Maurizio Cimadamore has updated the pull request incrementally with 10 additional commits since the last revision: - Update test/jdk/java/foreign/TestIntrinsics.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/valist/VaListTest.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestVarArgs.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestUpcall.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestIllegalLink.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestSymbolLookup.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestDowncall.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/StdLibTest.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/SafeFunctionAccessTest.java Co-authored-by: Paul Sandoz - Update src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.java.net/jdk/pull/4316/files - new: https://git.openjdk.java.net/jdk/pull/4316/files/2b668f7c..41ab74b6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=01-02 Stats: 10 lines in 10 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/4316.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4316/head:pull/4316 PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v3]
On Wed, 2 Jun 2021 18:40:48 GMT, Paul Sandoz wrote: >> Maurizio Cimadamore has updated the pull request incrementally with 10 >> additional commits since the last revision: >> >> - Update test/jdk/java/foreign/TestIntrinsics.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/valist/VaListTest.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/TestVarArgs.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/TestUpcall.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/TestIllegalLink.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/TestSymbolLookup.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/TestDowncall.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/StdLibTest.java >> >>Co-authored-by: Paul Sandoz >> - Update test/jdk/java/foreign/SafeFunctionAccessTest.java >> >>Co-authored-by: Paul Sandoz >> - Update >> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java >> >>Co-authored-by: Paul Sandoz > > src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java > line 138: > >> 136: * >> 137: * This method is > href="package-summary.html#restricted">restricted. >> 138: * Restricted method are unsafe, and, if used incorrectly, their >> use might crash > > Suggestion: > > * Restricted methods are unsafe, and, if used incorrectly, their use > might crash Ouch - this seems like an issue with _all_ restricted methods. > src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java > line 160: > >> 158: * to allocate structs returned by-value. >> 159: * >> 160: * @see SymbolLookup > > For JDK code it is more common for `@See` to occur after the > parameters/return/throws, and before any `@Since`. I'll fix - PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
On Wed, 2 Jun 2021 18:17:43 GMT, Joe Wang wrote: > Special characters are different in File and URI. Treat File input as File > using FileInputStream instead of converting to an URI, but fall back to URI > in case of error for compatibility (in error handling). Thanks Daniel! For the question, it will not be dead code as it accepts systemId in String form as well. But as least when File is the input, we don't end up going through a convoluted (if not unnecessary) File -URI -URL - File conversion. I think the later has sth. to do with the fact that Apache still supports older JDKs. While we agree not to touch that code for this fix (and also for not changing the mechanisms, e.g. how errors are handled), we'll keep an eye out on it. - PR: https://git.openjdk.java.net/jdk/pull/4318
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v2]
On Wed, 2 Jun 2021 20:17:20 GMT, Maurizio Cimadamore
wrote:
>> This patch overhauls the library loading mechanism used by the Foreign
>> Linker API. We realized that, while handy, the *default* lookup abstraction
>> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
>>
>> This patch replaces `LibraryLookup` with a simpler `SymbolLookup`
>> abstraction, a functional interface. Crucially, `SymbolLookup` does not
>> concern with library loading, only symbol lookup. For this reason, two
>> factories are added:
>>
>> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to
>> lookup symbols in libraries loaded by current loader
>> * `CLinker::systemLookup` - a more stable replacement for the *default*
>> lookup, which looks for symbols in libc.so (or its equivalent in other
>> platforms). The contents of this lookup are unspecified.
>>
>> Both factories are *restricted*, so they can only be called when
>> `--enable-native-access` is set.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update
> test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
>
> Co-authored-by: Jorn Vernee
Looks good. Just minor comments to accept/reject.
The shim library is an interesting approach. I did wonder if the libvm library
could be used, but it might have some weird side-effects or bring in more
symbols than necessary.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
138:
> 136: *
> 137: * This method is href="package-summary.html#restricted">restricted.
> 138: * Restricted method are unsafe, and, if used incorrectly, their use
> might crash
Suggestion:
* Restricted methods are unsafe, and, if used incorrectly, their use might
crash
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
160:
> 158: * to allocate structs returned by-value.
> 159: *
> 160: * @see SymbolLookup
For JDK code it is more common for `@See` to occur after the
parameters/return/throws, and before any `@Since`.
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java
line 418:
> 416: private static class AllocHolder {
> 417:
> 418: private static final CLinker linker = getSystemLinker();
Suggestion:
private static final CLinker SYS_LINKER = getSystemLinker();
test/jdk/java/foreign/SafeFunctionAccessTest.java line 53:
> 51: );
> 52:
> 53: static SymbolLookup lookup = SymbolLookup.loaderLookup();
Suggestion:
static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();
test/jdk/java/foreign/StdLibTest.java line 158:
> 156: static class StdLibHelper {
> 157:
> 158: final static SymbolLookup LOOKUP;
Suggestion:
static final SymbolLookup LOOKUP;
test/jdk/java/foreign/TestDowncall.java line 60:
> 58: }
> 59:
> 60: static SymbolLookup lookup = SymbolLookup.loaderLookup();
Suggestion:
static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();
test/jdk/java/foreign/TestIllegalLink.java line 48:
> 46: public class TestIllegalLink {
> 47:
> 48: private static final MemoryAddress dummyTarget =
> MemoryAddress.ofLong(1);
Suggestion:
private static final MemoryAddress DUMMY_TARGET = MemoryAddress.ofLong(1);
test/jdk/java/foreign/TestIntrinsics.java line 60:
> 58: }
> 59:
> 60: static SymbolLookup lookup = SymbolLookup.loaderLookup();
Suggestion:
static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();
test/jdk/java/foreign/TestSymbolLookup.java line 50:
> 48: }
> 49:
> 50: static SymbolLookup lookup = SymbolLookup.loaderLookup();
Suggestion:
static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();
test/jdk/java/foreign/TestUpcall.java line 69:
> 67: static CLinker abi = CLinker.getInstance();
> 68:
> 69: static SymbolLookup lookup = SymbolLookup.loaderLookup();
Suggestion:
static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();
test/jdk/java/foreign/TestVarArgs.java line 68:
> 66: }
> 67:
> 68: static final MemoryAddress varargsAddr =
Suggestion:
static final MemoryAddress VARARGS_ADDR =
test/jdk/java/foreign/valist/VaListTest.java line 74:
> 72: }
> 73:
> 74: static SymbolLookup lookup = SymbolLookup.loaderLookup();
Suggestion:
static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
On Wed, 2 Jun 2021 18:17:43 GMT, Joe Wang wrote: > Special characters are different in File and URI. Treat File input as File > using FileInputStream instead of converting to an URI, but fall back to URI > in case of error for compatibility (in error handling). I now agree that what you propose is the safer route to fix this particular issue. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4318
Re: RFR: JDK-8266254: Update to use jtreg 6 [v2]
On Wed, 2 Jun 2021 20:15:55 GMT, Jonathan Gibbons wrote: >> Please review the change to update to using jtreg 6. >> >> The primary change is to the jib-profiles.js file, which specifies the >> version of jtreg to use, for those systems that rely on this file. In >> addition, the `requiredVersion` has been updated in the various `TEST.ROOT` >> files. >> >> All the tests that could be updated ahead of time have been updated. There >> are a few tests remaining that need to be done at this time, because of the >> change in the module name for TestNG 7.3. It changed from a default of >> `testng` to and explicit `org.testng`. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright years Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
On Wed, 2 Jun 2021 18:17:43 GMT, Joe Wang wrote: > Special characters are different in File and URI. Treat File input as File > using FileInputStream instead of converting to an URI, but fall back to URI > in case of error for compatibility (in error handling). Argh! You're right. It's this line which is horribly wrong: _ostream = new FileOutputStream(url.getFile()); OK - hopefully with your fix this becomes dead code? - PR: https://git.openjdk.java.net/jdk/pull/4318
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore wrote: > This patch overhauls the library loading mechanism used by the Foreign Linker > API. We realized that, while handy, the *default* lookup abstraction > (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > abstraction, a functional interface. Crucially, `SymbolLookup` does not > concern with library loading, only symbol lookup. For this reason, two > factories are added: > > * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to > lookup symbols in libraries loaded by current loader > * `CLinker::systemLookup` - a more stable replacement for the *default* > lookup, which looks for symbols in libc.so (or its equivalent in other > platforms). The contents of this lookup are unspecified. > > Both factories are *restricted*, so they can only be called when > `--enable-native-access` is set. > _Mailing list message from [Chapman Flack](mailto:c...@anastigmatix.net) on > [security-dev](mailto:security-...@mail.openjdk.java.net):_ > > On 06/02/21 13:30, Maurizio Cimadamore wrote: > > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > > abstraction, a functional interface. Crucially, `SymbolLookup` does not > > concern with library loading, only symbol lookup. For this reason, two > > factories are added: > > Hi, > > While I am thinking about this, what will be the behavior when the JVM > itself has been dynamically loaded into an embedding application, and > launched with the JNI invocation API? > > Will there be a *Lookup flavor that is able to find any exported symbol > known in the embedding environment the JVM was loaded into? (This is the > sort of condition the Mac OS linker checks when given the -bundle_loader > option.) > > Regards, > Chapman Flack (maintainer of a project that happens to work that way) Hi, at the moment we don't have plans to add such a lookup, but I believe it should be possible to build such a lookup using `dlopen` and the linker API. I have provided an example elsewhere of how easy it easy to build a wrapper lookup around dlopen/dlsym: https://gist.github.com/mcimadamore/0883ea6f4836ae0c1d2a31c48197da1a Perhaps something like that could be useful in the use case you mention? - PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v2]
> This patch overhauls the library loading mechanism used by the Foreign Linker > API. We realized that, while handy, the *default* lookup abstraction > (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > abstraction, a functional interface. Crucially, `SymbolLookup` does not > concern with library loading, only symbol lookup. For this reason, two > factories are added: > > * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to > lookup symbols in libraries loaded by current loader > * `CLinker::systemLookup` - a more stable replacement for the *default* > lookup, which looks for symbols in libc.so (or its equivalent in other > platforms). The contents of this lookup are unspecified. > > Both factories are *restricted*, so they can only be called when > `--enable-native-access` is set. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/4316/files - new: https://git.openjdk.java.net/jdk/pull/4316/files/01d9c198..2b668f7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4316.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4316/head:pull/4316 PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: JDK-8266254: Update to use jtreg 6 [v2]
> Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: Update copyright years - Changes: - all: https://git.openjdk.java.net/jdk/pull/4315/files - new: https://git.openjdk.java.net/jdk/pull/4315/files/0d1e554a..4ef5614f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4315&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4315&range=00-01 Stats: 6 lines in 6 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/4315.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4315/head:pull/4315 PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
On Wed, 2 Jun 2021 18:17:43 GMT, Joe Wang wrote: > Special characters are different in File and URI. Treat File input as File > using FileInputStream instead of converting to an URI, but fall back to URI > in case of error for compatibility (in error handling). I tried, but getRawPath returned output/%23/dom.xml, that in turn resulted in java.io.FileNotFoundException later. For StreamResult, when a String-form systemId is in File protocol, a FileOutputStream will eventually get created. Since StreamResult accepts OutputStream, there's no need for the File -> URI -> systemId -> URL -> File conversion (as did in TransformerImpl). I think it's better to create a FileOutputStream right from the File input. - PR: https://git.openjdk.java.net/jdk/pull/4318
Re: RFR: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
On Wed, 2 Jun 2021 18:17:43 GMT, Joe Wang wrote: > Special characters are different in File and URI. Treat File input as File > using FileInputStream instead of converting to an URI, but fall back to URI > in case of error for compatibility (in error handling). I believe this is the wrong fix - or at least an incomplete fix that will hide the bug under the carpet. Looking at TransformImpl I would change line TransformImpl.java:521 to: 521: - String path = uri.getPath(); //decoded String 521: + String path = uri.getRawPath(); //raw String - PR: https://git.openjdk.java.net/jdk/pull/4318
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v3]
On Wed, 2 Jun 2021 17:58:29 GMT, Vyom Tewari wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8268113: Inline local vars where reasonable > > src/java.base/share/classes/java/util/BitSet.java line 105: > >> 103: * the user knows what he's doing and try harder to preserve it. >> 104: */ >> 105: private transient boolean sizeIsSticky = false; > > This change is OK, but it is not related to "8268113", do you really wants to > do these changes as part of "8268113" ? This is just a tiny code clean-up, so I think it's ok to go - PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore
wrote:
> This patch overhauls the library loading mechanism used by the Foreign Linker
> API. We realized that, while handy, the *default* lookup abstraction
> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
>
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup`
> abstraction, a functional interface. Crucially, `SymbolLookup` does not
> concern with library loading, only symbol lookup. For this reason, two
> factories are added:
>
> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to
> lookup symbols in libraries loaded by current loader
> * `CLinker::systemLookup` - a more stable replacement for the *default*
> lookup, which looks for symbols in libc.so (or its equivalent in other
> platforms). The contents of this lookup are unspecified.
>
> Both factories are *restricted*, so they can only be called when
> `--enable-native-access` is set.
LGTM, minor nit inline
test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
line 62:
> 60: );
> 61: MH_distance_ptrs = abi.downcallHandle(
> 62: lookup.lookup("distance_ptrs").get(),
Spurious white space
Suggestion:
lookup.lookup("distance_ptrs").get(),
-
Marked as reviewed by jvernee (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4316
RFR: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
Special characters are different in File and URI. Treat File input as File using FileInputStream instead of converting to an URI, but fall back to URI in case of error for compatibility (in error handling). - Commit messages: - 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path Changes: https://git.openjdk.java.net/jdk/pull/4318/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4318&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266019 Stats: 39 lines in 2 files changed: 31 ins; 1 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/4318.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4318/head:pull/4318 PR: https://git.openjdk.java.net/jdk/pull/4318
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
On Wed, 2 Jun 2021 17:54:06 GMT, Stuart Marks wrote:
>> I'm fixing this along with a couple intertwined issues.
>>
>> 1. Add Map.Entry::copyOf as an idempotent copy operation.
>>
>> 2. Clarify that AbstractMap.SimpleImmutableEntry is itself unmodifiable (not
>> really immutable) but that subclasses can be modifiable.
>>
>> 3. Clarify some confusing, historical wording about Map.Entry instances
>> being obtainable only via iteration of a Map's entry-set view. This was
>> never really true, since anyone could implement the Map.Entry interface, and
>> it certainly was not true since JDK 1.6 when SimpleEntry and
>> SimpleImmutableEntry were added.
>
> Stuart Marks has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Tiny editorial tweaks.
src/java.base/share/classes/java/util/Map.java line 396:
> 394:
> 395: /**
> 396: * A map entry (key-value pair). The Entry may be unmodifiable, or
> the
In that case then maybe it should be `{@code Entry}` in both places where
`entry` was replaced with `Entry` ?
-
PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v3]
On Wed, 2 Jun 2021 16:37:49 GMT, Сергей Цыпанов wrote: >> There is a few JDK classes duplicating the contents of Long.hashCode() for >> hash code calculation. They should explicitly delegate to Long.hashCode(). > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8268113: Inline local vars where reasonable src/java.base/share/classes/java/util/BitSet.java line 105: > 103: * the user knows what he's doing and try harder to preserve it. > 104: */ > 105: private transient boolean sizeIsSticky = false; This change is OK, but it is not related to "8268113", do you really wants to do these changes as part of "8268113" ? - PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8266317: Vector API enhancements [v5]
> This PR contains API and implementation changes for [JEP-414 Vector API > (Second Incubator)](https://openjdk.java.net/jeps/414), in preparation for > when targeted. > > Enhancements are made to the API for the support of operations on characters, > such as for UTF-8 character decoding. Specifically, methods for > loading/storing a `short` vector from/to a `char[]` array, and new vector > comparison operators for unsigned comparisons with integral vectors. The x64 > implementation is enhanced to supported unsigned comparisons. > > Enhancements are made to the API for loading/storing a `byte` vector from/to > a `boolean[]` array. > > The testing of loads/stores can be expanded for scatter/gather, but before > doing that i think some refactoring of the tests is required to reposition > tests in the right classes. I would like to do that work after integration of > the PR. Paul Sandoz has updated the pull request incrementally with one additional commit since the last revision: Check vlen in bytes for unsigned support - Changes: - all: https://git.openjdk.java.net/jdk/pull/3803/files - new: https://git.openjdk.java.net/jdk/pull/3803/files/12b23f62..20a9c9ce Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3803&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3803&range=03-04 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3803.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3803/head:pull/3803 PR: https://git.openjdk.java.net/jdk/pull/3803
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
> I'm fixing this along with a couple intertwined issues. > > 1. Add Map.Entry::copyOf as an idempotent copy operation. > > 2. Clarify that AbstractMap.SimpleImmutableEntry is itself unmodifiable (not > really immutable) but that subclasses can be modifiable. > > 3. Clarify some confusing, historical wording about Map.Entry instances being > obtainable only via iteration of a Map's entry-set view. This was never > really true, since anyone could implement the Map.Entry interface, and it > certainly was not true since JDK 1.6 when SimpleEntry and > SimpleImmutableEntry were added. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Tiny editorial tweaks. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4295/files - new: https://git.openjdk.java.net/jdk/pull/4295/files/841a154c..c67b6445 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4295&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4295&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4295.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4295/head:pull/4295 PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov wrote: >> 8267706: bin/idea.sh tries to use cygpath on WSL > > Nikita Gubarkov has updated the pull request incrementally with one > additional commit since the last revision: > > 8267706: Break long line in make/ide/idea/jdk/idea.gmk I filed a new issue yesterday, linked to the old one. There should be no need to create another. https://bugs.openjdk.java.net/browse/JDK-8268083 - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore wrote: > This patch overhauls the library loading mechanism used by the Foreign Linker > API. We realized that, while handy, the *default* lookup abstraction > (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > abstraction, a functional interface. Crucially, `SymbolLookup` does not > concern with library loading, only symbol lookup. For this reason, two > factories are added: > > * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to > lookup symbols in libraries loaded by current loader > * `CLinker::systemLookup` - a more stable replacement for the *default* > lookup, which looks for symbols in libc.so (or its equivalent in other > platforms). The contents of this lookup are unspecified. > > Both factories are *restricted*, so they can only be called when > `--enable-native-access` is set. Some notes on how the *system* lookup is implemented (see class SystemLookup): * On Linux and MacOS, we generate, at build time, an empty shim library. Since this library depends on the C standard libraries, we can, at runtime, load this shim library, and use `dlsym` to lookup symbols in it (since `dlsym` will also look at the dependencies). * Since Windows does not allow for library symbols in dependent libraries to be re-exported, Windows uses a different approach - which loads either `ucrtbase.dll` or `msvcrt.dll` (the former is preferred, if available), and returns a lookup object based on that. In both cases, the required libraries are loaded in a classloader-independent fashion, by taking advantage of the support available in NativeLibraries. Class loader lookups are built on top of ClassLoader::findNative (which is also the method used by JNI to find JNI methods). This patch removes all the native code which was required to support the default lookup abstraction. The implementation changes are relatively straightforward - but several test and microbenchmark updates were required. - PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8267893: Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes [v5]
On Fri, 28 May 2021 02:14:45 GMT, Igor Ignatyev wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spaces updated.
>
> test/failure_handler/src/share/classes/jdk/test/failurehandler/action/PatternAction.java
> line 63:
>
>> 61: }
>> 62: for (int i = 0, n = args.length; i < n; ++i) {
>> 63: args[i] = args[i].replace("%java",
>> helper.findApp("java").getAbsolutePath());
>
> are we sure that `java` from `PATH` (which is what `findApp` returns) is the
> right `java`?
The paths contain testJdk and compileJdk before PATH. We use it to find any of
our tools.
-
PR: https://git.openjdk.java.net/jdk/pull/4234
RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
This patch overhauls the library loading mechanism used by the Foreign Linker API. We realized that, while handy, the *default* lookup abstraction (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. This patch replaces `LibraryLookup` with a simpler `SymbolLookup` abstraction, a functional interface. Crucially, `SymbolLookup` does not concern with library loading, only symbol lookup. For this reason, two factories are added: * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to lookup symbols in libraries loaded by current loader * `CLinker::systemLookup` - a more stable replacement for the *default* lookup, which looks for symbols in libc.so (or its equivalent in other platforms). The contents of this lookup are unspecified. Both factories are *restricted*, so they can only be called when `--enable-native-access` is set. - Commit messages: - Improve javadoc - Initial push Changes: https://git.openjdk.java.net/jdk/pull/4316/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268129 Stats: 1278 lines in 44 files changed: 569 ins; 617 del; 92 mod Patch: https://git.openjdk.java.net/jdk/pull/4316.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4316/head:pull/4316 PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 17:12:21 GMT, Stuart Marks wrote: > A quick search reveals that Guava has a public subclass of > SimpleImmutableEntry: > https://guava.dev/releases/30.1.1-jre/api/docs/com/google/common/cache/RemovalNotification.html >There are possibly others. It doesn't seem worth the incompatibility to me, as >it would break stuff in order to have only a "cleaner" meaning for >"immutable." Also, there are other classes in the JDK that claim they are >immutable but which can be subclassed, e.g., BigInteger. I don't see a good >way of fixing this. Thanks, i've done some digging too, I foolishly hoped that nobody would subclass a class with `Immutable` in its name, oh boy i was wrong :) So documenting the existing behavior seems the only possible way. - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8265783: Create a separate library for x86 Intel SVML assembly intrinsics [v16]
> This PR contains Short Vector Math Library support related changes for > [JEP-414 Vector API (Second Incubator)](https://openjdk.java.net/jeps/414), > in preparation for when targeted. > > Intel Short Vector Math Library (SVML) based intrinsics in native x86 > assembly provide optimized implementation for Vector API transcendental and > trigonometric methods. > These methods are built into a separate library instead of being part of > libjvm.so or jvm.dll. > > The following changes are made: >The source for these methods is placed in the jdk.incubator.vector module > under src/jdk.incubator.vector/linux/native/libsvml and > src/jdk.incubator.vector/windows/native/libsvml. >The assembly source files are named as “*.S” and include files are named > as “*.S.inc”. >The corresponding build script is placed at > make/modules/jdk.incubator.vector/Lib.gmk. >Changes are made to build system to support dependency tracking for > assembly files with includes. >The built native libraries (libsvml.so/svml.dll) are placed in bin > directory of JDK on Windows and lib directory of JDK on Linux. >The C2 JIT uses the dll_load and dll_lookup to get the addresses of > optimized methods from this library. > > Build system changes and module library build scripts are contributed by > Magnus (magnus.ihse.bur...@oracle.com). > > Looking forward to your review and feedback. > > Performance: > Micro benchmark Base Optimized Unit Gain(Optimized/Base) > Double128Vector.ACOS 45.91 87.34 ops/ms 1.90 > Double128Vector.ASIN 45.06 92.36 ops/ms 2.05 > Double128Vector.ATAN 19.92 118.36 ops/ms 5.94 > Double128Vector.ATAN2 15.24 88.17 ops/ms 5.79 > Double128Vector.CBRT 45.77 208.36 ops/ms 4.55 > Double128Vector.COS 49.94 245.89 ops/ms 4.92 > Double128Vector.COSH 26.91 126.00 ops/ms 4.68 > Double128Vector.EXP 71.64 379.65 ops/ms 5.30 > Double128Vector.EXPM1 35.95 150.37 ops/ms 4.18 > Double128Vector.HYPOT 50.67 174.10 ops/ms 3.44 > Double128Vector.LOG 61.95 279.84 ops/ms 4.52 > Double128Vector.LOG10 59.34 239.05 ops/ms 4.03 > Double128Vector.LOG1P 18.56 200.32 ops/ms 10.79 > Double128Vector.SIN 49.36 240.79 ops/ms 4.88 > Double128Vector.SINH 26.59 103.75 ops/ms 3.90 > Double128Vector.TAN 41.05 152.39 ops/ms 3.71 > Double128Vector.TANH 45.29 169.53 ops/ms 3.74 > Double256Vector.ACOS 54.21 106.39 ops/ms 1.96 > Double256Vector.ASIN 53.60 107.99 ops/ms 2.01 > Double256Vector.ATAN 21.53 189.11 ops/ms 8.78 > Double256Vector.ATAN2 16.67 140.76 ops/ms 8.44 > Double256Vector.CBRT 56.45 397.13 ops/ms 7.04 > Double256Vector.COS 58.26 389.77 ops/ms 6.69 > Double256Vector.COSH 29.44 151.11 ops/ms 5.13 > Double256Vector.EXP 86.67 564.68 ops/ms 6.52 > Double256Vector.EXPM1 41.96 201.28 ops/ms 4.80 > Double256Vector.HYPOT 66.18 305.74 ops/ms 4.62 > Double256Vector.LOG 71.52 394.90 ops/ms 5.52 > Double256Vector.LOG10 65.43 362.32 ops/ms 5.54 > Double256Vector.LOG1P 19.99 300.88 ops/ms 15.05 > Double256Vector.SIN 57.06 380.98 ops/ms 6.68 > Double256Vector.SINH 29.40 117.37 ops/ms 3.99 > Double256Vector.TAN 44.90 279.90 ops/ms 6.23 > Double256Vector.TANH 54.08 274.71 ops/ms 5.08 > Double512Vector.ACOS 55.65 687.54 ops/ms 12.35 > Double512Vector.ASIN 57.31 777.72 ops/ms 13.57 > Double512Vector.ATAN 21.42 729.21 ops/ms 34.04 > Double512Vector.ATAN2 16.37 414.33 ops/ms 25.32 > Double512Vector.CBRT 56.78 834.38 ops/ms 14.69 > Double512Vector.COS 59.88 837.04 ops/ms 13.98 > Double512Vector.COSH 30.34 172.76 ops/ms 5.70 > Double512Vector.EXP 99.66 1608.12 ops/ms 16.14 > Double512Vector.EXPM1 43.39 318.61 ops/ms 7.34 > Double512Vector.HYPOT 73.87 1502.72 ops/ms 20.34 > Double512Vector.LOG 74.84 996.00 ops/ms 13.31 > Double512Vector.LOG10 71.12 1046.52 ops/ms 14.72 > Double512Vector.LOG1P 19.75 776.87 ops/ms 39.34 > Double512Vector.POW 37.42 384.13 ops/ms 10.26 > Double512Vector.SIN 59.74 728.45 ops/ms 12.19 > Double512Vector.SINH 29.47 143.38 ops/ms 4.87 > Double512Vector.TAN 46.20 587.21 ops/ms 12.71 > Double512Vector.TANH 57.36 495.42 ops/ms 8.64 > Double64Vector.ACOS 24.04 73.67 ops/ms 3.06 > Double64Vector.ASIN 23.78 75.11 ops/ms 3.16 > Double64Vector.ATAN 14.14 62.81 ops/ms 4.44 > Double64Vector.ATAN2 10.38 44.43 ops/ms 4.28 > Double64Vector.CBRT 16.47 107.50 ops/ms 6.53 > Double64Vector.COS 23.42 152.01 ops/ms 6.49 > Double64Vector.COSH 17.34 113.34 ops/ms 6.54 > Double64Vector.EXP 27.08 203.53 ops/ms 7.52 > Double64Vector.EXPM1 18.77 96.73 ops/ms 5.15 > Double64Vector.HYPOT 18.54 103.62 ops/ms 5.59 > Double64Vector.LOG 26.75 142.63 ops/ms 5.33 > Double64Vector.LOG10 25.85 139.71 ops/ms 5.40 > Double64Vector.LOG1P 13.26 97.94 ops/ms 7.38 > Double64Vector.SIN 23.28 146.91 ops/ms 6.31 > Double64Vector.SINH 17.62 88.59 ops/ms 5.03 > Double64Vector.TAN 21.00 86.43 ops/ms 4.12 > Double64Vector.TANH 23.75 111.35 ops/ms 4.69 > Float128Vector.ACOS 57.52 110.65 ops/ms 1.92 > Float128Vector.ASIN 57.15 117.95 ops/ms 2.06 > Float128Vector.ATAN 22.52 318.74 ops/ms 14.15 > Float128Vector.ATAN2 17.06 246.07 ops/ms 14.42 > Float128Vecto
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 07:35:25 GMT, Rémi Forax wrote: > i wonder if we should not declare SimpleImmutableEntry final, while it's not > a backward compatible change, > it's may be better on the long term because SimpleImmutableEntry is already > used as an immutable type, > so instead of documenting the fact that SimpleImmutableEntry is not declared > final thus SimpleImmutableEntry as a type does not guarantte shallow > immutability, it may be better to fix the root cause. A quick search reveals that Guava has a public subclass of SimpleImmutableEntry: https://guava.dev/releases/30.1.1-jre/api/docs/com/google/common/cache/RemovalNotification.html There are possibly others. It doesn't seem worth the incompatibility to me, as it would break stuff in order to have only a "cleaner" meaning for "immutable." Also, there are other classes in the JDK that claim they are immutable but which can be subclassed, e.g., BigInteger. I don't see a good way of fixing this. - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Looks good, I had expected we would have more tests depending on the automatic module. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick Changes to Math and Long look fine. - Marked as reviewed by darcy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Some of the modified files have copyright year left unchanged. `2021` needs to be appended. - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: JDK-8267764: jpackage cannot handle window screensaver files when EXE renamed as SCR
On Wed, 2 Jun 2021 13:48:47 GMT, Andy Herrick wrote: > JDK-8267764: jpackage cannot handle window screensaver files when EXE renamed > as SCR Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4306
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
Hi Patrick, some minor comments.
src/java.base/share/classes/java/lang/CharacterData.java line 80:
> 78: case 2 -> CharacterData02.instance;
> 79: case 3 -> CharacterData03.instance;
> 80: case 14 -> CharacterData0E.instance; // Private Use
Plane 14 is not `private use`
src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 90:
> 88: // findVirtual etc.
> 89: return switch (refKind) {
> 90: case REF_invokeSpecial: {
Is ':' preferred here (and other cases too) instead of "->"?
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: JDK-8267936: PreserveAllAnnotations option isn't tested
If the reflection runtime doesn't implement the semantics of -XX+PreserveAllAnnotations, I suggest deprecating/obsoleting/etc. that option now. -Joe On 6/2/2021 7:48 AM, Peter Levart wrote: On Tue, 1 Jun 2021 09:30:40 GMT, Jaroslav Tulach wrote: There doesn't seem to be much support for the complete changes in #4245. To get at least something useful from that endeavor I have extracted the test for existing behavior of `-XX:+PreserveAllAnnotations` and I am offering it in this pull request without any changes to the JVM behavior. What I would do is to add a patch for the parser bug and then extend the test in 3 dimensions: - run it with and without the `-XX+PreserveAllAnnotations` option (adapt expected results accordingly) - test annotation use when besides the annotation that changes retention from CLASS -> RUNTIME there is another RUNTIME annotation present on the annotated element or not (this would fail before the bugfix) - test with different annotated elements (Class, Method, Field, Constructor, Parameter) to exercise different code paths in the VM. - PR: https://git.openjdk.java.net/jdk/pull/4280
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v7]
On Wed, 2 Jun 2021 15:00:56 GMT, Roger Riggs wrote: >> Methods are added to java.lang.Process to read and write characters and >> lines from and to a spawned Process. >> The Charset used to encode and decode characters to bytes can be specified >> or use the >> operating system native encoding as is available from the "native.encoding" >> system property. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Editorial improvements in outputWriter and inputReader. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v3]
> There is a few JDK classes duplicating the contents of Long.hashCode() for > hash code calculation. They should explicitly delegate to Long.hashCode(). Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8268113: Inline local vars where reasonable - Changes: - all: https://git.openjdk.java.net/jdk/pull/4309/files - new: https://git.openjdk.java.net/jdk/pull/4309/files/3f5b431d..df8be00a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4309&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4309&range=01-02 Stats: 6 lines in 3 files changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4309/head:pull/4309 PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v3]
On Wed, 2 Jun 2021 14:50:23 GMT, liach
wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8268113: Inline local vars where reasonable
>
> src/java.base/share/classes/java/lang/Double.java line 881:
>
>> 879: public static int hashCode(double value) {
>> 880: long bits = doubleToLongBits(value);
>> 881: return Long.hashCode(bits);
>
> Imo these should be squashed to just
>
> return Long.hashCode(doubleToLongBits(value));
>
> since the local variable is no longer meaningful. Previously, they were
> required as they were retrieved twice in hash code calculation.
>
> Same for other usages.
Done!
-
PR: https://git.openjdk.java.net/jdk/pull/4309
Integrated: 8267569: java.io.File.equals contains misleading Javadoc
On Thu, 27 May 2021 20:33:50 GMT, Brian Burkhalter wrote: > Modify the specification of `java.io.File.equals` to clarify that equality is > based only on a comparison of abstract pathnames as opposed to the file > system objects that the `File`s represent. This pull request has now been integrated. Changeset: 56b65e4a Author:Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/56b65e4a8d519801d170e16063ccb7dd3069c4be Stats: 14 lines in 1 file changed: 7 ins; 0 del; 7 mod 8267569: java.io.File.equals contains misleading Javadoc Reviewed-by: alanb, dfuchs, bchristi, naoto - PR: https://git.openjdk.java.net/jdk/pull/4232
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v2]
> There is a few JDK classes duplicating the contents of Long.hashCode() for > hash code calculation. They should explicitly delegate to Long.hashCode(). Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8268113: Delegate to Double.hashCode() - Changes: - all: https://git.openjdk.java.net/jdk/pull/4309/files - new: https://git.openjdk.java.net/jdk/pull/4309/files/9c7ddc0c..3f5b431d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4309&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4309&range=00-01 Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4309/head:pull/4309 PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v2]
On Wed, 2 Jun 2021 14:51:35 GMT, liach
wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8268113: Delegate to Double.hashCode()
>
> src/java.desktop/macosx/classes/apple/laf/JRSUIConstants.java line 115:
>
>> 113: public int hashCode() {
>> 114: final long bits = Double.doubleToLongBits(doubleValue);
>> 115: return Long.hashCode(bits);
>
> This one and the `DoubleDV` one should actually delegate to
> `Double.hashCode`, which delegates to `Long.hashCode` after this change.
Good point, done!
-
PR: https://git.openjdk.java.net/jdk/pull/4309
RFR: JDK-8266254: Update to use jtreg 6
Please review the change to update to using jtreg 6. The primary change is to the jib-profiles.js file, which specifies the version of jtreg to use, for those systems that rely on this file. In addition, the `requiredVersion` has been updated in the various `TEST.ROOT` files. All the tests that could be updated ahead of time have been updated. There are a few tests remaining that need to be done at this time, because of the change in the module name for TestNG 7.3. It changed from a default of `testng` to and explicit `org.testng`. - Commit messages: - JDK-8266254: Update to use jtreg 6 Changes: https://git.openjdk.java.net/jdk/pull/4315/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4315&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266254 Stats: 17 lines in 11 files changed: 0 ins; 1 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/4315.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4315/head:pull/4315 PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 366:
> 364: }
> 365: default -> throw new IllegalArgumentException(methodName);
> 366: };
I thinki it's simpler to have something like that
var handle = switch(methodName) {
...
};
return methodType != null ? new ConstantCallSite(handle) : handle;
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 278:
> 276: private static boolean isObjectMethod(Method m) {
> 277: return switch (m.getName()) {
> 278: case "toString" -> (m.getReturnType() == String.class
the extra parenthesis are not needed
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov wrote: >> 8267706: bin/idea.sh tries to use cygpath on WSL > > Nikita Gubarkov has updated the pull request incrementally with one > additional commit since the last revision: > > 8267706: Break long line in make/ide/idea/jdk/idea.gmk We require a new issue to be filed. Thank you for looking into fixing this! - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick src/java.base/share/classes/java/lang/invoke/MemberName.java line 331: > 329: assert(false) : this+" != > "+MethodHandleNatives.refKindName((byte)originalRefKind); > 330: yield true; > 331: } this code always yield true, better to check if the assert are enabled, do the switch in that case and always return true - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line
1663:
> 1661: case D_TYPE -> mv.visitInsn(Opcodes.DCONST_0);
> 1662: case L_TYPE -> mv.visitInsn(Opcodes.ACONST_NULL);
> 1663: default -> throw new InternalError("unknown type: " + type);
perhaps
mv.visitInsn(switch(type) { ...
-
PR: https://git.openjdk.java.net/jdk/pull/4312
RFR: 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently
Please find below a fix that will make `java/net/DatagramSocket/DatagramSocketMulticasting.java` more resilient to the rare case where addresses bound to a network interfaces are updated while the test is running. Instead of using `NetworkInterface::equals` to compare network interfaces, the test now use `NetworkConfiguration::isSameInterface` which only looks at the network interface name and index and ignore the addresses which are bound to it. - Commit messages: - 8264975: java/net/DatagramSocket/DatagramSocketMulticasting.java fails infrequently Changes: https://git.openjdk.java.net/jdk/pull/4313/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4313&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264975 Stats: 36 lines in 3 files changed: 32 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4313.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4313/head:pull/4313 PR: https://git.openjdk.java.net/jdk/pull/4313
Integrated: 8267521: Post JEP 411 refactoring: maximum covering > 50K
On Fri, 21 May 2021 01:52:27 GMT, Weijun Wang wrote:
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value if not void. The local variable will be called `tmp` if
> later reassigned or `dummy` if it will be discarded.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
>
> I'll add a copyright year update commit before integration.
This pull request has now been integrated.
Changeset: 508cec75
Author:Weijun Wang
URL:
https://git.openjdk.java.net/jdk/commit/508cec7535cd0ad015d566389bc9e5f53ce4103b
Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod
8267521: Post JEP 411 refactoring: maximum covering > 50K
Reviewed-by: dfuchs, prr
-
PR: https://git.openjdk.java.net/jdk/pull/4138
