Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Fri, 13 May 2022 04:41:03 GMT, ExE Boss wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated copyrights >> Fixed cast style to add a space after cast, (where consistent with file >> style) >> Improved code per review comments in PollSelectors. > > src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128: > >> 126: // timed poll interrupted so need to adjust timeout >> 127: long adjust = System.nanoTime() - startTime; >> 128: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust); > > This will now always assign a negative number to `to`. > > > > `=-` is not a compound assignment, it’s negation followed by a normal > assignment. Well spotted, I don't think that change was intentionally. - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8277493: [REDO] Quarantined jpackage apps are labeled as "damaged"
On Wed, 4 May 2022 03:56:10 GMT, Alexander Matveev wrote: > - No changes to code provided by original fix. > - Added ad hoc signing on macOS aarch64, since macOS aarch64 cannot execute > unsigned code and code should be at least ad hoc signed. > - Signing of app bundle produced by AppContentTest.java fails, since it has > invalid app bundle structure with added files into Content folder. Thus test > is executed but failure is always expected on macOS aarch64. 8277493: [REDO] Quarantined jpackage apps are labeled as "damaged" [v2] - Modified test to use Parameter instead of Parameters - Combined unsignAppBundle(), signAppBundle() and signAdHocAppBundle() into one function to reduce code duplication. Calling signAppBundle() without any identity will result in just unsigning app. - PR: https://git.openjdk.java.net/jdk/pull/8527
Re: RFR: 8277493: [REDO] Quarantined jpackage apps are labeled as "damaged" [v2]
> - No changes to code provided by original fix. > - Added ad hoc signing on macOS aarch64, since macOS aarch64 cannot execute > unsigned code and code should be at least ad hoc signed. > - Signing of app bundle produced by AppContentTest.java fails, since it has > invalid app bundle structure with added files into Content folder. Thus test > is executed but failure is always expected on macOS aarch64. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8277493: [REDO] Quarantined jpackage apps are labeled as "damaged" [v2] - Changes: - all: https://git.openjdk.java.net/jdk/pull/8527/files - new: https://git.openjdk.java.net/jdk/pull/8527/files/c163503a..e71647b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8527=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8527=00-01 Stats: 291 lines in 2 files changed: 20 ins; 219 del; 52 mod Patch: https://git.openjdk.java.net/jdk/pull/8527.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8527/head:pull/8527 PR: https://git.openjdk.java.net/jdk/pull/8527
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Wed, 11 May 2022 16:30:41 GMT, Roger Riggs wrote: >> PR#8599 8244681: proposes to add compiler warnings for possible lossy >> conversions >> From the CSR: >> >> "If the type of the right-hand operand of a compound assignment is not >> assignment compatible with the type of the variable, a cast is implied and >> possible lossy conversion may silently occur. While similar situations are >> resolved as compilation errors for primitive assignments, there are no >> similar rules defined for compound assignments." >> >> This PR anticipates the warnings and adds explicit casts to replace the >> implicit casts. >> In most cases, the cast matches the type of the right-hand side to type of >> the compound assignment. >> Since these casts are truncating the value, there might be a different >> refactoring that avoids the cast >> but a direct replacement was chosen here. >> >> (Advise and suggestions will inform similar updates to other OpenJDK >> modules). > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyrights > Fixed cast style to add a space after cast, (where consistent with file > style) > Improved code per review comments in PollSelectors. src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128: > 126: // timed poll interrupted so need to adjust timeout > 127: long adjust = System.nanoTime() - startTime; > 128: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust); This will now always assign a negative number to `to`. `=-` is not a compound assignment, it’s negation followed by a normal assignment. - PR: https://git.openjdk.java.net/jdk/pull/8642
RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
Hi all, Some tests fail with Shenandoah GC after JDK-8282191. The reason is that the assert in `ShenandoahControlThread::request_gc` misses the case of `GCCause::_codecache_GC_threshold`. It would be better to fix it. Thanks. Best regards, Jie - Commit messages: - ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold Changes: https://git.openjdk.java.net/jdk/pull/8691/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8691=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286681 Stats: 17 lines in 2 files changed: 17 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8691/head:pull/8691 PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store [v2]
On Fri, 13 May 2022 01:35:40 GMT, Xiaohong Gong wrote: >> Checking whether the indexes of masked lanes are inside of the valid memory >> boundary is necessary for masked vector memory access. However, this could >> be saved if the given offset is inside of the vector range that could make >> sure no IOOBE (IndexOutOfBoundaryException) happens. The masked load APIs >> have saved this kind of check for common cases. And this patch did the >> similar optimization for the masked vector store. >> >> The performance for the new added store masked benchmarks improves about >> `1.83x ~ 2.62x` on a x86 system: >> >> Benchmark BeforeAfter Gain Units >> StoreMaskedBenchmark.byteStoreArrayMask 12757.936 23291.118 1.826 ops/ms >> StoreMaskedBenchmark.doubleStoreArrayMask 1520.932 3921.616 2.578 ops/ms >> StoreMaskedBenchmark.floatStoreArrayMask 2713.031 7122.535 2.625 ops/ms >> StoreMaskedBenchmark.intStoreArrayMask 4113.772 8220.206 1.998 ops/ms >> StoreMaskedBenchmark.longStoreArrayMask1993.986 4874.148 2.444 ops/ms >> StoreMaskedBenchmark.shortStoreArrayMask 8543.593 17821.086 2.086 ops/ms >> >> Similar performane gain can also be observed on ARM hardware. > > Xiaohong Gong has updated the pull request incrementally with one additional > commit since the last revision: > > Wrap the offset check into a static method However, we seem to lack the ability to do an unsigned comparison reliably. C2 can transform `x + MIN_VALUE <=> y + MIN_VALUE` into `x u<=> y` but it will fail if `x` or `y` is an addition with constant in such cases the constants will be merged together. As a result, I think we need an intrinsic for this. `Integer.compareUnsigned` may fit but it manifests the result into an integer register which may lead to suboptimal materialisation of flags, another approach would be to have a separate method `Integer.lessThanUnsigned` which only returns `boolean` and C2 can have better time splitting the boolean comparison through `IfNode`, which will prevent the materialisation of `boolean` values. What do you two think? I.e, after splitting if through merge point, the shape of `if (Integer.lessThanUnsigned(a, b))` would be transformed from ab \ / CmpU | Bool | If / \ IfTrueIfFalse \ / Region10 \ | / Phi 0 \ / CmpI into ab \ / CmpU Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store
On Fri, 13 May 2022 01:27:18 GMT, Xiaohong Gong wrote: >> Maybe we could use `a.length - vsp.length() > 0 && offset u< a.length - >> vsp.length()` which would hoist the first check outside of the loop. >> Thanks. > >> Maybe we could use `a.length - vsp.length() > 0 && offset u< a.length - >> vsp.length()` which would hoist the first check outside of the loop. Thanks. > > Thanks for the review @merykitty ! We need the check `offset >= 0` which I > think is different from `a.length - vsp.length()`. @XiaohongGong `a >= 0 && a < b` is the same as `b >= 0 && a u< b`, it is how we are doing range check today. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8620
Integrated: 8286623: Bundle zlib by default with JDK on macos aarch64
On Thu, 12 May 2022 08:43:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change to the build system which now makes > bundled zlib as the default for macos aarch64 systems? > > We have been running into various intermittent failures on macos aarch64 > systems for several months now. After investigation we have been able to > ascertain that the root cause of the issue lies within the zlib that's > shipped on macos aarch64 systems. The complete details about that issue is > available at https://bugs.openjdk.java.net/browse/JDK-8282954. > We have filed a bug with Apple for their inputs on what we can do to fix it > in that shipped library. Given the nature of that issue, we don't have a > timeline on when/if the solution for that will be available. Until that time, > at least, the proposal is to use bundled zlib (which doesn't reproduce those > failures) by default on macos aarch64. This pull request has now been integrated. Changeset: c3bade2e Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/c3bade2e08f865bf1e65d48e6d27bff9c022d35f Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod 8286623: Bundle zlib by default with JDK on macos aarch64 Reviewed-by: lancea, ihse, erikj - PR: https://git.openjdk.java.net/jdk/pull/8675
Re: RFR: 8286623: Bundle zlib by default with JDK on macos aarch64
On Thu, 12 May 2022 08:43:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change to the build system which now makes > bundled zlib as the default for macos aarch64 systems? > > We have been running into various intermittent failures on macos aarch64 > systems for several months now. After investigation we have been able to > ascertain that the root cause of the issue lies within the zlib that's > shipped on macos aarch64 systems. The complete details about that issue is > available at https://bugs.openjdk.java.net/browse/JDK-8282954. > We have filed a bug with Apple for their inputs on what we can do to fix it > in that shipped library. Given the nature of that issue, we don't have a > timeline on when/if the solution for that will be available. Until that time, > at least, the proposal is to use bundled zlib (which doesn't reproduce those > failures) by default on macos aarch64. Thank you everyone for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/8675
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v3]
On Thu, 12 May 2022 16:07:54 GMT, Paul Sandoz wrote: > Yes, the tests were run in debug mode. The reporting of the missing constant > occurs for the compiled method that is called from the method where the > constants are declared e.g.: > > ``` > 719 240bjdk.incubator.vector.Int256Vector::fromArray0 (15 > bytes) > ** Rejected vector op (LoadVectorMasked,int,8) because architecture does > not support it > ** missing constant: offsetInRange=Parm > @ 11 > jdk.incubator.vector.IntVector::fromArray0Template (22 bytes) force inline > by annotation > ``` > > So it appears to be working as expected. A similar pattern occurs at a > lower-level for the passing of the mask class. `Int256Vector::fromArray0` > passes a constant class to `IntVector::fromArray0Template` (the compilation > of which bails out before checking that the `offsetInRange` is constant). Make sense to me! I think I understand what you mean. I will have more tests with the integer constant change. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store [v2]
> Checking whether the indexes of masked lanes are inside of the valid memory > boundary is necessary for masked vector memory access. However, this could be > saved if the given offset is inside of the vector range that could make sure > no IOOBE (IndexOutOfBoundaryException) happens. The masked load APIs have > saved this kind of check for common cases. And this patch did the similar > optimization for the masked vector store. > > The performance for the new added store masked benchmarks improves about > `1.83x ~ 2.62x` on a x86 system: > > Benchmark BeforeAfter Gain Units > StoreMaskedBenchmark.byteStoreArrayMask 12757.936 23291.118 1.826 ops/ms > StoreMaskedBenchmark.doubleStoreArrayMask 1520.932 3921.616 2.578 ops/ms > StoreMaskedBenchmark.floatStoreArrayMask 2713.031 7122.535 2.625 ops/ms > StoreMaskedBenchmark.intStoreArrayMask 4113.772 8220.206 1.998 ops/ms > StoreMaskedBenchmark.longStoreArrayMask1993.986 4874.148 2.444 ops/ms > StoreMaskedBenchmark.shortStoreArrayMask 8543.593 17821.086 2.086 ops/ms > > Similar performane gain can also be observed on ARM hardware. Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision: Wrap the offset check into a static method - Changes: - all: https://git.openjdk.java.net/jdk/pull/8620/files - new: https://git.openjdk.java.net/jdk/pull/8620/files/eb67f681..2229dd24 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8620=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8620=00-01 Stats: 56 lines in 8 files changed: 5 ins; 0 del; 51 mod Patch: https://git.openjdk.java.net/jdk/pull/8620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8620/head:pull/8620 PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store [v2]
On Thu, 12 May 2022 03:36:31 GMT, Paul Sandoz wrote: >> Thanks for the review @PaulSandoz ! For the >> `VectorIntrinsics.checkFromIndexSize`, I'm afraid it's not suitable to be >> used here because the `outOfBounds` exception will be thrown if the offset >> is not inside of the valid array boundary. And for the masked operations, >> this is not needed since we only need to check the masked lanes. Please >> correct me if I didn't understand correctly. Thanks! > > Silly me! i commented too quickly, `checkFromIndexSize` cannot be used. My > intent was that we could use a method rather than repeating the expression in > numerous places (including masked loads IIRC). Hi @PaulSandoz , I'v updated the offset check for masked load/store. Could you please help to check whether it is ok? Thanks so much! - PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8286279: [vectorapi] Only check index of masked lanes if offset is out of array boundary for masked store
On Thu, 12 May 2022 09:49:17 GMT, Quan Anh Mai wrote: > Maybe we could use `a.length - vsp.length() > 0 && offset u< a.length - > vsp.length()` which would hoist the first check outside of the loop. Thanks. Thanks for the review @merykitty ! We need the check `offset >= 0` which I think is different from `a.length - vsp.length()`. - PR: https://git.openjdk.java.net/jdk/pull/8620
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Reviewed. I also added `noreg-trivial` labels to the bug reports. - PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v3]
On Tue, 10 May 2022 12:48:25 GMT, Jatin Bhateja wrote:
>> Hi All,
>>
>> Patch adds the planned support for new vector operations and APIs targeted
>> for [JEP 426: Vector API (Fourth
>> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173)
>>
>> Following is the brief summary of changes:-
>>
>> 1) Extends the scope of existing lanewise API for following new vector
>> operations.
>>- VectorOperations.BIT_COUNT: counts the number of one-bits
>>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero
>> bits
>>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing
>> zero bits
>>- VectorOperations.REVERSE: reversing the order of bits
>>- VectorOperations.REVERSE_BYTES: reversing the order of bytes
>>- compress and expand bits: Semantics are based on Hacker's Delight
>> section 7-4 Compress, or Generalized Extract.
>>
>> 2) Adds following new APIs to perform cross lane vector compress and
>> expansion operations under the influence of a mask.
>>- Vector.compress
>>- Vector.expand
>>- VectorMask.compress
>>
>> 3) Adds predicated and non-predicated versions of following new APIs to load
>> and store the contents of vector from foreign MemorySegments.
>> - Vector.fromMemorySegment
>> - Vector.intoMemorySegment
>>
>> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support
>> for each newly added operation.
>>
>>
>> Patch has been regressed over AARCH64 and X86 targets different AVX levels.
>>
>> Kindly review and share your feedback.
>>
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 11 commits:
>
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
> - 8284960: Correcting a typo.
> - 8284960: Integrating changes from panama-vector (Add @since 19 tags).
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
> - 8284960: AARCH64 backend changes.
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/3fa1c404...b021e082
Overall, looks good.
Some minor questions/suggestions follow.
src/hotspot/cpu/aarch64/aarch64_neon.ad line 5700:
> 5698: as_FloatRegister($dst$$reg));
> 5699: }
> 5700: if (bt == T_INT) {
I find it hard to reason about the code in its current form.
Maybe make the second `if` (`bt == T_INT`) nested and move it under `if (bt ==
T_SHORT || bt == T_INT)`?
src/hotspot/cpu/x86/macroAssembler_x86.cpp line 2587:
> 2585:
> 2586: void MacroAssembler::vmovdqu(XMMRegister dst, AddressLiteral src,
> Register scratch_reg, int vector_len) {
> 2587: assert(vector_len <= AVX_512bit, "unexpected vector length");
The assert becomes redundant.
src/hotspot/cpu/x86/matcher_x86.hpp line 195:
> 193: case Op_PopCountVI:
> 194: return ((ety == T_INT &&
> VM_Version::supports_avx512_vpopcntdq()) ||
> 195:(is_subword_type(ety) &&
> VM_Version::supports_avx512_bitalg())) ? 0 : 50;
Should be easier to read when the condition is split. E.g.:
if (is_subword_type(ety)) {
return VM_Version::supports_avx512_bitalg())) ? 0 : 50;
} else {
assert(ety == T_INT, "sanity"); // for documentation purposes
return VM_Version::supports_avx512_vpopcntdq() ? 0 : 50;
}
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 7953:
> 7951: StubRoutines::x86::_vector_iota_indices =
> generate_iota_indices("iota_indices");
> 7952:
> 7953: if (UsePopCountInstruction && VM_Version::supports_avx2() &&
> !VM_Version::supports_avx512_vpopcntdq()) {
Why is the LUT unconditionally generated? `UsePopCountInstruction` still guides
the usages.
src/hotspot/cpu/x86/vm_version_x86.hpp line 375:
> 373: decl(RDTSCP,"rdtscp",48) /* RDTSCP
> instruction */ \
> 374: decl(RDPID, "rdpid", 49) /* RDPID
> instruction */ \
> 375: decl(FSRM, "fsrm", 50) /* Fast Short REP
> MOV */ \
`test/lib-test/jdk/test/whitebox/CPUInfoTest.java` should be adjusted as well,
shouldn't it?
src/hotspot/cpu/x86/x86.ad line 2113:
> 2111:
> 2112: case Op_CountLeadingZerosV:
> 2113: if ((bt == T_INT || bt == T_LONG) &&
> VM_Version::supports_avx512cd()) {
Newly introduced `is_non_subword_integral_type(bt)` can be used here instead of
`bt == T_INT || bt == T_LONG`.
src/hotspot/share/classfile/vmIntrinsics.hpp line 1152:
> 1150:
> "Ljdk/internal/vm/vector/VectorSupport$ComExpOperation;)"
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
On Thu, 12 May 2022 20:59:53 GMT, Michael Hall wrote: > However, I don't know how the app is supposed to be started without a > launcher... jpackage supplies an alternative launcher that doesn't have plist. - PR: https://git.openjdk.java.net/jdk/pull/8666
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline
Ok, let us know when it's out of draft, i've several minor modifications about the code. regards, Rémi - Original Message - > From: "Jorn Vernee" > To: "core-libs-dev" , "hotspot-dev" > > Sent: Thursday, May 12, 2022 11:25:52 PM > Subject: Re: RFR: 8286669: Replace MethodHandle specialization with ASM in > mainline > On Thu, 12 May 2022 17:17:37 GMT, Jorn Vernee wrote: > >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly >> pertain to the switch to ASM and away from MethodHandle combinators for the >> binding recipe specialization. But, there is one more commit which adds >> freeing >> of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn > > Hmm, looks like there's an unexpected a tier1 failure on SysV in the GHA (I > only > tested Windows locally, since these changes have been live in the panama repo > for a while now...). Will convert back to draft and see what's up. > > - > > PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286594: (zipfs) Mention paths with dot elements in ZipException and cleanups
On Thu, 12 May 2022 15:07:29 GMT, Sean Mullan wrote: > A more descriptive title for the bug report would be helpful. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/8655
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline
On Thu, 12 May 2022 17:17:37 GMT, Jorn Vernee wrote: > Hi, > > This PR brings over commits from the panama-foreign repo. These commits > mostly pertain to the switch to ASM and away from MethodHandle combinators > for the binding recipe specialization. But, there is one more commit which > adds freeing of downcall stubs, since those changes were mostly Java as well. > > Thanks, > Jorn Hmm, looks like there's an unexpected a tier1 failure on SysV in the GHA (I only tested Windows locally, since these changes have been live in the panama repo for a while now...). Will convert back to draft and see what's up. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe [v2]
> On May 12, 2022, at 3:13 PM, Magnus Ihse Bursie wrote: > > On Thu, 12 May 2022 04:15:50 GMT, Alexander Matveev > wrote: > >>> - It is not possible to support native JDK commands such as "java" inside >>> Mac App Store bundles due to embedded info.plist. Workarounds suggested in >>> JDK-8286122 does not seems to be visible. >>> - With proposed fix we will enforce "--strip-native-commands" option for >>> jlink, so native JDK commands are not included when generating Mac App >>> Store bundles. >>> - Custom runtime provided via --runtime-image should not contain native >>> commands as well, otherwise jpackage will throw error. >>> - Added two tests to validate fix. >> >> Alexander Matveev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8286122: [macos]: App bundle cannot upload to Mac App Store due to >> info.plist embedded in java exe [v2] > > We have the `NSMicrophoneUsageDescription` permission on the `java` launcher > in the JDK, since otherwise no Java program can access the mike, even though > most won't care. I agree that the situation is different for a jpackaged app, > where the developer knows if that permission is needed or not. I’d have to agree with Apple DTS that this is an interesting exception. > > Yes, plistbuddy is an official Apple program. > > My understanding of the PR was that native commands are removed by jlink if > the user is packaging on a mac for the App Store. I thought this was a > workaround that solved the immediate problem of not being able to submit the > app to App Store. (However, I don't know how the app is supposed to be > started without a launcher…) > I thought that if the app was indicated as intended for the App Store and native commands were also indicated an error would be thrown and the app not built. It doesn’t allow apps that will fail to attempt the App Store but does nothing for getting them there. Switching from an error to a warning and forcing the native commands to be stripped would allow the app into the app store but with unknown functionality probably not working. My understanding.
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe [v2]
On Thu, 12 May 2022 04:15:50 GMT, Alexander Matveev wrote: >> - It is not possible to support native JDK commands such as "java" inside >> Mac App Store bundles due to embedded info.plist. Workarounds suggested in >> JDK-8286122 does not seems to be visible. >> - With proposed fix we will enforce "--strip-native-commands" option for >> jlink, so native JDK commands are not included when generating Mac App Store >> bundles. >> - Custom runtime provided via --runtime-image should not contain native >> commands as well, otherwise jpackage will throw error. >> - Added two tests to validate fix. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8286122: [macos]: App bundle cannot upload to Mac App Store due to > info.plist embedded in java exe [v2] We have the `NSMicrophoneUsageDescription` permission on the `java` launcher in the JDK, since otherwise no Java program can access the mike, even though most won't care. I agree that the situation is different for a jpackaged app, where the developer knows if that permission is needed or not. Yes, plistbuddy is an official Apple program. My understanding of the PR was that native commands are removed by jlink if the user is packaging on a mac for the App Store. I thought this was a workaround that solved the immediate problem of not being able to submit the app to App Store. (However, I don't know how the app is supposed to be started without a launcher...) - PR: https://git.openjdk.java.net/jdk/pull/8666
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec [v2]
On Thu, 12 May 2022 20:04:33 GMT, Joe Darcy wrote: >> While doing a CSR review of another issue, I noticed some cases in >> InputStream and OutputStream what would benefit from being upgraded to >> implSpec and related javadoc tags. >> >> The "A subclass must provide an implementation of this method." statements >> on several abstract methods don't add much value, but I chose to leave them >> in for this request. >> >> Please also review the corresponding CSR: >> https://bugs.openjdk.java.net/browse/JDK-8286605 > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. CSR also updated to reflect changes made in response to review comments; thanks. - PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec [v2]
On Wed, 11 May 2022 22:43:43 GMT, liach wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Respond to review feedback.
>
> src/java.base/share/classes/java/io/OutputStream.java line 151:
>
>> 149: * The {@code write} method of {@code OutputStream} calls
>> 150: * the write method of one argument on each of the bytes to be
>> 151: * written out. Subclasses are encouraged to override this method
>> and
>
> Shouldn't the "subclasses" information be part of the API Note?
Sure; moved to an apiNote.
-
PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec [v2]
> While doing a CSR review of another issue, I noticed some cases in > InputStream and OutputStream what would benefit from being upgraded to > implSpec and related javadoc tags. > > The "A subclass must provide an implementation of this method." statements on > several abstract methods don't add much value, but I chose to leave them in > for this request. > > Please also review the corresponding CSR: > https://bugs.openjdk.java.net/browse/JDK-8286605 Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8663/files - new: https://git.openjdk.java.net/jdk/pull/8663/files/a29e5c36..7d88f44c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8663=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8663=00-01 Stats: 13 lines in 2 files changed: 3 ins; 8 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8663/head:pull/8663 PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec
On Thu, 12 May 2022 12:24:17 GMT, Alan Bateman wrote:
>> While doing a CSR review of another issue, I noticed some cases in
>> InputStream and OutputStream what would benefit from being upgraded to
>> implSpec and related javadoc tags.
>>
>> The "A subclass must provide an implementation of this method." statements
>> on several abstract methods don't add much value, but I chose to leave them
>> in for this request.
>>
>> Please also review the corresponding CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8286605
>
> src/java.base/share/classes/java/io/InputStream.java line 177:
>
>> 175: *
>> 176: * @apiNote
>> 177: * A subclass must provide an implementation of this method.
>
> Is this sentence useful to keep? The method is abstract so a concrete
> implementation has to implement it. On the other other hand, an abstract
> subclass does not need to implement it.
If such a sentence occurred in new code, I would recommend it be removed. I
left it in place in the spirit of just adding apiNote, implSpec, etc., but I'm
happy to delete these comments too. I assume it was deemed useful to readers of
JDK 1.0, but the assumed background of Java developers now is rather different
:-)
> src/java.base/share/classes/java/io/InputStream.java line 688:
>
>> 686: * @implSpec
>> 687: * The {@code mark} method of {@code InputStream} does
>> 688: * nothing.
>
> Minor nit but the line break can be removed so that "nothing" is on the same
> line.
Sure. (I default to not making such reflow changes in the initial version of a
patch to avoid spurious diffs.
-
PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]
On Thu, 12 May 2022 01:27:30 GMT, Yasumasa Suenaga wrote: >> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 >> on Fedora 36. >> As you can see, the warnings spreads several areas. Let me know if I should >> separate them by area. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Calculate char offset before realloc() I will see what upstream thinks about the harfbuzz warning but in the mean time we can just disable it. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v8]
On Thu, 12 May 2022 18:02:26 GMT, Shruthi wrote: >> Removing the Duplicate keys present in XSLTErrorResources.java and >> XPATHErrorResources.java >> >> The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 > > Shruthi has refreshed the contents of this pull request, and previous commits > have been removed. The incremental views will show differences compared to > the previous content of the PR. The pull request contains one new commit > since the last revision: > > Modify copyright year They all look good now. Thanks. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v3]
On Thu, 12 May 2022 16:01:50 GMT, Brian Burkhalter wrote: >> Modify the specification of `SequenceInputStream.read(byte[],int,int)` to >> indicate that `-1` is returned at the EOF of the last stream even if `len` >> is zero. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8286200: Change @throws NPE clause of read(byte[],int,int) to better match > specification verbiage In the throws clauses, I think I would have put the additional conditional at the end of the sentence since the existing throws text corresponds to the exception. But the logic is correct as is. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]
On Thu, 12 May 2022 01:29:13 GMT, Yasumasa Suenaga wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Calculate char offset before realloc() > > Thanks for all to review this PR! I think we should separate this issue as > following: > > * Suppress warnings > * make/modules/java.desktop/lib/Awt2dLibraries.gmk > * src/hotspot/share/classfile/bytecodeAssembler.cpp > * src/hotspot/share/classfile/classFileParser.cpp > * > src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp > * src/hotspot/share/opto/memnode.cpp > * src/hotspot/share/opto/type.cpp > * src/hotspot/share/utilities/compilerWarnings.hpp > * src/hotspot/share/utilities/compilerWarnings_gcc.hpp > * src/java.base/unix/native/libjli/java_md_common.c > * Bug fixes > * src/java.base/share/native/libjli/java.c > * src/java.base/share/native/libjli/parse_manifest.c > * src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c > > I want to include the change of Awt2dLibraries.gmk (HarfBuzz) in this PR > because it is 3rd party library. > > I will separate in above if I do not hear any objections, and this issue (PR) > handles "suppress warnings" only. > @YaSuenag From my PoV this sounds like a good suggestion. +1 - PR: https://git.openjdk.java.net/jdk/pull/8646
Integrated: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach()
On Wed, 11 May 2022 23:08:32 GMT, Ioi Lam wrote: > The API `jdk.internal.perf.Perf.::attach(String user, int lvmid)` is never > used. It should be removed, and all the handling of a specified user name > should be removed. This pull request has now been integrated. Changeset: 74eee28a Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/74eee28a710f2d0c9f613522ee3d228d6b601252 Stats: 123 lines in 5 files changed: 2 ins; 97 del; 24 mod 8286560: Remove user parameter from jdk.internal.perf.Perf.attach() Reviewed-by: dholmes, alanb - PR: https://git.openjdk.java.net/jdk/pull/8669
Re: RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach() [v2]
On Thu, 12 May 2022 04:06:44 GMT, David Holmes wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @AlanBateman comments - remove thros IllegalArgumentException clause > > Nice cleanup! I checked back in JDK 7 and couldn't find any use of this > particular API. > > Thanks. Thanks to @dholmes-ora and @AlanBateman for the review. - PR: https://git.openjdk.java.net/jdk/pull/8669
Re: RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach() [v2]
On Thu, 12 May 2022 14:08:11 GMT, Alan Bateman wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @AlanBateman comments - remove thros IllegalArgumentException clause > > src/java.base/share/classes/jdk/internal/perf/Perf.java line 246: > >> 244: */ >> 245: private native ByteBuffer attach0(int lvmid) >> 246:throws IllegalArgumentException, IOException; > > You can drop the "throws IllegalArgumentException" here if you want, it's not > needed as it's a runtime exception. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/8669
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline
On Thu, 12 May 2022 17:17:37 GMT, Jorn Vernee wrote:
> Hi,
>
> This PR brings over commits from the panama-foreign repo. These commits
> mostly pertain to the switch to ASM and away from MethodHandle combinators
> for the binding recipe specialization. But, there is one more commit which
> adds freeing of downcall stubs, since those changes were mostly Java as well.
>
> Thanks,
> Jorn
test/micro/org/openjdk/bench/java/lang/foreign/LinkUpcall.java line 61:
> 59: BLANK = lookup().findStatic(LinkUpcall.class, "blank",
> MethodType.methodType(void.class));
> 60: } catch (ReflectiveOperationException e) {
> 61: throw new BootstrapMethodError(e);
You probably mean exception in initializer error.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 12 May 2022 15:58:57 GMT, Severin Gehwolf wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 113:
>>
>>> 111: }
>>> 112: buf[MAXPATHLEN-1] = '\0';
>>> 113: _path = os::strdup(buf);
>>
>> I think this code can be simplified a lot with stringStream and without
>> strtok, so no need for fixed buffers (which may fail with longer path names)
>> and no need for writable string copies on the stack.
>>
>> Something like this:
>>
>> stringStream ss;
>> ss.print_raw(_mount_point);
>> const char* p1 = _root;
>> const char* p2 = cgroup_path;
>> int last_matching_dash_pos = -1;
>> for (int i = 0; *p1 == *p2 && *p1 != 0; i ++) {
>> if (*p1 == '/') {
>> last_matching_dash_pos = i;
>> }
>> p1++; p2++;
>> }
>> ss.print_raw(_root, last_matching_dash_pos);
>> // Now use ss.base() to access the assembled string
>
> Nice, thanks! I'll update it.
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
> Please review this change to the cgroup v1 subsystem which makes it more > resilient on some of the stranger systems. Unfortunately, I wasn't able to > re-create a similar system as the reporter. The idea of using the longest > substring match for the cgroupv1 file paths was based on the fact that on > systemd systems processes run in separate scopes and the maven forked test > runner might exhibit this property. For that it makes sense to use the common > ancestor path. Nothing changes in the common cases where the `cgroup_path` > matches `_root` and when the `_root` is `/` (container the former, host > system the latter). > > In addition, the code paths which are susceptible to throw NPE have been > hardened to catch those situations. Should it happen in the future it makes > more sense (to me) to not have accurate container detection in favor of > continuing to keep running. > > Finally, with the added unit-tests a bug was uncovered on the "substring" > match case of cgroup paths in hotspot. `p` returned from `strstr` can never > point to `_root` as it's used as the "needle" to find in "haystack" > `cgroup_path` (not the other way round). > > Testing: > - [x] Added unit tests > - [x] GHA > - [x] Container tests on cgroups v1 Linux. Continue to pass Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Use stringStream to simplify controller path assembly - Changes: - all: https://git.openjdk.java.net/jdk/pull/8629/files - new: https://git.openjdk.java.net/jdk/pull/8629/files/bc873b3f..66241aa5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8629=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8629=00-01 Stats: 32 lines in 1 file changed: 0 ins; 21 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8629.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8629/head:pull/8629 PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v8]
> Removing the Duplicate keys present in XSLTErrorResources.java and > XPATHErrorResources.java > > The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 Shruthi has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Modify copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/8318/files - new: https://git.openjdk.java.net/jdk/pull/8318/files/283f8ef9..bcad12ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8318=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8318=06-07 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8318.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8318/head:pull/8318 PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8683
RFR: 8286669: Replace MethodHandle specialization with ASM in mainline
Hi, This PR brings over commits from the panama-foreign repo. These commits mostly pertain to the switch to ASM and away from MethodHandle combinators for the binding recipe specialization. But, there is one more commit which adds freeing of downcall stubs, since those changes were mostly Java as well. Thanks, Jorn - Depends on: https://git.openjdk.java.net/jdk/pull/7959 Commit messages: - Fix LinkUpcall benchmark - 8286306: Upcall wrapper class sharing - Polish - 8281595: ASM-ify scope acquire/release for down call parameters - 8281228: Preview branch's CLinker.downcallHandle crashes inside asm - 8278414: Replace binding recipe customization using MH combinators with bytecode spinning - 8276648: Downcall stubs are never freed Changes: https://git.openjdk.java.net/jdk/pull/8685/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8685=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286669 Stats: 2171 lines in 24 files changed: 1384 ins; 660 del; 127 mod Patch: https://git.openjdk.java.net/jdk/pull/8685.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8685/head:pull/8685 PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe [v2]
On Thu, 12 May 2022 04:15:50 GMT, Alexander Matveev wrote: >> - It is not possible to support native JDK commands such as "java" inside >> Mac App Store bundles due to embedded info.plist. Workarounds suggested in >> JDK-8286122 does not seems to be visible. >> - With proposed fix we will enforce "--strip-native-commands" option for >> jlink, so native JDK commands are not included when generating Mac App Store >> bundles. >> - Custom runtime provided via --runtime-image should not contain native >> commands as well, otherwise jpackage will throw error. >> - Added two tests to validate fix. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8286122: [macos]: App bundle cannot upload to Mac App Store due to > info.plist embedded in java exe [v2] Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8666
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Thu, 12 May 2022 12:10:53 GMT, Maurizio Cimadamore wrote: >> Okay, I see. I think I acted a little too hastily on this yesterday. I'll >> revert the change that uses this blocking mechanism. >> >> The stack more or less looks like this during an upcall: >> >> >> | --- >> | | >> | --- >> | <1: user define try block with exception handler (maybe)> | >> | --- >> | <2: user code start> | >> | --- >> | <3: method handle impl frames 1> | >> | --- >> | <4: upcall wrapper class with fallback handler 1> | >> | --- >> | <5: method handle impl frames 2> | >> | --- >> | <6: upcallk stub with fallback handler 2> | >> | <7: unknown native code> >> | --- >> | | >> >> >> I think there are several options to address async exceptions: >> 1. Do nothing special for async exceptions. i.e. if they happen anywhere >> between 1. and 6. they will end up in one of the fallback handlers and the >> VM will be terminated. >> 2. Block async exceptions in all code up from 6. >> 3. Somehow only block async exceptions only between 6. and 1. >> I think that is possible by changing the API so that the user passes us >> a method handle to their fallback exception handler, and we invoke it in our >> code in 4. We would need 2 methods for blocking and unblocking async >> exceptions from Java. Then we could disable async exceptions at the start of >> 6. enabled them at the start of the try block in 4. (around the call to user >> code), and disable them at the end of this try block. Then finally re-enable >> them at the end of 6. If an exception occurs in the try block in 4., >> delegate to the user-defined exception handler (but use the VM terminate >> strategy as a fallback for when another exception occurs). The other problem >> I see with that is that to make that fast enough (i.e. not incur a ~1.5-2x >> cost on call overhead), we would need compiler intrinsics for the >> blocking/unblocking, and in the past we've been unable to define 'critical' >> sections of code like that in C2 (it's an unsolved problem at this point). > >> Do nothing special for async exceptions. i.e. if they happen anywhere >> between 1. and 6. they will end up in one of the fallback handlers and the >> VM will be terminated. > > My understanding is that if they materialize while we're executing the upcall > Java code, if that code has a try/catch block, we will go there, rather than > crash the VM. > > In other words, IMHO the only problem with async exception is if they occur > _after_ the Java user code has completed, because that will crash the Java > adapter, this preventing it from returning to native call cleanly. > > So, either we disable async exceptions during that phase (e.g. after user > code has executed, but before we return back to native code), or we just punt > and stop. Since this seems like a corner^3 case, and since there are also > other issue with upcalls that can occur if other threads do not cooperate > (e.g. an upcall can get stuck into an infinite safepoint if the VM exits > while an async native thread runs the upcall), and given that obtaining a > linker is a restricted operation anyway, I don't think we should bend over > backwards to try to add 1% more safety to something that's unavoidably sharp > anyways. Ok. Then, if no one objects, I will leave this area as-is for now. (and perhaps come back to this issue in the future, if it becomes more pressing). - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach() [v2]
> The API `jdk.internal.perf.Perf.::attach(String user, int lvmid)` is never > used. It should be removed, and all the handling of a specified user name > should be removed. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @AlanBateman comments - remove thros IllegalArgumentException clause - Changes: - all: https://git.openjdk.java.net/jdk/pull/8669/files - new: https://git.openjdk.java.net/jdk/pull/8669/files/afa66232..0b73a9d4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8669=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8669=00-01 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8669.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8669/head:pull/8669 PR: https://git.openjdk.java.net/jdk/pull/8669
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v3]
On Thu, 12 May 2022 16:01:50 GMT, Brian Burkhalter wrote: >> Modify the specification of `SequenceInputStream.read(byte[],int,int)` to >> indicate that `-1` is returned at the EOF of the last stream even if `len` >> is zero. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8286200: Change @throws NPE clause of read(byte[],int,int) to better match > specification verbiage _(not a reviewer)_ Looks good - Marked as reviewed by rgiulie...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/8664
Integrated: JDK-8286615: Small refactor to SerializedLambda
On Thu, 12 May 2022 00:10:28 GMT, Joe Darcy wrote: > Noticed by inspection during a CSR review, small refactoring to use a > message-cause exception constructor when one is available. > > Will update the copyright before a push. This pull request has now been integrated. Changeset: 160944bc Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/160944bc6bd94d2927f398cf7732027c1b836a42 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod 8286615: Small refactor to SerializedLambda Reviewed-by: bpb, iris - PR: https://git.openjdk.java.net/jdk/pull/8670
Re: RFR: 8283689: Update the foreign linker VM implementation [v17]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 98 commits: - Merge branch 'master' into JEP-19-VM-IMPL2 - Undo spurious changes. - Merge branch 'JEP-19-VM-IMPL2' of https://github.com/JornVernee/jdk into JEP-19-VM-IMPL2 - Apply copyright year updates per request of @nick-arm Co-authored-by: Nick Gasson - Fix overwritten copyright years. - Missed 2 years - Update Oracle copyright years - Revert "Block async exceptions during upcalls" This reverts commit b29ad8f46732666f2d07e63ce8701b1eb7bed790. - Block async exceptions during upcalls - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 - ... and 88 more: https://git.openjdk.java.net/jdk/compare/2c5d1362...f55b6c59 - Changes: https://git.openjdk.java.net/jdk/pull/7959/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7959=16 Stats: 6913 lines in 155 files changed: 2576 ins; 3219 del; 1118 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
RFR: 8286393: Address possibly lossy conversions in java.rmi
Updates to modules java.rmi and java.smartcardio to remove warnings about lossy-conversions introduced by PR#8599. Explicit casts are inserted where implicit casts occur. 8286393: Address possibly lossy conversions in java.rmi 8286388: Address possibly lossy conversions in java.smartcardio - Commit messages: - 8286393: Address possibly lossy conversions in java.rmi Changes: https://git.openjdk.java.net/jdk/pull/8683/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8683=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286393 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8683/head:pull/8683 PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: JDK-8286347: incorrect use of `{@link}`
On Fri, 6 May 2022 23:30:44 GMT, Jonathan Gibbons wrote:
> Please review a small doc fix to update incorrect use of `{@link}` on arrays
> of primitive types.
This is fixed in 8282191 (JEP 424), which is already integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/8584
Integrated: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). This pull request has now been integrated. Changeset: 17c52789 Author:Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/17c52789b79a4ccd65308f90c4e02c1732b206be Stats: 71 lines in 32 files changed: 0 ins; 3 del; 68 mod 8286378: Address possibly lossy conversions in java.base Reviewed-by: naoto, xuelei, bpb, alanb - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: JDK-8286615: Small refactor to SerializedLambda [v2]
> Noticed by inspection during a CSR review, small refactoring to use a > message-cause exception constructor when one is available. > > Will update the copyright before a push. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update copyright year. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8670/files - new: https://git.openjdk.java.net/jdk/pull/8670/files/82d7ab2c..6b15c809 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8670=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8670=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8670.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8670/head:pull/8670 PR: https://git.openjdk.java.net/jdk/pull/8670
Re: RFR: 8283689: Update the foreign linker VM implementation [v16]
On Thu, 12 May 2022 16:01:17 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR updates the VM implementation of the foreign linker, by bringing >> over commits from the panama-foreign repo. >> >> This is split off from the main JEP integration for 19, since we have >> limited resources to handle this. As such, this PR might fall over to 20, >> but it would be nice if we could get it into 19. >> >> I've written up an overview of the Linker architecture here: >> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful >> to read that first. >> >> This patch moves from the "legacy" implementation, to what is currently >> implemented in the panama-foreign repo, except for replacing the use of >> method handle combinators with ASM. That will come in a later path. To >> recap. This PR contains the following changes: >> >> 1. VM stubs for downcalls are now generated up front, instead of lazily by >> C2 [1]. >> 2. the VM support for upcalls/downcalls now support all possible call >> shapes. And VM stubs and Java code implementing the buffered invocation >> strategy has been removed [2], [3], [4], [5]. >> 3. The existing C2 intrinsification support for the `linkToNative` method >> handle linker was no longer needed and has been removed [6] (support might >> be re-added in another form later). >> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now >> implements RuntimeBlob directly. Binding to java classes has been rewritten >> to use javaClasses.h/cpp (this wasn't previously possible due to these java >> classes being in an incubator module) [7], [8], [9]. >> >> While the patch mostly consists of VM changes, there are also some Java >> changes to support (2). >> >> The original commit structure has been mostly retained, so it might be >> useful to look at a specific commit, or the corresponding patch in the >> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) >> repo as well. I've also left some inline comments to explain some of the >> changes, which will hopefully make reviewing easier. >> >> Testing: Tier1-4 >> >> Thanks, >> Jorn >> >> [1]: >> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 >> [2]: >> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 >> [3]: >> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 >> [4]: >> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 >> [5]: >> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 >> [6]: >> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a >> [7]: >> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 >> [8]: >> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f >> [9]: >> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Undo spurious changes. The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout JEP-19-VM-IMPL2 git fetch https://git.openjdk.java.net/jdk master git merge FETCH_HEAD # if there are conflicts, follow the instructions given by git merge git commit -m "Merge master" git push - PR: https://git.openjdk.java.net/jdk/pull/7959
Integrated: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore wrote: > This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 This pull request has now been integrated. Changeset: 2c5d1362 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/2c5d136260fa717afa374db8b923b7c886d069b7 Stats: 65711 lines in 373 files changed: 43720 ins; 19432 del; 2559 mod 8282191: Implementation of Foreign Function & Memory API (Preview) Reviewed-by: erikj, jvernee, psandoz, dholmes, mchung - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Thu, 10 Feb 2022 15:25:09 GMT, Aleksei Efimov wrote: >>> The concerns are two folds: >>> >>> * without a regression test, you have to trust that the source changes >>> actually work, and there's typically no verification possible >>> * without a regression test, the next refactoring change in this area two >>> years from now could undo the fix and nobody would notice >>> >> >> Thanks, that was what I initially thought but wanted to check if I was >> missing something else given the previous discussion. I should be able to >> generate a build for the user and ask him to test in his real environment. >> As for the other concern, I'll evaluate options and let you know. > >> Thanks, that was what I initially thought but wanted to check if I was >> missing something else given the previous discussion. I should be able to >> generate a build for the user and ask him to test in his real environment. >> As for the other concern, I'll evaluate options and let you know. > > Thanks for the update, Martin. > I'm ok with pushing the fix without a test given that the user will verify it > in his real environment. > Maybe, you could also log a bug for a test addition and describe in it an > environment, and sort of a high-level scenario of how JDK-8275535 can be > reproduced. Hi @AlekseiEfimov , Apologies for the delay. We had no feedback from our customer. However, we released the fix in our builds and there were no regressions reported. I'll proceed with the integration of this fix. Thanks, Martin.- - PR: https://git.openjdk.java.net/jdk/pull/6043
Integrated: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 This pull request has now been integrated. Changeset: 3be394e1 Author:Martin Balao URL: https://git.openjdk.java.net/jdk/commit/3be394e1606dd17c2c14ce806c796f5eb2b1ad6e Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked Reviewed-by: aefimov, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v3]
On Thu, 5 May 2022 08:56:07 GMT, Xiaohong Gong wrote:
>> Currently the vector load with mask when the given index happens out of the
>> array boundary is implemented with pure java scalar code to avoid the IOOBE
>> (IndexOutOfBoundaryException). This is necessary for architectures that do
>> not support the predicate feature. Because the masked load is implemented
>> with a full vector load and a vector blend applied on it. And a full vector
>> load will definitely cause the IOOBE which is not valid. However, for
>> architectures that support the predicate feature like SVE/AVX-512/RVV, it
>> can be vectorized with the predicated load instruction as long as the
>> indexes of the masked lanes are within the bounds of the array. For these
>> architectures, loading with unmasked lanes does not raise exception.
>>
>> This patch adds the vectorization support for the masked load with IOOBE
>> part. Please see the original java implementation (FIXME: optimize):
>>
>>
>> @ForceInline
>> public static
>> ByteVector fromArray(VectorSpecies species,
>>byte[] a, int offset,
>>VectorMask m) {
>> ByteSpecies vsp = (ByteSpecies) species;
>> if (offset >= 0 && offset <= (a.length - species.length())) {
>> return vsp.dummyVector().fromArray0(a, offset, m);
>> }
>>
>> // FIXME: optimize
>> checkMaskFromIndexSize(offset, vsp, m, 1, a.length);
>> return vsp.vOp(m, i -> a[offset + i]);
>> }
>>
>> Since it can only be vectorized with the predicate load, the hotspot must
>> check whether the current backend supports it and falls back to the java
>> scalar version if not. This is different from the normal masked vector load
>> that the compiler will generate a full vector load and a vector blend if the
>> predicate load is not supported. So to let the compiler make the expected
>> action, an additional flag (i.e. `usePred`) is added to the existing
>> "loadMasked" intrinsic, with the value "true" for the IOOBE part while
>> "false" for the normal load. And the compiler will fail to intrinsify if the
>> flag is "true" and the predicate load is not supported by the backend, which
>> means that normal java path will be executed.
>>
>> Also adds the same vectorization support for masked:
>> - fromByteArray/fromByteBuffer
>> - fromBooleanArray
>> - fromCharArray
>>
>> The performance for the new added benchmarks improve about `1.88x ~ 30.26x`
>> on the x86 AVX-512 system:
>>
>> Benchmark before After Units
>> LoadMaskedIOOBEBenchmark.byteLoadArrayMaskIOOBE 737.542 1387.069 ops/ms
>> LoadMaskedIOOBEBenchmark.doubleLoadArrayMaskIOOBE 118.366 330.776 ops/ms
>> LoadMaskedIOOBEBenchmark.floatLoadArrayMaskIOOBE 233.832 6125.026 ops/ms
>> LoadMaskedIOOBEBenchmark.intLoadArrayMaskIOOBE233.816 7075.923 ops/ms
>> LoadMaskedIOOBEBenchmark.longLoadArrayMaskIOOBE 119.771 330.587 ops/ms
>> LoadMaskedIOOBEBenchmark.shortLoadArrayMaskIOOBE 431.961 939.301 ops/ms
>>
>> Similar performance gain can also be observed on 512-bit SVE system.
>
> Xiaohong Gong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rename "use_predicate" to "needs_predicate"
Yes, the tests were run in debug mode. The reporting of the missing constant
occurs for the compiled method that is called from the method where the
constants are declared e.g.:
719 240bjdk.incubator.vector.Int256Vector::fromArray0 (15
bytes)
** Rejected vector op (LoadVectorMasked,int,8) because architecture does not
support it
** missing constant: offsetInRange=Parm
@ 11
jdk.incubator.vector.IntVector::fromArray0Template (22 bytes) force inline by
annotation
So it appears to be working as expected. A similar pattern occurs at a
lower-level for the passing of the mask class. `Int256Vector::fromArray0`
passes a constant class to `IntVector::fromArray0Template` (the compilation of
which bails out before checking that the `offsetInRange` is constant).
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems
On Thu, 12 May 2022 14:00:44 GMT, Thomas Stuefe wrote:
>> Please review this change to the cgroup v1 subsystem which makes it more
>> resilient on some of the stranger systems. Unfortunately, I wasn't able to
>> re-create a similar system as the reporter. The idea of using the longest
>> substring match for the cgroupv1 file paths was based on the fact that on
>> systemd systems processes run in separate scopes and the maven forked test
>> runner might exhibit this property. For that it makes sense to use the
>> common ancestor path. Nothing changes in the common cases where the
>> `cgroup_path` matches `_root` and when the `_root` is `/` (container the
>> former, host system the latter).
>>
>> In addition, the code paths which are susceptible to throw NPE have been
>> hardened to catch those situations. Should it happen in the future it makes
>> more sense (to me) to not have accurate container detection in favor of
>> continuing to keep running.
>>
>> Finally, with the added unit-tests a bug was uncovered on the "substring"
>> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
>> point to `_root` as it's used as the "needle" to find in "haystack"
>> `cgroup_path` (not the other way round).
>>
>> Testing:
>> - [x] Added unit tests
>> - [x] GHA
>> - [x] Container tests on cgroups v1 Linux. Continue to pass
>
> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 113:
>
>> 111: }
>> 112: buf[MAXPATHLEN-1] = '\0';
>> 113: _path = os::strdup(buf);
>
> I think this code can be simplified a lot with stringStream and without
> strtok, so no need for fixed buffers (which may fail with longer path names)
> and no need for writable string copies on the stack.
>
> Something like this:
>
> stringStream ss;
> ss.print_raw(_mount_point);
> const char* p1 = _root;
> const char* p2 = cgroup_path;
> int last_matching_dash_pos = -1;
> for (int i = 0; *p1 == *p2 && *p1 != 0; i ++) {
> if (*p1 == '/') {
> last_matching_dash_pos = i;
> }
> p1++; p2++;
> }
> ss.print_raw(_root, last_matching_dash_pos);
> // Now use ss.base() to access the assembled string
Nice, thanks! I'll update it.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v2]
On Thu, 12 May 2022 15:50:47 GMT, Raffaello Giulietti wrote: > I think the same change shall apply to the `@throws NullPointerException` > clause. Yeah looks like it. - PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v2]
On Thu, 12 May 2022 15:56:34 GMT, Brian Burkhalter wrote: > > I think the same change shall apply to the `@throws NullPointerException` > > clause. > > Yeah looks like it. Fixed by commit 111ea3e2f4203f05d17431953a5ffaa868176f98. - PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v3]
> Modify the specification of `SequenceInputStream.read(byte[],int,int)` to > indicate that `-1` is returned at the EOF of the last stream even if `len` is > zero. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8286200: Change @throws NPE clause of read(byte[],int,int) to better match specification verbiage - Changes: - all: https://git.openjdk.java.net/jdk/pull/8664/files - new: https://git.openjdk.java.net/jdk/pull/8664/files/7582dbff..111ea3e2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8664=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8664=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8664.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8664/head:pull/8664 PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8283689: Update the foreign linker VM implementation [v16]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Undo spurious changes. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7959/files - new: https://git.openjdk.java.net/jdk/pull/7959/files/aab2d15c..f961121a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7959=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7959=14-15 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Integrated: 8286287: Reading file as UTF-16 causes Error which "shouldn't happen"
On Tue, 10 May 2022 20:22:39 GMT, Naoto Sato wrote: > `String.decodeWithDecoder()` method requires the `CharsetDecoder` parameter > replaces on malformed/unmappable characters with replacements. However, there > was a code path that lacked to set the `CodingErrorAction.REPLACE` on the > decoder. Unrelated, one typo in a test was also fixed. This pull request has now been integrated. Changeset: cc7560e9 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/cc7560e995eac56709d9e55a1561135fad246cb2 Stats: 54 lines in 2 files changed: 52 ins; 2 del; 0 mod 8286287: Reading file as UTF-16 causes Error which "shouldn't happen" Reviewed-by: jpai, bpb, rriggs - PR: https://git.openjdk.java.net/jdk/pull/8640
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v18]
On Thu, 12 May 2022 13:46:11 GMT, xpbob wrote: >> set memory.swappiness to 0,swap space will not be used >> determine the value of memory.swappiness >> https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt >> >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 100.00M >> Maximum Processes Limit: 4194305 >> >> => >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 50.00M >> Maximum Processes Limit: 4194305 > > xpbob has updated the pull request incrementally with one additional commit > since the last revision: > > rename method LGTM. - Marked as reviewed by sgehwolf (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: 8283689: Update the foreign linker VM implementation [v15]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'JEP-19-VM-IMPL2' of https://github.com/JornVernee/jdk into JEP-19-VM-IMPL2 - Fix overwritten copyright years. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7959/files - new: https://git.openjdk.java.net/jdk/pull/7959/files/0f49ff0b..aab2d15c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7959=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7959=13-14 Stats: 36 lines in 33 files changed: 4 ins; 0 del; 32 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v7]
On Wed, 11 May 2022 05:22:22 GMT, Shruthi wrote: >> Removing the Duplicate keys present in XSLTErrorResources.java and >> XPATHErrorResources.java >> >> The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 > > Shruthi has updated the pull request incrementally with one additional commit > since the last revision: > > Modify copyright year Copyright years for XRTreeFragSelectWrapper.java and XSLTErrorResources.java were still not updated, with the later missing the LastModified tag. Please double check all files before integrate. - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v2]
On Thu, 12 May 2022 15:48:47 GMT, Brian Burkhalter wrote: >> Modify the specification of `SequenceInputStream.read(byte[],int,int)` to >> indicate that `-1` is returned at the EOF of the last stream even if `len` >> is zero. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8286200: Change @throws IOOBE clause of read(byte[],int,int) to better > match specification verbiage I think the same change shall apply to the `@throws NullPointerException` clause. - PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 08:29:52 GMT, Guoxiong Li wrote: >>> Remind: please use the command `/jep JEP-424` [1] to mark this PR. >>> >>> [1] >>> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep >> >> Question: I'm willing to try it out. If something goes wrong - would a `jep >> unneeded` be enough to "unstuck" this PR? > >> would a jep unneeded be enough to "unstuck" this PR? > > Yes if no bug. Conceptually, the `/jep unneeded` will behave as no jep > command. @lgxbslgx - JEP has been targeted, but the JEP action seems to be blocking this - what should we do? - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF
On Thu, 12 May 2022 08:41:47 GMT, Raffaello Giulietti wrote: > Also, in the current implementation, when the end of the last contained > stream has been reached and `-1` is returned, none of the arguments is > checked, so a caller can pass `null` for `b` or out of bounds indices `off` > and `len`. This is at odd with the `@throws` clauses. Resolved by commit 7582dbff416e1fb164cfe924c128eb5ee73084f4. - PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8286200: SequenceInputStream::read(b, off, 0) returns -1 at EOF [v2]
> Modify the specification of `SequenceInputStream.read(byte[],int,int)` to > indicate that `-1` is returned at the EOF of the last stream even if `len` is > zero. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8286200: Change @throws IOOBE clause of read(byte[],int,int) to better match specification verbiage - Changes: - all: https://git.openjdk.java.net/jdk/pull/8664/files - new: https://git.openjdk.java.net/jdk/pull/8664/files/b5883b84..7582dbff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8664=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8664=00-01 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8664.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8664/head:pull/8664 PR: https://git.openjdk.java.net/jdk/pull/8664
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 65 commits: - Merge branch 'master' into foreign-preview - Merge branch 'master' into foreign-preview - Fix crashes in heap segment benchmarks due to misaligned access - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Add tests for loaderLookup/restricted method corner cases - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Tweak examples in SymbolLookup javadoc - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - ... and 55 more: https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7888=44 Stats: 65711 lines in 373 files changed: 43720 ins; 19432 del; 2559 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v12]
On Thu, 12 May 2022 11:08:10 GMT, Ludovic Henry wrote:
>> Looks like you are making great progress.
>>
>> Have you thought about ways the intrinsic implementation might be simplified
>> if some code is retained in Java and passed as constant arguments? e.g.
>> table of constants, scalar loop, bounds checks etc, such that the intrinsic
>> primarily focuses on the vectorized code. To some extent that's related to
>> John's point on generalization, and through simplification there may be some
>> generalization.
>>
>> For example if there was a general intrinsic that returned a long value
>> (e.g. first 32 bits are the offset in the array to continue processing, the
>> second 32 bits are the current hashcode value) then we could call that from
>> the Java implementations that then proceed with the scalar loop up to the
>> array length. The Java implementation intrinsic would return `(0L | 1L <<
>> 32)`.
>>
>> Separately it would be nice to consider computing the hash code from the
>> contents of a memory segment, similar to how we added `mismatch` support,
>> but the trick of returning a value that merges the offset and hash code
>> would not work, unless we return the remaining elements to process and that
>> guaranteed to be less than a certain value (or alternatively a relative
>> offset value given an upper bound argument, and performing the intrinsic
>> call in a loop until no progress can be made, which works better for
>> safepoints).
>>
>> The `long[]` hashcode is annoying given `(element ^ (element >>> 32))`, but
>> if we simplify the intrinsic maybe we can add back that complexity?
>
> @PaulSandoz yes, keeping the "short" string part in pure Java and switching
> to an intrinsified/vectorized version for "long" strings is on the next
> avenue of exploration. I would also put the intrinsic as a runtime stub to
> avoid unnecessarily increase the size of the calling method unnecessarily.
> The overhead of the call would be amortised because it would only be called
> for longer strings anyway.
>
> I haven't given much thoughts to how we could split up the different elements
> of the algorithm to generalise the approach just yet. I'll give it a try, see
> how far I can get with it, and keep you updated on my findings.
@luhenry ok, we took a similar approach to the mismatch intrinsic, carefully
analyzing the threshold by which the intrinsic would be called.
My suggestion would be to follow that approach further and head towards an
internal intrinsic perhaps with this signature:
@IntrinsicCandidate
static long hashCode(Class eType, Object base, long offset, int length /* in
bytes * /) {
return 0 | (1L << 32); // or perhaps just return 0
}
```
Then on a further iteration try and pass the polynomial constant and table of
powers (stable array) as arguments.
-
PR: https://git.openjdk.java.net/jdk/pull/7700
Re: RFR: 8283689: Update the foreign linker VM implementation [v10]
On Thu, 12 May 2022 13:55:20 GMT, Nick Gasson wrote: >> That's fine, just needed to check. But as these are now being committed to >> mainline the year does need to change to 2022 - at least in Oracle >> copyright. @nick-arm will have to advise what he thinks should be done with >> the ARM copyright. > > I think the Arm line can be updated to 2022 at the same time. @nick-arm Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v14]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Apply copyright year updates per request of @nick-arm Co-authored-by: Nick Gasson - Changes: - all: https://git.openjdk.java.net/jdk/pull/7959/files - new: https://git.openjdk.java.net/jdk/pull/7959/files/8100e0a7..0f49ff0b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7959=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7959=12-13 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286594: (zipfs) Improvements after JDK-8251329
On Wed, 11 May 2022 15:32:38 GMT, Christoph Langer wrote: > This augments the ZipException upon rejecting a Zip File containing entry > names with "." or ".." elements. > > It furthermore tries to clean up & compact the logic for detecting "." and > ".." and it adds a method IndexNode::nameAsString to return the node name as > a String. A more descriptive title for the bug report would be helpful. - PR: https://git.openjdk.java.net/jdk/pull/8655
Re: RFR: 8283689: Update the foreign linker VM implementation [v12]
On Thu, 12 May 2022 14:53:03 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR updates the VM implementation of the foreign linker, by bringing >> over commits from the panama-foreign repo. >> >> This is split off from the main JEP integration for 19, since we have >> limited resources to handle this. As such, this PR might fall over to 20, >> but it would be nice if we could get it into 19. >> >> I've written up an overview of the Linker architecture here: >> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful >> to read that first. >> >> This patch moves from the "legacy" implementation, to what is currently >> implemented in the panama-foreign repo, except for replacing the use of >> method handle combinators with ASM. That will come in a later path. To >> recap. This PR contains the following changes: >> >> 1. VM stubs for downcalls are now generated up front, instead of lazily by >> C2 [1]. >> 2. the VM support for upcalls/downcalls now support all possible call >> shapes. And VM stubs and Java code implementing the buffered invocation >> strategy has been removed [2], [3], [4], [5]. >> 3. The existing C2 intrinsification support for the `linkToNative` method >> handle linker was no longer needed and has been removed [6] (support might >> be re-added in another form later). >> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now >> implements RuntimeBlob directly. Binding to java classes has been rewritten >> to use javaClasses.h/cpp (this wasn't previously possible due to these java >> classes being in an incubator module) [7], [8], [9]. >> >> While the patch mostly consists of VM changes, there are also some Java >> changes to support (2). >> >> The original commit structure has been mostly retained, so it might be >> useful to look at a specific commit, or the corresponding patch in the >> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) >> repo as well. I've also left some inline comments to explain some of the >> changes, which will hopefully make reviewing easier. >> >> Testing: Tier1-4 >> >> Thanks, >> Jorn >> >> [1]: >> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 >> [2]: >> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 >> [3]: >> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 >> [4]: >> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 >> [5]: >> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 >> [6]: >> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a >> [7]: >> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 >> [8]: >> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f >> [9]: >> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Update Oracle copyright years src/hotspot/cpu/aarch64/foreign_globals_aarch64.cpp line 3: > 1: /* > 2: * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights > reserved. > 3: * Copyright (c) 2019, 2021, Arm Limited. All rights reserved. Suggestion: * Copyright (c) 2019, 2022, Arm Limited. All rights reserved. src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 3: > 1: /* > 2: * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights > reserved. > 3: * Copyright (c) 2019, 2021, Arm Limited. All rights reserved. Suggestion: * Copyright (c) 2019, 2022, Arm Limited. All rights reserved. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v13]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Missed 2 years - Changes: - all: https://git.openjdk.java.net/jdk/pull/7959/files - new: https://git.openjdk.java.net/jdk/pull/7959/files/9a7bb6bb..8100e0a7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7959=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7959=11-12 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v10]
On Thu, 12 May 2022 13:55:20 GMT, Nick Gasson wrote: >> That's fine, just needed to check. But as these are now being committed to >> mainline the year does need to change to 2022 - at least in Oracle >> copyright. @nick-arm will have to advise what he thinks should be done with >> the ARM copyright. > > I think the Arm line can be updated to 2022 at the same time. I've updated all the Oracle copyright years in the files touched by this PR. @nick-arm If you wouldn't mind, could you use the "Add a suggestion" feature (the +/- button when leaving a review comment) to suggest copyright year updates to the relevant files (I think it's just foreign_globals_aarch64.cpp and universalUpcallHandler_aarch64.cpp). That way, when I accept those suggestions, an automatic commit will be made with you as the commit co-author, and it's even clearer that I'm making that change on your behalf. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v12]
> Hi, > > This PR updates the VM implementation of the foreign linker, by bringing over > commits from the panama-foreign repo. > > This is split off from the main JEP integration for 19, since we have limited > resources to handle this. As such, this PR might fall over to 20, but it > would be nice if we could get it into 19. > > I've written up an overview of the Linker architecture here: > http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful > to read that first. > > This patch moves from the "legacy" implementation, to what is currently > implemented in the panama-foreign repo, except for replacing the use of > method handle combinators with ASM. That will come in a later path. To recap. > This PR contains the following changes: > > 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 > [1]. > 2. the VM support for upcalls/downcalls now support all possible call shapes. > And VM stubs and Java code implementing the buffered invocation strategy has > been removed [2], [3], [4], [5]. > 3. The existing C2 intrinsification support for the `linkToNative` method > handle linker was no longer needed and has been removed [6] (support might be > re-added in another form later). > 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now > implements RuntimeBlob directly. Binding to java classes has been rewritten > to use javaClasses.h/cpp (this wasn't previously possible due to these java > classes being in an incubator module) [7], [8], [9]. > > While the patch mostly consists of VM changes, there are also some Java > changes to support (2). > > The original commit structure has been mostly retained, so it might be useful > to look at a specific commit, or the corresponding patch in the > [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) > repo as well. I've also left some inline comments to explain some of the > changes, which will hopefully make reviewing easier. > > Testing: Tier1-4 > > Thanks, > Jorn > > [1]: > https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 > [2]: > https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 > [3]: > https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 > [4]: > https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 > [5]: > https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 > [6]: > https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a > [7]: > https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 > [8]: > https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f > [9]: > https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Update Oracle copyright years - Changes: - all: https://git.openjdk.java.net/jdk/pull/7959/files - new: https://git.openjdk.java.net/jdk/pull/7959/files/1c04a42e..9a7bb6bb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7959=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7959=10-11 Stats: 70 lines in 70 files changed: 0 ins; 0 del; 70 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Thu, 10 Mar 2022 08:52:17 GMT, Сергей Цыпанов wrote:
>> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with
>> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when
>> called with vararg of size 0, 1, 2.
>>
>> In general replacement of `Arrays.asList()` with `List.of()` is dubious as
>> the latter is null-hostile, however in some cases we are sure that arguments
>> are non-null. Into this PR I've included the following cases (in addition to
>> those where the argument is proved to be non-null at compile-time):
>> - `MethodHandles.longestParameterList()` never returns null
>> - parameter types are never null
>> - interfaces used for proxy construction and returned from
>> `Class.getInterfaces()` are never null
>> - exceptions types of method signature are never null
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8282662: Revert dubious changes in MethodType
src/java.base/share/classes/sun/security/validator/EndEntityChecker.java line
119:
> 117: // TLS key exchange algorithms requiring keyEncipherment key usage
> 118: private static final Collection KU_SERVER_ENCRYPTION =
> 119: Arrays.asList("RSA");
I understand that you haven't modified the `KU_SERVER_KEY_AGREEMENT` on line
122 because it has 4 elements. However, these 2 nearby lines using different
styles look a little strange to me. Why restrict the number to 0, 1, and 2? If
we have defined methods with up to 9 arguments, does that mean the new type is
suitable for 9 elements?
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach()
On Wed, 11 May 2022 23:08:32 GMT, Ioi Lam wrote: > The API `jdk.internal.perf.Perf.::attach(String user, int lvmid)` is never > used. It should be removed, and all the handling of a specified user name > should be removed. Marked as reviewed by alanb (Reviewer). src/java.base/share/classes/jdk/internal/perf/Perf.java line 246: > 244: */ > 245: private native ByteBuffer attach0(int lvmid) > 246:throws IllegalArgumentException, IOException; You can drop the "throws IllegalArgumentException" here if you want, it's not needed as it's a runtime exception. - PR: https://git.openjdk.java.net/jdk/pull/8669
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems
On Tue, 10 May 2022 12:29:10 GMT, Severin Gehwolf wrote:
> Please review this change to the cgroup v1 subsystem which makes it more
> resilient on some of the stranger systems. Unfortunately, I wasn't able to
> re-create a similar system as the reporter. The idea of using the longest
> substring match for the cgroupv1 file paths was based on the fact that on
> systemd systems processes run in separate scopes and the maven forked test
> runner might exhibit this property. For that it makes sense to use the common
> ancestor path. Nothing changes in the common cases where the `cgroup_path`
> matches `_root` and when the `_root` is `/` (container the former, host
> system the latter).
>
> In addition, the code paths which are susceptible to throw NPE have been
> hardened to catch those situations. Should it happen in the future it makes
> more sense (to me) to not have accurate container detection in favor of
> continuing to keep running.
>
> Finally, with the added unit-tests a bug was uncovered on the "substring"
> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
> point to `_root` as it's used as the "needle" to find in "haystack"
> `cgroup_path` (not the other way round).
>
> Testing:
> - [x] Added unit tests
> - [x] GHA
> - [x] Container tests on cgroups v1 Linux. Continue to pass
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 113:
> 111: }
> 112: buf[MAXPATHLEN-1] = '\0';
> 113: _path = os::strdup(buf);
I think this code can be simplified a lot with stringStream and without strtok,
so no need for fixed buffers (which may fail with longer path names) and no
need for writable string copies on the stack.
Something like this:
stringStream ss;
ss.print_raw(_mount_point);
const char* p1 = _root;
const char* p2 = cgroup_path;
int last_matching_dash_pos = -1;
for (int i = 0; *p1 == *p2 && *p1 != 0; i ++) {
if (*p1 == '/') {
last_matching_dash_pos = i;
}
p1++; p2++;
}
ss.print_raw(_root, last_matching_dash_pos);
// Now use ss.base() to access the assembled string
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8283689: Update the foreign linker VM implementation [v10]
On Thu, 12 May 2022 13:07:24 GMT, David Holmes wrote: >> I think the Arm Ltd one was probably changed by me in one of the PRs in the >> Panama repo. > > That's fine, just needed to check. But as these are now being committed to > mainline the year does need to change to 2022 - at least in Oracle copyright. > @nick-arm will have to advise what he thinks should be done with the ARM > copyright. I think the Arm line can be updated to 2022 at the same time. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v17]
On Thu, 12 May 2022 13:32:03 GMT, xpbob wrote: >> set memory.swappiness to 0,swap space will not be used >> determine the value of memory.swappiness >> https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt >> >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 100.00M >> Maximum Processes Limit: 4194305 >> >> => >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 50.00M >> Maximum Processes Limit: 4194305 > > xpbob has updated the pull request incrementally with one additional commit > since the last revision: > > add test Thanks for review. @jerboaa The expression is clearer after the change - PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v18]
> set memory.swappiness to 0,swap space will not be used > determine the value of memory.swappiness > https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt > > > Memory Limit: 50.00M > Memory Soft Limit: Unlimited > Memory & Swap Limit: 100.00M > Maximum Processes Limit: 4194305 > > => > > Memory Limit: 50.00M > Memory Soft Limit: Unlimited > Memory & Swap Limit: 50.00M > Maximum Processes Limit: 4194305 xpbob has updated the pull request incrementally with one additional commit since the last revision: rename method - Changes: - all: https://git.openjdk.java.net/jdk/pull/8285/files - new: https://git.openjdk.java.net/jdk/pull/8285/files/93bc46a6..584488f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8285=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8285=16-17 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8285.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8285/head:pull/8285 PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v17]
On Thu, 12 May 2022 13:32:03 GMT, xpbob wrote: >> set memory.swappiness to 0,swap space will not be used >> determine the value of memory.swappiness >> https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt >> >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 100.00M >> Maximum Processes Limit: 4194305 >> >> => >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 50.00M >> Maximum Processes Limit: 4194305 > > xpbob has updated the pull request incrementally with one additional commit > since the last revision: > > add test Thanks for review. @jerboaa Added test in TestMemoryWithCgroupV1 - PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v17]
On Thu, 12 May 2022 13:32:03 GMT, xpbob wrote: >> set memory.swappiness to 0,swap space will not be used >> determine the value of memory.swappiness >> https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt >> >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 100.00M >> Maximum Processes Limit: 4194305 >> >> => >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 50.00M >> Maximum Processes Limit: 4194305 > > xpbob has updated the pull request incrementally with one additional commit > since the last revision: > > add test LGTM. Consider a better name for the test :) test/hotspot/jtreg/containers/docker/TestMemoryWithCgroupV1.java line 90: > 88: } > 89: > 90: private static void testOperatingSystemMXBeanAwareness(String > memoryAllocation, String swapAllocation, Please use a more telling name for this. Perhaps this? `testOSBeanSwappinessMemory`. - Marked as reviewed by sgehwolf (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
> On May 12, 2022, at 8:10 AM, Michael Hall wrote: > > > >> >> My primary suggestion would to be to use an UUID for the unique ID. They are >> of fixed length, are for all intents and purposes unique and you can conjure >> them up from your hat. (An alternative is that the user needs to specify a >> unique ID, but that is probably a less ideal solution.) Presumably, we can >> have some kind of prefix like "org.openjdk.jpackage." before the UUID to >> make them a bit understandable, if someone where to inspect the package >> metadata.e > > I was thinking jpackage would change the XXX to app name but a fixed size > unique field would probably be better. >> >> This seems like a fully workable solution to me. However, I'd really like to >> understand option #1 better, which was: "Package the `java` executable in a >> standard bundle, replacing `runtime/Contents/Home/bin/java` with a symlink >> to that." >> >> I don't know what a "standard bundle" is, or how you would go around to >> package the java executable in one. But on the surface, it sounds much nicer >> than binary editing. >> >> I also don't understand if that means that the standard bundle needs to be >> created at jpackage time, so it gives us the chance to set a proper ID, or >> if the standard bundle can be created at build time, and then the existence >> of this bundle just makes Apple avoid the bundle ID collision checks. Or if >> I'm just misunderstanding it all... >> >> /Magnus > > I may again not understanding but I was thinking they were talking about > something like symlinks to a machine installed JDK and this seemed to me to > defeat some of the purpose of embedding the jdk. But he could be thinking > else. Something external to the application anyhow it seemed. > I thought they meant something like the embedded JDK would be like a framework bundle. Since the framework is expected to be the same in multiple apps it would be excluded from the duplicate id check. (I think that is related to the older bug that the Apple guy thought might have come back.) Scott
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v17]
> set memory.swappiness to 0,swap space will not be used > determine the value of memory.swappiness > https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt > > > Memory Limit: 50.00M > Memory Soft Limit: Unlimited > Memory & Swap Limit: 100.00M > Maximum Processes Limit: 4194305 > > => > > Memory Limit: 50.00M > Memory Soft Limit: Unlimited > Memory & Swap Limit: 50.00M > Maximum Processes Limit: 4194305 xpbob has updated the pull request incrementally with one additional commit since the last revision: add test - Changes: - all: https://git.openjdk.java.net/jdk/pull/8285/files - new: https://git.openjdk.java.net/jdk/pull/8285/files/296a409e..93bc46a6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8285=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8285=15-16 Stats: 36 lines in 1 file changed: 29 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/8285.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8285/head:pull/8285 PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
On 2022-05-12 04:58, Magnus Ihse Bursie wrote: On 2022-05-12 13:17, Michael Hall wrote: A solution like including a bundle identifier something like net.java.openjdk.MYAPP.java would be possible at packaging time but not at build time. To fix this at build time you would need to generate a name unique to each installed jdk. Including release net.java.openjdk.JDK_RELEASE.java might avoid some conflicts but would still be open to conflict for two applications at the same release. So it can’t be addressed ‘before the fact’ either. The only thing I am currently thinking of that might work would be include a replaceable part in the identifier. So something like net.java.openjdk.java.XX Where jpackage could include something to change the X…. to a unique application name. If you don’t change the string size you could probably avoid some of the resizing issues Apple DTS mentions. Whether there is a standard Apple tool to do this I don’t know. As you say, we're a bit in a bind since the java executable needs to be created when the JDK is built, but the bundle ID needs to be determined when jpackage is run (and a suitable, per-application ID can be created), and there is no standard tooling to update the bundle ID after build time. I believe what you are describing is exactly solution #2 suggested by the Apple engineer. I don't think that would be horribly difficult to achieve, though. Sure, it's a bit of a hack, but not the ugliest I've seen in my days. If we go down this route, I suppose we do something like this: 1) When building the JDK, we create an additional copy of the java executable, and store it with the jdk.jpackage resources. Let's call it java.template, for the sake of it. This is identical to the real bin/java except for the fact that it contains a different bundle ID, with a large enough padding field, like X... This way, we don't have to mess around with the real java executable for the JDK. Aren't we embedding this bundle ID in every launcher, so you would need a .template for each possible launcher that could be included in an app? What I think we need to do is first evaluate if we actually need to embed this bundle ID in the executables at all, or rather, what would the consequences be if we didn't. My understanding is that we only do this to be able to run them outside of a bundle directory structure, but this would need some pretty thorough testing of course. If we are to generate a special set of launchers for jpackage, maybe all we need to do is not embed any bundle ID in them, as they are meant to only be used within a jpackaged app, so they should be covered by Info.plist files anyway. /Erik
Re: RFR: 8283689: Update the foreign linker VM implementation [v10]
On Thu, 12 May 2022 08:23:11 GMT, Nick Gasson wrote: >> src/hotspot/cpu/aarch64/foreign_globals_aarch64.cpp line 3: >> >>> 1: /* >>> 2: * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights >>> reserved. >>> 3: * Copyright (c) 2019, 2021, Arm Limited. All rights reserved. >> >> Only update third-party copyrights under direction from that copyright >> holder. This may not be the correct format for example. >> Also it is 2022. :) > > I think the Arm Ltd one was probably changed by me in one of the PRs in the > Panama repo. That's fine, just needed to check. But as these are now being committed to mainline the year does need to change to 2022 - at least in Oracle copyright. @nick-arm will have to advise what he thinks should be done with the ARM copyright. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8286623: Bundle zlib by default with JDK on macos aarch64
On Thu, 12 May 2022 08:43:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change to the build system which now makes > bundled zlib as the default for macos aarch64 systems? > > We have been running into various intermittent failures on macos aarch64 > systems for several months now. After investigation we have been able to > ascertain that the root cause of the issue lies within the zlib that's > shipped on macos aarch64 systems. The complete details about that issue is > available at https://bugs.openjdk.java.net/browse/JDK-8282954. > We have filed a bug with Apple for their inputs on what we can do to fix it > in that shipped library. Given the nature of that issue, we don't have a > timeline on when/if the solution for that will be available. Until that time, > at least, the proposal is to use bundled zlib (which doesn't reproduce those > failures) by default on macos aarch64. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8675
Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec
On Wed, 11 May 2022 20:40:30 GMT, Joe Darcy wrote:
> While doing a CSR review of another issue, I noticed some cases in
> InputStream and OutputStream what would benefit from being upgraded to
> implSpec and related javadoc tags.
>
> The "A subclass must provide an implementation of this method." statements on
> several abstract methods don't add much value, but I chose to leave them in
> for this request.
>
> Please also review the corresponding CSR:
> https://bugs.openjdk.java.net/browse/JDK-8286605
src/java.base/share/classes/java/io/InputStream.java line 177:
> 175: *
> 176: * @apiNote
> 177: * A subclass must provide an implementation of this method.
Is this sentence useful to keep? The method is abstract so a concrete
implementation has to implement it. On the other other hand, an abstract
subclass does not need to implement it.
src/java.base/share/classes/java/io/InputStream.java line 688:
> 686: * @implSpec
> 687: * The {@code mark} method of {@code InputStream} does
> 688: * nothing.
Minor nit but the line break can be removed so that "nothing" is on the same
line.
-
PR: https://git.openjdk.java.net/jdk/pull/8663
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v14]
> Despite the hash value being cached for Strings, computing the hash still > represents a significant CPU usage for applications handling lots of text. > > Even though it would be generally better to do it through an enhancement to > the autovectorizer, the complexity of doing it by hand is trivial and the > gain is sizable (2x speedup) even without the Vector API. The algorithm has > been proposed by Richard Startin and Paul Sandoz [1]. > > Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` > > > Benchmark(size) Mode Cnt Score > Error Units > StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 > ± 0.210 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 > ± 0.127 ns/op > StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 > ± 0.099 ns/op > StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 > ± 0.444 ns/op > StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 > ± 0.846 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 > ± 4.071 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 > ± 0.092 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 > ± 0.159 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 > ± 1.218 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 > ± 1.225 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 > ± 14.621 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 > ± 0.097 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 > ± 0.158 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 > ± 0.065 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 > ± 0.251 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 > ± 1.667 ns/op > StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 > ± 0.191 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 > ± 0.362 ns/op > StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 > ± 0.112 ns/op > StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 > ± 0.702 ns/op > StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 > ± 3.290 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 > ± 43.847 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 > ± 0.038 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 > ± 0.422 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 > ± 0.178 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 > ± 0.089 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 > ± 1.834 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 > ± 2.927 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 > ± 0.396 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 > ± 0.189 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 > ± 0.340 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 > ± 2.699 ns/op > > > At Datadog, we handle a great amount of text (through logs management for > example), and hashing String represents a large part of our CPU usage. It's > very unlikely that we are the only one as String.hashCode is such a core > feature of the JVM-based languages with its use in HashMap for example. > Having even only a 2x speedup would allow us to save thousands of CPU cores > per month and improve correspondingly the energy/carbon impact. > > [1] > https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf Ludovic Henry has updated the pull request incrementally with one additional commit since the last revision: Reenable SpecialArraysHashCode by default - Changes: - all: https://git.openjdk.java.net/jdk/pull/7700/files - new:
Re: RFR: 8283689: Update the foreign linker VM implementation [v7]
On Thu, 12 May 2022 09:24:23 GMT, Jorn Vernee wrote: > Do nothing special for async exceptions. i.e. if they happen anywhere between > 1. and 6. they will end up in one of the fallback handlers and the VM will be > terminated. My understanding is that if they materialize while we're executing the upcall Java code, if that code has a try/catch block, we will go there, rather than crash the VM. In other words, IMHO the only problem with async exception is if they occur _after_ the Java user code has completed, because that will crash the Java adapter, this preventing it from returning to native call cleanly. So, either we disable async exceptions during that phase (e.g. after user code has executed, but before we return back to native code), or we just punt and stop. Since this seems like a corner^3 case, and since there are also other issue with upcalls that can occur if other threads do not cooperate (e.g. an upcall can get stuck into an infinite safepoint if the VM exits while an async native thread runs the upcall), and given that obtaining a linker is a restricted operation anyway, I don't think we should bend over backwards to try to add 1% more safety to something that's unavoidably sharp anyways. - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v16]
On Thu, 12 May 2022 09:50:39 GMT, xpbob wrote: >> set memory.swappiness to 0,swap space will not be used >> determine the value of memory.swappiness >> https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt >> >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 100.00M >> Maximum Processes Limit: 4194305 >> >> => >> >> Memory Limit: 50.00M >> Memory Soft Limit: Unlimited >> Memory & Swap Limit: 50.00M >> Maximum Processes Limit: 4194305 > > xpbob has updated the pull request incrementally with one additional commit > since the last revision: > > add swap method This looks good. I think the only thing missing is a test for the JDK side. Perhaps write one using the `OperatingSystemMXBean`'s `getTotalSwapSpaceSize()` method within a container with `--memory=200m --memory-swap=250m` and using `--memory-swappiness=0`. You could use `test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java` and `CheckOperatingSystemMXBean.java` as a model. Again a cgroupsv1 specific test. - PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
> > My primary suggestion would to be to use an UUID for the unique ID. They are > of fixed length, are for all intents and purposes unique and you can conjure > them up from your hat. (An alternative is that the user needs to specify a > unique ID, but that is probably a less ideal solution.) Presumably, we can > have some kind of prefix like "org.openjdk.jpackage." before the UUID to make > them a bit understandable, if someone where to inspect the package metadata.e I was thinking jpackage would change the XXX to app name but a fixed size unique field would probably be better. > > This seems like a fully workable solution to me. However, I'd really like to > understand option #1 better, which was: "Package the `java` executable in a > standard bundle, replacing `runtime/Contents/Home/bin/java` with a symlink to > that." > > I don't know what a "standard bundle" is, or how you would go around to > package the java executable in one. But on the surface, it sounds much nicer > than binary editing. > > I also don't understand if that means that the standard bundle needs to be > created at jpackage time, so it gives us the chance to set a proper ID, or if > the standard bundle can be created at build time, and then the existence of > this bundle just makes Apple avoid the bundle ID collision checks. Or if I'm > just misunderstanding it all... > > /Magnus I may again not understanding but I was thinking they were talking about something like symlinks to a machine installed JDK and this seemed to me to defeat some of the purpose of embedding the jdk. But he could be thinking else. Something external to the application anyhow it seemed.
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
On 2022-05-12 13:17, Michael Hall wrote: I had read this but possibly didn’t grok the issue myself. If I understand correctly now the conflict isn’t within the application but across applications to some other application that has already been added to the Mac App Store that included the commands with the given CFBundleIdentifier. Yes, that is indeed how I also interpret this. Presumably, the very first developer to submit a jpackaged application to App Store (from what I can tell now OpenJDK itself is not present there, which I mistakenly believed before) got everything working without troubles, but blocked all other developers from submitting their apps. A solution like including a bundle identifier something like net.java.openjdk.MYAPP.java would be possible at packaging time but not at build time. To fix this at build time you would need to generate a name unique to each installed jdk. Including release net.java.openjdk.JDK_RELEASE.java might avoid some conflicts but would still be open to conflict for two applications at the same release. So it can’t be addressed ‘before the fact’ either. The only thing I am currently thinking of that might work would be include a replaceable part in the identifier. So something like net.java.openjdk.java.XX Where jpackage could include something to change the X…. to a unique application name. If you don’t change the string size you could probably avoid some of the resizing issues Apple DTS mentions. Whether there is a standard Apple tool to do this I don’t know. As you say, we're a bit in a bind since the java executable needs to be created when the JDK is built, but the bundle ID needs to be determined when jpackage is run (and a suitable, per-application ID can be created), and there is no standard tooling to update the bundle ID after build time. I believe what you are describing is exactly solution #2 suggested by the Apple engineer. I don't think that would be horribly difficult to achieve, though. Sure, it's a bit of a hack, but not the ugliest I've seen in my days. If we go down this route, I suppose we do something like this: 1) When building the JDK, we create an additional copy of the java executable, and store it with the jdk.jpackage resources. Let's call it java.template, for the sake of it. This is identical to the real bin/java except for the fact that it contains a different bundle ID, with a large enough padding field, like X... This way, we don't have to mess around with the real java executable for the JDK. 2) At jpackage time, this java.template file is installed instead of bin/java, and the padding field is replaced by a unique value. The java executable is small (33kB on macOS, currently) so a simple search through the binary field for the pattern is likely to work alright. As long as there are no checksums being broken, this should be straightforward. My primary suggestion would to be to use an UUID for the unique ID. They are of fixed length, are for all intents and purposes unique and you can conjure them up from your hat. (An alternative is that the user needs to specify a unique ID, but that is probably a less ideal solution.) Presumably, we can have some kind of prefix like "org.openjdk.jpackage." before the UUID to make them a bit understandable, if someone where to inspect the package metadata. This seems like a fully workable solution to me. However, I'd really like to understand option #1 better, which was: "Package the `java` executable in a standard bundle, replacing `runtime/Contents/Home/bin/java` with a symlink to that." I don't know what a "standard bundle" is, or how you would go around to package the java executable in one. But on the surface, it sounds much nicer than binary editing. I also don't understand if that means that the standard bundle needs to be created at jpackage time, so it gives us the chance to set a proper ID, or if the standard bundle can be created at build time, and then the existence of this bundle just makes Apple avoid the bundle ID collision checks. Or if I'm just misunderstanding it all... /Magnus
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v13]
> Despite the hash value being cached for Strings, computing the hash still > represents a significant CPU usage for applications handling lots of text. > > Even though it would be generally better to do it through an enhancement to > the autovectorizer, the complexity of doing it by hand is trivial and the > gain is sizable (2x speedup) even without the Vector API. The algorithm has > been proposed by Richard Startin and Paul Sandoz [1]. > > Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` > > > Benchmark(size) Mode Cnt Score > Error Units > StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 > ± 0.210 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 > ± 0.127 ns/op > StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 > ± 0.099 ns/op > StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 > ± 0.444 ns/op > StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 > ± 0.846 ns/op > StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 > ± 4.071 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 > ± 0.092 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 > ± 0.159 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 > ± 1.218 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 > ± 1.225 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 > ± 14.621 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 > ± 0.097 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 > ± 0.158 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 > ± 0.065 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 > ± 0.019 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 > ± 0.251 ns/op > StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 > ± 1.667 ns/op > StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 > ± 0.191 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 > ± 0.362 ns/op > StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 > ± 0.112 ns/op > StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 > ± 0.702 ns/op > StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 > ± 3.290 ns/op > StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 > ± 43.847 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 > ± 0.038 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 > ± 0.422 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 > ± 0.178 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 > ± 0.089 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 > ± 1.834 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 > ± 2.927 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 > ± 0.396 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 > ± 0.189 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 > ± 0.032 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 > ± 0.340 ns/op > StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 > ± 2.699 ns/op > > > At Datadog, we handle a great amount of text (through logs management for > example), and hashing String represents a large part of our CPU usage. It's > very unlikely that we are the only one as String.hashCode is such a core > feature of the JVM-based languages with its use in HashMap for example. > Having even only a 2x speedup would allow us to save thousands of CPU cores > per month and improve correspondingly the energy/carbon impact. > > [1] > https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf Ludovic Henry has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits: - Ensure a proper register is used + Slight performance optimizations - Merge branch 'master' of
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems
On Thu, 12 May 2022 06:13:47 GMT, Ioi Lam wrote: > I just started to look at the code so just one comment for now. Sure, thanks for the review! - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
> On May 12, 2022, at 4:42 AM, Magnus Ihse Bursie > wrote: > >> Some further spelunking reveals the issue. Consider this: >> >> % otool -s __TEXT __info_plist -v >> APP_NAME.app/Contents/runtime/Contents/Home/bin/java >> … >> >> CFBundleIdentifier >> net.java.openjdk.java >> … >> >> >> >> This is an obscure corner case in the macOS bundle story: A standalone tool, >> like `java`, which isn’t in a bundle structure, and thus can’t have a >> standalone `Info.plist` file, can put its information property list in a >> `__TEXT` / `__info_plist` section within its executable. And it seems that >> the folks who created your Java runtime did just that. >> >> Given that, the Mac App Store error is valid: You are trying to submit an >> executable whose bundle ID matches some existing app. >> >> To get around this check you’ll need to give `java` a new bundle ID. That’s >> not easy to do. This `__TEXT` / `__info_plist` section is set up by a linker >> option (see `-sectcreate` in >> and there’s no good way to modify it after the fact [1]. I had read this but possibly didn’t grok the issue myself. If I understand correctly now the conflict isn’t within the application but across applications to some other application that has already been added to the Mac App Store that included the commands with the given CFBundleIdentifier. A solution like including a bundle identifier something like net.java.openjdk.MYAPP.java would be possible at packaging time but not at build time. To fix this at build time you would need to generate a name unique to each installed jdk. Including release net.java.openjdk.JDK_RELEASE.java might avoid some conflicts but would still be open to conflict for two applications at the same release. So it can’t be addressed ‘before the fact’ either. The only thing I am currently thinking of that might work would be include a replaceable part in the identifier. So something like net.java.openjdk.java.XX Where jpackage could include something to change the X…. to a unique application name. If you don’t change the string size you could probably avoid some of the resizing issues Apple DTS mentions. Whether there is a standard Apple tool to do this I don’t know.
Re: RFR: 8282664: Unroll by hand StringUTF16, StringLatin1, and Arrays polynomial hash loops [v12]
On Wed, 11 May 2022 19:13:55 GMT, Paul Sandoz wrote:
>> Ludovic Henry has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 18 commits:
>>
>> - Fix overlapping registers
>> - Actually fix h when hashcode is vectorized
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> vectorized-stringlatin1-hashcode
>> - Fix h when vectorized for Arrays.hashCode
>> - Add missing check for AryHashCode node
>> - Fix some merge conflicts
>> - Disable Arrays.hashCode intrinsic by default for CI
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> vectorized-stringlatin1-hashcode
>> - Some small refactoring: store power_of_31_backwards in the code directly,
>> compact code, and more
>> - {wip} Generalize string hashcode to Arrays.hashCode
>> - ... and 8 more:
>> https://git.openjdk.java.net/jdk/compare/3fa1c404...29dab16b
>
> Looks like you are making great progress.
>
> Have you thought about ways the intrinsic implementation might be simplified
> if some code is retained in Java and passed as constant arguments? e.g. table
> of constants, scalar loop, bounds checks etc, such that the intrinsic
> primarily focuses on the vectorized code. To some extent that's related to
> John's point on generalization, and through simplification there may be some
> generalization.
>
> For example if there was a general intrinsic that returned a long value (e.g.
> first 32 bits are the offset in the array to continue processing, the second
> 32 bits are the current hashcode value) then we could call that from the Java
> implementations that then proceed with the scalar loop up to the array
> length. The Java implementation intrinsic would return `(0L | 1L << 32)`.
>
> Separately it would be nice to consider computing the hash code from the
> contents of a memory segment, similar to how we added `mismatch` support, but
> the trick of returning a value that merges the offset and hash code would not
> work, unless we return the remaining elements to process and that guaranteed
> to be less than a certain value (or alternatively a relative offset value
> given an upper bound argument, and performing the intrinsic call in a loop
> until no progress can be made, which works better for safepoints).
>
> The `long[]` hashcode is annoying given `(element ^ (element >>> 32))`, but
> if we simplify the intrinsic maybe we can add back that complexity?
@PaulSandoz yes, keeping the "short" string part in pure Java and switching to
an intrinsified/vectorized version for "long" strings is on the next avenue of
exploration. I would also put the intrinsic as a runtime stub to avoid
unnecessarily increase the size of the calling method unnecessarily. The
overhead of the call would be amortised because it would only be called for
longer strings anyway.
I haven't given much thoughts to how we could split up the different elements
of the algorithm to generalise the approach just yet. I'll give it a try, see
how far I can get with it, and keep you updated on my findings.
-
PR: https://git.openjdk.java.net/jdk/pull/7700
