Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v14]
On Thu, 23 Feb 2023 07:21:08 GMT, Eirik Bjorsnos wrote: > Seems compatibility with existing 6-bit devices might have been the primary > concern: This also explains the placement of brackets, braces, bars, tilde etc. They would look visually similar on 6-bit devices: https://user-images.githubusercontent.com/300291/220845393-de12301a-73e2-46fe-a6ce-ce101d0a2b3b.png;> - PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v14]
On Wed, 22 Feb 2023 20:01:52 GMT, Eirik Bjorsnos wrote: >> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying >> 'the oldest ASCII trick in the book'. >> >> The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two >> latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is >> updated to use `equalsIgnoreCase` >> >> To verify the correctness of `equalsIgnoreCase`, a new test is added to >> `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1 >> code point pairs have an `equalsIgnoreCase` consistent with >> Character.toUpperCase, Character.toLowerCase. >> >> Performance is tested for matching and mismatching cases of code point pairs >> picked from the ASCII letter, ASCII number and latin1 letter ranges. Results >> in the first comment below. > > Eirik Bjorsnos has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 21 additional > commits since the last revision: > > - Merge branch 'master' into regionmatches-latin1-speedup > - Merge branch 'master' into regionmatches-latin1-speedup > - Make the loop variables chars to avoid casting > - Use improved case-twiddling comment as suggested by Martin > - Replace 'oldest ASCII trick in the book' use in toUpperCase, toLowerCase > with "by removing (setting) a single bit" > - Align local variable naming in toLowerCase, toUpperCase with > equalsIgnoreCase by using 'lower' and 'upper' > - Rename unconventionally named local variable 'U' to 'upper' > - Merge remote-tracking branch 'origin/master' into > regionmatches-latin1-speedup > - Add whitespace between methods > - Merge branch 'master' into regionmatches-latin1-speedup > - ... and 11 more: https://git.openjdk.org/jdk/compare/31689be3...597b346a I found this in Appendix A of the 1973 `Draft Proposed Revision of ASCII`. Seems compatibility with existing 6-bit devices might have been the primary concern: A 6.4 It is expected that devices having the capability of printing only 64 graphic symbols will continue to be important. It may be desirable to arrange these devices to print one symbol for the bit pattern of both upper and lower case of a given alphabetic letter. To facilitate this, there should be a single- bit difference between the upper and lowercase representations of any given letter. Combined with the requirement that a given case of the alphabet be contiguous, this dictated the assignment of the alphabet, as shown in columns 4 through 7. https://user-images.githubusercontent.com/300291/220842000-3efa64fe-9154-4069-9e81-e202d5731f6f.png;> https://ia800606.us.archive.org/17/items/enf-ascii-1972-1975/Image070917152640_text.pdf - PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Thu, 23 Feb 2023 06:18:49 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separate RFE).
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove size restriction for structs. Add TODO for Big Endian.
I should add tests for the tricky corner cases like the following ones:
EXPORT struct S_FF f_S_S_FF(float p0, float p1, float p2, float p3, float p4,
float p5, float p6, float p7, float p8, float p9, float p10, float p11, struct
S_FF p12, float p13) { return p12; }
EXPORT floatf_F_S_FF(float p0, float p1, float p2, float p3, float p4,
float p5, float p6, float p7, float p8, float p9, float p10, float p11, struct
S_FF p12, float p13) { return p13; }
EXPORT struct S_FF f_S_SSS_FF(struct S_FF p0, struct S_FF p1, struct S_FF
p2, struct S_FF p3, struct S_FF p4, struct S_FF p5, struct S_FF p6, float p7) {
return p6; }
EXPORT floatf_F_SSS_FF(struct S_FF p0, struct S_FF p1, struct S_FF
p2, struct S_FF p3, struct S_FF p4, struct S_FF p5, struct S_FF p6, float p7) {
return p7; }
Can I add them to the existing libraries? If so, what is the correct naming
scheme and what is needed to get them executed (adding the EXPORT alone is not
sufficient). Or should I create a separate test for these cases?
Advice will be appreciated!
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8302806: (fs) Remove com.sun.nio.file.SensitivityWatchEventModifier [v3]
On Wed, 22 Feb 2023 23:35:08 GMT, Brian Burkhalter wrote: >> This enum is not used in the JDK and did not appear in external source code >> searches. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8302806: Remove sensitivity from PollingWatchService You need remove the SENSITIVITY_xxx constants from sun.nio.fs.ExtendedOptions and that will should help identify the code in the polling WatchService implementation that need to be removed. - PR: https://git.openjdk.org/jdk/pull/12626
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Thu, 23 Feb 2023 06:18:49 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separate RFE).
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove size restriction for structs. Add TODO for Big Endian.
I have removed the size restriction for structs. Passing a struct consisting of
1 char works (on Little Endian). However, passing a struct consisting of 3
chars doesn't (getting IndexOutOfBoundsException: Out of bound access on
segment MemorySegment). Neither on PPC64, nor on x86. Is that known or should I
file a bug for that?
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Remove size restriction for structs. Add TODO for Big Endian.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/7315fd20..a4d844f7
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12708=02
- incr: https://webrevs.openjdk.org/?repo=jdk=12708=01-02
Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview)
On Wed, 22 Feb 2023 17:03:16 GMT, Jorn Vernee wrote: > I will do a more thorough review soon. Thanks a lot! > > The ABI has some tricky corner cases related to HFA (Homogeneous Float > > Aggregate). The same argument may need to get passed in both, a FP reg and > > a GP reg or stack slot (see "no partial DW rule"). This cases are not > > covered by the existing tests. > > FWIW, we have to do this for Windows vararg floats as well > ([here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java#L231-L239)) > > This can be done by `dup`-ing the value, and using 2 `vmStore`s. (each > `vmStore` corresponding to a single register/stack location). Doing something > similar might be simpler than the `INTEGER_AND_FLOAT` and `STACK_AND_FLOAT` > storage types you're using right now. I'm not sure if that is related to the > other limitations you mention? Might be interesting to look into. (perhaps as > a separate RFE. I don't have a big issue since the current approach stays in > PPC-only code) Maybe I need to think a bit more about it. I don't really like the extra cases for `INTEGER_AND_FLOAT` and `STACK_AND_FLOAT`. On the other side, doing it in the CallArranger would break the design of factoring out the allocation from the binding generation. In addition, it seems like PPC64 is even more tricky than the Windows case. I need to pass 2 float arguments in a GP reg (or stack slot) plus one of these 2 floats in float register F13. I think this can get implemented more easily in the backend. Do you agree? - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v2]
On Thu, 23 Feb 2023 04:37:49 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separate RFE).
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Clean fix for NativeMemorySegmentImpl issue with byteSize 0.
> > (I'd be happy to implement the needed changes in shared code if you want,
> > since it touches `BindingSpecializer` which is pretty dense)
>
> FYI:
> [master...JornVernee:jdk:I2L](https://github.com/openjdk/jdk/compare/master...JornVernee:jdk:I2L)
> (assuming `I2L` has the same semantics as `extsw`). Then just add a
> `.cast(int.class, long.class)` wherever currently an `int` is `vmStore`d in
> the PPC CallArranger.
Correct, `extsw` performs a `I2L` conversion. I had thought about this already,
but I think my current implementation is more efficient as it combines register
moves with the 64 bit extend. Your proposal would generate separate extend and
move instructions, right?
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v2]
On Wed, 22 Feb 2023 18:23:54 GMT, Jorn Vernee wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Clean fix for NativeMemorySegmentImpl issue with byteSize 0.
>
> src/java.base/share/classes/jdk/internal/foreign/PlatformLayouts.java line
> 266:
>
>> 264: * The {@code T*} native type.
>> 265: */
>> 266: public static final ValueLayout.OfAddress C_POINTER =
>> ValueLayout.ADDRESS.withBitAlignment(64);
>
> I think this is where the issue with the check in `MemorySegment::copy` comes
> from. Note how other platforms add a call to `asUnbounded` for the created
> layout, which makes any pointer boxed using this layout writable/readable
> (such as the in memory return pointer for upcalls).
Thanks for the hint! You found it pretty quickly. I had missed that when
rebasing my early prototype. Fixed.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v2]
On Wed, 22 Feb 2023 18:31:45 GMT, Maurizio Cimadamore
wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Clean fix for NativeMemorySegmentImpl issue with byteSize 0.
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1359:
>
>> 1357: long size = elementCount * srcElementLayout.byteSize();
>> 1358: srcImpl.checkAccess(srcOffset, size, true);
>> 1359: if (dstImpl instanceof NativeMemorySegmentImpl &&
>> dstImpl.byteSize() == 0) {
>
> As Jorn said, this change is not acceptable, as it allows bulk copy
> disregarding the segment real size. In such cases, the issue is always a
> missing unsafe resize somewhere in the linker code.
Removed this workaround. I'm glad to get rid of it.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v2]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Clean fix for NativeMemorySegmentImpl issue with byteSize 0.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/4a5debfc..7315fd20
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12708=01
- incr: https://webrevs.openjdk.org/?repo=jdk=12708=00-01
Stats: 6 lines in 2 files changed: 0 ins; 4 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8302204: Optimize BigDecimal.divide
On Tue, 14 Feb 2023 03:20:14 GMT, Sergey Kuksenko wrote:
>> [JDK-8269667](https://bugs.openjdk.org/browse/JDK-8269667) has uncovered the
>> poor performance of BigDecimal.divide under certain circumstance.
>>
>> We confront similar situations when benchmarking Spark3 on TPC-DS test kit.
>> According to the flame-graph below, it is StripZeros that spends most of the
>> time of BigDecimal.divide. Hence we propose this patch to optimize stripping
>> zeros.
>> 
>>
>> Currently, createAndStripZerosToMatchScale() is performed linearly. That is,
>> the target value is parsed from back to front, each time stripping out
>> single ‘0’. To optimize, we can adopt the method of binary search. That is,
>> each time we try to strip out ${scale/2} ‘0’s.
>>
>> The performance looks good. Therotically, time complexity of our method is
>> O(log n), while the current one is O(n). In practice, benchmarks on Spark3
>> show that 1/3 less time (102s->68s) is spent on TPC-DS query4. We also runs
>> Jtreg and JCK to check correctness, and it seems fine.
>>
>> More about environment:
>> we run Spark3.3.0 on Openjdk11, but it seems jdk version doesn’t have much
>> impact on BigDecimal. Spark cluster consists of a main node and 2 core
>> nodes, each has 4cores, 16g memory and 4x500GB storage.
>
> The pr looks promising in terms of performance.
> What makes sense to do:
>
> *) Don't rely on external benchmarks. It's fine if such exists, but anyway
> set of microbenchmarks (using JMH) will be much better. More clear, readable
> results, etc. E.g., it may show that other operations (for example, sqrt)
> were speeded up too.
>
> *) Find boundaries.
> "divideAndRemainder(bigTenToThe(scaleStep))" may produce non-zero reminder.
> Find conditions when it happens. How big is performance regression in such
> cases?
>
> Some other optimizations:
> *)
> Current code checks only the lowest bit (odd or even) to cut off indivisible
> cases.
> Making "divideAndRemainder(bigTenToThe(scaleStep))" - you make check
> scaleStep lowest bits to do cut off. See "BigInteger.getLowestSetBit()"
>
> *)
> BigInteger division by int value is faster. It's a special case. What is
> faster, doing the single division by bigTenToThe(scaleStep) or doing several
> divisions (split scaleStep to fit into int)? Exploration is required.
@kuksenko Hi, I have updated the performance of this pr, I wonder if that's ok
-
PR: https://git.openjdk.org/jdk/pull/12509
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 09:46:59 GMT, Doug Simon wrote: > > That said, I think it is reasonable on a given JVM invocation if > > Float.floatToFloat16(f) gave the same result for input f regardless of in > > what context it was called. > > Yes, I'm under the impression that for math API methods like this, the > stability of input to output must be preserved for a single JVM invocation. > Or are there existing methods for which the interpreter and compiled code > execution is allowed to differ? A similar but not exactly analagous situation occurs for the intrinsics of various java.lang.Math methods. Many of the methods of interest allow implementation flexibility, subject to various quality of implementation criteria. Those criteria bound the maximum error at a single point, phrased in terms of ulps -- units in the last place, and require semi-monotonicity -- which describes the relation of outputs of adjacent floating-point values. Taking two compliant implementations of Math.foo(), it is not necessarily a valid implementation to switch between them because the semi-monotonicity constraint can be violated. The solution is to always use or always not use an intrinsic for Math.foo on a given JVM invocation. HTH - PR: https://git.openjdk.org/jdk/pull/12704
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 21:21:42 GMT, Vladimir Kozlov wrote:
>>> I'm also a bit concerned that we are rushing in to "fix" this. IIUC we have
>>> three mechanisms for implementing this functionality:
>>>
>>> 1. The interpreted Java code
>>>
>>> 2. The compiled non-intrinisc sharedRuntime code
>>>
>>> 3. The compiler intrinsic that uses a hardware instruction.
>>>
>>>
>>> Unless the hardware instructions for all relevant CPUs behave exactly the
>>> same, then I don't see how we can have parity of behaviour across these
>>> three mechanisms.
>>>
>>> The observed behaviour may be surprising but it seems not to be a bug. And
>>> is this even a real concern - would real programs actually need to peek at
>>> the raw bits and so see the difference, or does it suffice to handle Nan's
>>> opaquely?
>>
>> From the spec
>> (https://download.java.net/java/early_access/jdk20/docs/api/java.base/java/lang/Float.html#float16ToFloat(short))
>>
>> "Returns the float value closest to the numerical value of the argument, a
>> floating-point binary16 value encoded in a short. The conversion is exact;
>> all binary16 values can be exactly represented in float. Special cases:
>>
>> If the argument is zero, the result is a zero with the same sign as the
>> argument.
>> If the argument is infinite, the result is an infinity with the same
>> sign as the argument.
>> If the argument is a NaN, the result is a NaN. "
>>
>> If the float argument is a NaN, you are supposed to get a float16 NaN as a
>> result -- that is all the specification requires. However, the
>> implementation makes stronger guarantees to try to preserve some non-zero
>> NaN significand bits if they are set.
>>
>> "NaN boxing" is a technique used to put extra information into the
>> significand bits a NaN and pass the around. It is consistent with the
>> intended use of the feature by IEEE 754 and used in various language
>> runtimes: e.g.,
>>
>> https://piotrduperas.com/posts/nan-boxing
>> https://leonardschuetz.ch/blog/nan-boxing/
>> https://anniecherkaev.com/the-secret-life-of-nan
>>
>> The Java specs are careful to avoid mentioning quiet vs signaling NaNs in
>> general discussion.
>>
>> That said, I think it is reasonable on a given JVM invocation if
>> Float.floatToFloat16(f) gave the same result for input f regardless of in
>> what context it was called.
>
>> We don't know that all HW will produce the same NaN "payload", right?
>> Instead, we might need interpreter intrinsics. I assume that is how the trig
>> functions are handled that @jddarcy mentioned.
>
> Good point. We can't guarantee that all OpenJDK ports HW do the same.
>
> If CPU has corresponding instructions we need to generate a stub during VM
> startup with HW instructions and use it in all cases (or directly the same
> instruction in JIT compiled code).
> If CPU does not have instruction we should use runtime C++ function in all
> cases to be consistent.
> Thanks @vnkozlov @dean-long. One last question before I withdraw the PR: As
> QNaN bit is supported across current architectures like x86, ARM and may be
> others as well for conversion, couldn't we go ahead with this PR? The
> architectures that behave differently could then follow the technique
> suggested by Vladimir Kozlov as and when they implement the intrinsic?
No, because it's not just the SNaN vs QNaN that is different, but also the NaN
"payload" or "boxing" that is different. For example, the intrinsic gives me
different results on aarch64 vs Intel with this test:
public class Foo {
public static float hf2f(short s) {
return Float.floatToFloat16(s);
}
public static short f2hf(float f) {
return Float.floatToFloat16(f);
}
public static void main(String[] args) {
float f = Float.intBitsToFloat(0x7fc0 | 0x2000 );
System.out.println(Integer.toHexString(f2hf(f)));
f = Float.intBitsToFloat(0x7fc0 | 0x20 );
System.out.println(Integer.toHexString(f2hf(f)));
f = Float.intBitsToFloat(0x7fc0 | 0x4);
System.out.println(Integer.toHexString(f2hf(f)));
f = Float.intBitsToFloat(0x7fc0 | 0x2000 | 0x20 | 0x4);
System.out.println(Integer.toHexString(f2hf(f)));
}
}
-
PR: https://git.openjdk.org/jdk/pull/12704
Re: RFR: 8302806: (fs) Remove com.sun.nio.file.SensitivityWatchEventModifier [v3]
> This enum is not used in the JDK and did not appear in external source code > searches. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8302806: Remove sensitivity from PollingWatchService - Changes: - all: https://git.openjdk.org/jdk/pull/12626/files - new: https://git.openjdk.org/jdk/pull/12626/files/c980dba9..e17f66c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12626=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12626=01-02 Stats: 21 lines in 1 file changed: 1 ins; 8 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/12626.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12626/head:pull/12626 PR: https://git.openjdk.org/jdk/pull/12626
Re: RFR: 8292914: Drop the counter from lambda class names
On 15 Feb 2023, at 10:34, Mandy Chung wrote: On Wed, 15 Feb 2023 17:32:38 GMT, David M. Lloyd wrote: The class generated for lambda proxies is now defined as a hidden class. This means that the counter, which was used to ensure a unique class name and avoid clashes, is now redundant. In addition to performing redundant work, this also impacts build reproducibility for native image generators which might already have a strategy to cope with hidden classes but cannot cope with indeterminate definition order for lambda proxy classes. This solves JDK-8292914 by making lambda proxy names always be stable without any configuration needed. This would also replace #10024. we want the generated classes to be dumped for debugging before the hidden class is defined e.g. to troubleshoot class loading issue (see `-Djdk.internal.lambda.dumpProxyClasses=` system property) I agree with David that the extra counter is a problem. Further, I think it will continue to be a problem in any likely Leyden use cases, precisely because it blocks reproducibility and predictability. So, although Brian is right that there are likely to be more problems like it in the future, and we will surely want a comprehensive solution, I think removing the counter is legitimate incremental progress, not likely to be contradicted by future developments. While I’m agreeing with everybody here I’ll also agree with Mandy: We need a simple debugging story, and the global counter make it simple. But surely there are other things we could do as well, including using the global counter to name the dump file but *not* to name the class itself. That is, don’t pollute the HC classfile bytes with the counter “noise” but by all means use it to sequence debugging activities. Maybe even better, we could use the “decorated” class name assigned by the JVM, *after* the HC is loaded, to form the dump file name. One easy way to do this is rename the dump file after the JVM loads the HC and picks the decoration. (Note that the decoration is just the I-hash of the class mirror of the HC, and *also* has the good property that it does *not* pollute the classfile bytes. It’s OK that each fresh invocation of the JVM is likely to choose a different decoration, as long as we don’t try to predict it statically. This means backtraces cannot be fully predicted statically; tough luck there.) Another way to handle it is ask the JVM to do the file dumping. This might seem excessively indirect, given the simple thing we do now in the Java code, but… we probably want to be able to ask the JVM (for Leyden training runs) to report all class files loaded (especially those dynamically generated!) so we can analyze them and do good stuff with them. This implies that, whether or not we dump HC files from Java code, the *JVM also has to dump them* somewhere or other. Where this takes me is: The responsibility for dumping HC contents should be moved from Java code to the JVM. I still think removing the counter is a good step in isolation, but (as Brian says) the root issue is that we want good reporting of HC contents for more than just ad hoc debugging. So we need to think more broadly about how to preserve and report HC classfile contents, for Leyden as well as ad hoc debugging.
Integrated: JDK-8302028: Port fdlibm atan2 to Java
On Thu, 16 Feb 2023 19:30:06 GMT, Joe Darcy wrote: > Working down the porting list, next stop, atan2. This pull request has now been integrated. Changeset: fcaf8714 Author:Joe Darcy URL: https://git.openjdk.org/jdk/commit/fcaf871408321ed523cf1c6dd3adf9914f2bf9aa Stats: 615 lines in 6 files changed: 595 ins; 8 del; 12 mod 8302028: Port fdlibm atan2 to Java Reviewed-by: bpb - PR: https://git.openjdk.org/jdk/pull/12608
Re: RFR: JDK-8302028: Port fdlibm atan2 to Java [v5]
On Tue, 21 Feb 2023 23:03:04 GMT, Joe Darcy wrote: >> Working down the porting list, next stop, atan2. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Looks good. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.org/jdk/pull/12608
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 21:21:42 GMT, Vladimir Kozlov wrote: >>> I'm also a bit concerned that we are rushing in to "fix" this. IIUC we have >>> three mechanisms for implementing this functionality: >>> >>> 1. The interpreted Java code >>> >>> 2. The compiled non-intrinisc sharedRuntime code >>> >>> 3. The compiler intrinsic that uses a hardware instruction. >>> >>> >>> Unless the hardware instructions for all relevant CPUs behave exactly the >>> same, then I don't see how we can have parity of behaviour across these >>> three mechanisms. >>> >>> The observed behaviour may be surprising but it seems not to be a bug. And >>> is this even a real concern - would real programs actually need to peek at >>> the raw bits and so see the difference, or does it suffice to handle Nan's >>> opaquely? >> >> From the spec >> (https://download.java.net/java/early_access/jdk20/docs/api/java.base/java/lang/Float.html#float16ToFloat(short)) >> >> "Returns the float value closest to the numerical value of the argument, a >> floating-point binary16 value encoded in a short. The conversion is exact; >> all binary16 values can be exactly represented in float. Special cases: >> >> If the argument is zero, the result is a zero with the same sign as the >> argument. >> If the argument is infinite, the result is an infinity with the same >> sign as the argument. >> If the argument is a NaN, the result is a NaN. " >> >> If the float argument is a NaN, you are supposed to get a float16 NaN as a >> result -- that is all the specification requires. However, the >> implementation makes stronger guarantees to try to preserve some non-zero >> NaN significand bits if they are set. >> >> "NaN boxing" is a technique used to put extra information into the >> significand bits a NaN and pass the around. It is consistent with the >> intended use of the feature by IEEE 754 and used in various language >> runtimes: e.g., >> >> https://piotrduperas.com/posts/nan-boxing >> https://leonardschuetz.ch/blog/nan-boxing/ >> https://anniecherkaev.com/the-secret-life-of-nan >> >> The Java specs are careful to avoid mentioning quiet vs signaling NaNs in >> general discussion. >> >> That said, I think it is reasonable on a given JVM invocation if >> Float.floatToFloat16(f) gave the same result for input f regardless of in >> what context it was called. > >> We don't know that all HW will produce the same NaN "payload", right? >> Instead, we might need interpreter intrinsics. I assume that is how the trig >> functions are handled that @jddarcy mentioned. > > Good point. We can't guarantee that all OpenJDK ports HW do the same. > > If CPU has corresponding instructions we need to generate a stub during VM > startup with HW instructions and use it in all cases (or directly the same > instruction in JIT compiled code). > If CPU does not have instruction we should use runtime C++ function in all > cases to be consistent. Thanks @vnkozlov @dean-long. One last question before I withdraw the PR: As QNaN bit is supported across current architectures like x86, ARM and may be others as well for conversion, couldn't we go ahead with this PR? The architectures that behave differently could then follow the technique suggested by Vladimir Kozlov as and when they implement the intrinsic? - PR: https://git.openjdk.org/jdk/pull/12704
Re: RFR: 8292914: Drop the counter from lambda class names [v8]
On Fri, 17 Feb 2023 19:37:59 GMT, David M. Lloyd wrote: >> The class generated for lambda proxies is now defined as a hidden class. >> This means that the counter, which was used to ensure a unique class name >> and avoid clashes, is now redundant. In addition to performing redundant >> work, this also impacts build reproducibility for native image generators >> which might already have a strategy to cope with hidden classes but cannot >> cope with indeterminate definition order for lambda proxy classes. >> >> This solves JDK-8292914 by making lambda proxy names always be stable >> without any configuration needed. This would also replace #10024. > > David M. Lloyd has updated the pull request incrementally with two additional > commits since the last revision: > > - Many tests have patterns for lambda class names; update them > - Update comments and javadoc showin the old pattern This looks ok to me but I’ll defer to Mandy. I’m also on vacation so don’t wait for me :-) - PR: https://git.openjdk.org/jdk/pull/12579
Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]
On Wed, 22 Feb 2023 17:05:00 GMT, Kasper Nielsen wrote: >> Volker Simonis has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Remove assertions which insist on Lambda proxy classes being strongly >> linked to their class loader >> - Removed unused import of STRONG und updated copyright year > > In all the places I've seen LambdaMetaFactory used, it is because of > performance over reflection/non-static method handles. See, for example, > https://www.optaplanner.org/blog/2018/01/09/JavaReflectionButMuchFaster.html. > I believe Optaplanner is still using it. > > There is also quite a number of posts on StackOverflow on people trying to > use LambdaMetafactory: > https://stackoverflow.com/search?q=LambdaMetafactory @kaspernielsen, i believe that now that hidden class + class data are part of the public API, you do not need to use lambda metafactory directly. But it requires Java 17 not 8 or 11. - PR: https://git.openjdk.org/jdk/pull/12493
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 05:21:48 GMT, Joe Darcy wrote: >> I'm also a bit concerned that we are rushing in to "fix" this. IIUC we have >> three mechanisms for implementing this functionality: >> >> 1. The interpreted Java code >> 2. The compiled non-intrinisc sharedRuntime code >> 3. The compiler intrinsic that uses a hardware instruction. >> >> Unless the hardware instructions for all relevant CPUs behave exactly the >> same, then I don't see how we can have parity of behaviour across these >> three mechanisms. >> >> The observed behaviour may be surprising but it seems not to be a bug. And >> is this even a real concern - would real programs actually need to peek at >> the raw bits and so see the difference, or does it suffice to handle Nan's >> opaquely? > >> I'm also a bit concerned that we are rushing in to "fix" this. IIUC we have >> three mechanisms for implementing this functionality: >> >> 1. The interpreted Java code >> >> 2. The compiled non-intrinisc sharedRuntime code >> >> 3. The compiler intrinsic that uses a hardware instruction. >> >> >> Unless the hardware instructions for all relevant CPUs behave exactly the >> same, then I don't see how we can have parity of behaviour across these >> three mechanisms. >> >> The observed behaviour may be surprising but it seems not to be a bug. And >> is this even a real concern - would real programs actually need to peek at >> the raw bits and so see the difference, or does it suffice to handle Nan's >> opaquely? > > From the spec > (https://download.java.net/java/early_access/jdk20/docs/api/java.base/java/lang/Float.html#float16ToFloat(short)) > > "Returns the float value closest to the numerical value of the argument, a > floating-point binary16 value encoded in a short. The conversion is exact; > all binary16 values can be exactly represented in float. Special cases: > > If the argument is zero, the result is a zero with the same sign as the > argument. > If the argument is infinite, the result is an infinity with the same sign > as the argument. > If the argument is a NaN, the result is a NaN. " > > If the float argument is a NaN, you are supposed to get a float16 NaN as a > result -- that is all the specification requires. However, the implementation > makes stronger guarantees to try to preserve some non-zero NaN significand > bits if they are set. > > "NaN boxing" is a technique used to put extra information into the > significand bits a NaN and pass the around. It is consistent with the > intended use of the feature by IEEE 754 and used in various language > runtimes: e.g., > > https://piotrduperas.com/posts/nan-boxing > https://leonardschuetz.ch/blog/nan-boxing/ > https://anniecherkaev.com/the-secret-life-of-nan > > The Java specs are careful to avoid mentioning quiet vs signaling NaNs in > general discussion. > > That said, I think it is reasonable on a given JVM invocation if > Float.floatToFloat16(f) gave the same result for input f regardless of in > what context it was called. > We don't know that all HW will produce the same NaN "payload", right? > Instead, we might need interpreter intrinsics. I assume that is how the trig > functions are handled that @jddarcy mentioned. Good point. We can't guarantee that all OpenJDK ports HW do the same. If CPU has corresponding instructions we need to generate a stub during VM startup with HW instructions and use it in all cases (or directly the same instruction in JIT compiled code). If CPU does not have instruction we should use runtime C++ function in all cases to be consistent. - PR: https://git.openjdk.org/jdk/pull/12704
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 05:21:48 GMT, Joe Darcy wrote: >> I'm also a bit concerned that we are rushing in to "fix" this. IIUC we have >> three mechanisms for implementing this functionality: >> >> 1. The interpreted Java code >> 2. The compiled non-intrinisc sharedRuntime code >> 3. The compiler intrinsic that uses a hardware instruction. >> >> Unless the hardware instructions for all relevant CPUs behave exactly the >> same, then I don't see how we can have parity of behaviour across these >> three mechanisms. >> >> The observed behaviour may be surprising but it seems not to be a bug. And >> is this even a real concern - would real programs actually need to peek at >> the raw bits and so see the difference, or does it suffice to handle Nan's >> opaquely? > >> I'm also a bit concerned that we are rushing in to "fix" this. IIUC we have >> three mechanisms for implementing this functionality: >> >> 1. The interpreted Java code >> >> 2. The compiled non-intrinisc sharedRuntime code >> >> 3. The compiler intrinsic that uses a hardware instruction. >> >> >> Unless the hardware instructions for all relevant CPUs behave exactly the >> same, then I don't see how we can have parity of behaviour across these >> three mechanisms. >> >> The observed behaviour may be surprising but it seems not to be a bug. And >> is this even a real concern - would real programs actually need to peek at >> the raw bits and so see the difference, or does it suffice to handle Nan's >> opaquely? > > From the spec > (https://download.java.net/java/early_access/jdk20/docs/api/java.base/java/lang/Float.html#float16ToFloat(short)) > > "Returns the float value closest to the numerical value of the argument, a > floating-point binary16 value encoded in a short. The conversion is exact; > all binary16 values can be exactly represented in float. Special cases: > > If the argument is zero, the result is a zero with the same sign as the > argument. > If the argument is infinite, the result is an infinity with the same sign > as the argument. > If the argument is a NaN, the result is a NaN. " > > If the float argument is a NaN, you are supposed to get a float16 NaN as a > result -- that is all the specification requires. However, the implementation > makes stronger guarantees to try to preserve some non-zero NaN significand > bits if they are set. > > "NaN boxing" is a technique used to put extra information into the > significand bits a NaN and pass the around. It is consistent with the > intended use of the feature by IEEE 754 and used in various language > runtimes: e.g., > > https://piotrduperas.com/posts/nan-boxing > https://leonardschuetz.ch/blog/nan-boxing/ > https://anniecherkaev.com/the-secret-life-of-nan > > The Java specs are careful to avoid mentioning quiet vs signaling NaNs in > general discussion. > > That said, I think it is reasonable on a given JVM invocation if > Float.floatToFloat16(f) gave the same result for input f regardless of in > what context it was called. We don't know that all HW will produce the same NaN "payload", right? Instead, we might need interpreter intrinsics. I assume that is how the trig functions are handled that @jddarcy mentioned. - PR: https://git.openjdk.org/jdk/pull/12704
Re: RFR: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider
On Tue, 21 Feb 2023 13:44:52 GMT, Madjosz wrote:
> Fixes JDK-8302983 (and duplicate JDK-8302898)
Thanks for fixing this. Some comments follow.
test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 43:
> 41: /**
> 42: * @summary Tests for ZoneRulesProvider class.
> 43: * @bug 8299571
Can be collapsed into one line
test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 119:
> 117: return null;
> 118: }
> 119: }
Needs a semi-colon, otherwise would not compile.
Anyway, this inner class can be combined with the one in the test above, and
made into a separate class.
test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 123:
> 121: ZoneRulesProvider.registerProvider(provider);
> 122: assertTrue(ZoneId.getAvailableZoneIds().contains(zone),
> "Unexpected non-availability for " + zone);
> 123: assertNotNull(ZoneId.of(zone), "ZoneId instance for " + zone + "
> should be obtainable");
If the `zone` does not exist, it will not return `null` but throw an exception.
Assertion needs to be modified correctly.
test/jdk/java/time/test/java/time/zone/TestZoneRulesProvider.java line 136:
> 134: // instantiation check
> 135: try {
> 136: assertNotNull(ZoneId.of(zone), "ZoneId instance for " + zone
> + " should still be obtainable");
Same here.
-
Changes requested by naoto (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12690
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v14]
> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying > 'the oldest ASCII trick in the book'. > > The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two > latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is > updated to use `equalsIgnoreCase` > > To verify the correctness of `equalsIgnoreCase`, a new test is added to > `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1 > code point pairs have an `equalsIgnoreCase` consistent with > Character.toUpperCase, Character.toLowerCase. > > Performance is tested for matching and mismatching cases of code point pairs > picked from the ASCII letter, ASCII number and latin1 letter ranges. Results > in the first comment below. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 21 additional commits since the last revision: - Merge branch 'master' into regionmatches-latin1-speedup - Merge branch 'master' into regionmatches-latin1-speedup - Make the loop variables chars to avoid casting - Use improved case-twiddling comment as suggested by Martin - Replace 'oldest ASCII trick in the book' use in toUpperCase, toLowerCase with "by removing (setting) a single bit" - Align local variable naming in toLowerCase, toUpperCase with equalsIgnoreCase by using 'lower' and 'upper' - Rename unconventionally named local variable 'U' to 'upper' - Merge remote-tracking branch 'origin/master' into regionmatches-latin1-speedup - Add whitespace between methods - Merge branch 'master' into regionmatches-latin1-speedup - ... and 11 more: https://git.openjdk.org/jdk/compare/e7379286...597b346a - Changes: - all: https://git.openjdk.org/jdk/pull/12632/files - new: https://git.openjdk.org/jdk/pull/12632/files/fe4efe9f..597b346a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12632=13 - incr: https://webrevs.openjdk.org/?repo=jdk=12632=12-13 Stats: 9 lines in 5 files changed: 1 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12632.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12632/head:pull/12632 PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v11]
On Wed, 22 Feb 2023 19:39:28 GMT, Ian Graves wrote: >> This is an approach to adding a flag to jlink that will allow --compress to >> take the same types of arguments as jmod, thus bringing the two into >> alignment. This likely requires a CSR and a discussion on whether we should >> deprecate or simply remove the original numeric compression arguments. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Adding runtime testing of new compression args Added additional test runs in JLinkTest to check runtime images that have been compressed using zip-X style flags. - PR: https://git.openjdk.org/jdk/pull/11617
Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v11]
> This is an approach to adding a flag to jlink that will allow --compress to > take the same types of arguments as jmod, thus bringing the two into > alignment. This likely requires a CSR and a discussion on whether we should > deprecate or simply remove the original numeric compression arguments. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Adding runtime testing of new compression args - Changes: - all: https://git.openjdk.org/jdk/pull/11617/files - new: https://git.openjdk.org/jdk/pull/11617/files/b2c4457f..74ec4f4b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11617=10 - incr: https://webrevs.openjdk.org/?repo=jdk=11617=09-10 Stats: 26 lines in 1 file changed: 26 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11617.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11617/head:pull/11617 PR: https://git.openjdk.org/jdk/pull/11617
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v13]
> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying
> 'the oldest ASCII trick in the book'.
>
> The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two
> latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is
> updated to use `equalsIgnoreCase`
>
> To verify the correctness of `equalsIgnoreCase`, a new test is added to
> `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1
> code point pairs have an `equalsIgnoreCase` consistent with
> Character.toUpperCase, Character.toLowerCase.
>
> Performance is tested for matching and mismatching cases of code point pairs
> picked from the ASCII letter, ASCII number and latin1 letter ranges. Results
> in the first comment below.
Eirik Bjorsnos has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains 20 additional commits
since the last revision:
- Merge branch 'master' into regionmatches-latin1-speedup
- Make the loop variables chars to avoid casting
- Use improved case-twiddling comment as suggested by Martin
- Replace 'oldest ASCII trick in the book' use in toUpperCase, toLowerCase
with "by removing (setting) a single bit"
- Align local variable naming in toLowerCase, toUpperCase with
equalsIgnoreCase by using 'lower' and 'upper'
- Rename unconventionally named local variable 'U' to 'upper'
- Merge remote-tracking branch 'origin/master' into
regionmatches-latin1-speedup
- Add whitespace between methods
- Merge branch 'master' into regionmatches-latin1-speedup
- Remove whitespace following '('
- ... and 10 more: https://git.openjdk.org/jdk/compare/ea132c56...fe4efe9f
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12632/files
- new: https://git.openjdk.org/jdk/pull/12632/files/cc185293..fe4efe9f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12632=12
- incr: https://webrevs.openjdk.org/?repo=jdk=12632=11-12
Stats: 1397 lines in 120 files changed: 873 ins; 291 del; 233 mod
Patch: https://git.openjdk.org/jdk/pull/12632.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12632/head:pull/12632
PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 02:08:27 GMT, Sandhya Viswanathan wrote: > Change the java/lang/float.java and the corresponding shared runtime constant > expression evaluation to generate QNaN. > The HW instructions generate QNaNs and not SNaNs for floating point > instructions. This happens across double, float, and float16 data types. The > most significant bit of mantissa is set to 1 for QNaNs. The proposed fix do exactly what everyone asked - the same result from Java code (Interpreter), runtime (C++ code) and intrinsic (HW instruction). Since HW instruction is already produces QNaNs, PR fixes only Java code (Interpreter) and runtime (C++) code to produce QNaNs. @TobiHartmann created test which covers all cases and should be added to this PR. - PR: https://git.openjdk.org/jdk/pull/12704
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
On Wed, 22 Feb 2023 18:57:00 GMT, Alan Bateman wrote: >> The cleaning action would not have access to the isShutdown() instance >> method of the (Phantom-reachable) AutoShutdownDelegatedExecutorService. > >> The cleaning action would not have access to the isShutdown() instance >> method of the (Phantom-reachable) AutoShutdownDelegatedExecutorService. > > The cleaning action has a reference to the delegate (the underlying > ExecutorService) so it can test if it shutdown as Daniel suggests - it's more > of an optimization to avoid doing a second call to shutdown in a privileged > action. OK, right - PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
On Wed, 22 Feb 2023 18:41:13 GMT, Brent Christian wrote: > The cleaning action would not have access to the isShutdown() instance method > of the (Phantom-reachable) AutoShutdownDelegatedExecutorService. The cleaning action has a reference to the delegate (the underlying ExecutorService) so it can test if it shutdown as Daniel's suggests - it's more of an optimization to avoid doing a second call to shutdown in a privileged action. - PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v12]
On Wed, 22 Feb 2023 16:37:45 GMT, Eirik Bjorsnos wrote: >> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying >> 'the oldest ASCII trick in the book'. >> >> The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two >> latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is >> updated to use `equalsIgnoreCase` >> >> To verify the correctness of `equalsIgnoreCase`, a new test is added to >> `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1 >> code point pairs have an `equalsIgnoreCase` consistent with >> Character.toUpperCase, Character.toLowerCase. >> >> Performance is tested for matching and mismatching cases of code point pairs >> picked from the ASCII letter, ASCII number and latin1 letter ranges. Results >> in the first comment below. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Make the loop variables chars to avoid casting Thanks for reviews Claes and Martin! I'll let this linger a bit before integrating in case Alan has comments after the latest updates. - PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
On Wed, 22 Feb 2023 18:02:46 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/concurrent/Executors.java line 844:
>>
>>> 842: @Override
>>> 843: public void shutdown() {
>>> 844: cleaner.clean();
>>
>> Hmmm... so now shutdown no longer requires permission. You should probably
>> call super.shutdown() before invoking cleaner.clean(), assuming that
>> shutdown() can be called twice. You could modify the cleanup action to only
>> call shutdown() if isShutdown() returns false.
>
>> Hmmm... so now shutdown no longer requires permission. You should probably
>> call super.shutdown() before invoking cleaner.clean(), assuming that
>> shutdown() can be called twice. You could modify the cleanup action to only
>> call shutdown() if isShutdown() returns false.
>
> You are right. It will be a non-issue once the SM goes away but while that
> execution mode is supported then the shutdown method and the cleaner action
> have to be different (as it was initially).
The cleaning action would not have access to the isShutdown() instance method
of the (Phantom-reachable) AutoShutdownDelegatedExecutorService.
-
PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview)
On Wed, 22 Feb 2023 05:31:46 GMT, Martin Doerr wrote:
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
Thanks for looking into this port!
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1359:
> 1357: long size = elementCount * srcElementLayout.byteSize();
> 1358: srcImpl.checkAccess(srcOffset, size, true);
> 1359: if (dstImpl instanceof NativeMemorySegmentImpl &&
> dstImpl.byteSize() == 0) {
As Jorn said, this change is not acceptable, as it allows bulk copy
disregarding the segment real size. In such cases, the issue is always a
missing unsafe resize somewhere in the linker code.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8298619: java/io/File/GetXSpace.java is failing [v2]
On Tue, 14 Feb 2023 16:31:48 GMT, Brian Burkhalter wrote: >>> Another possibility would be to add a native method to the test itself to >>> invoke >>> [GetDiskSpaceInformationW](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getdiskspaceinformationw) >>> to obtain the value of `CallerTotalAllocationUnits` (in bytes) from the >>> [DISK_SPACE_INFORMATION](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/ns-fileapi-disk_space_information) >>> structure. >> >> That would be more reliable than parsing the output of `fsutil volume` so >> worth trying. You might have to dig into which of these win32 works with >> quotas as that seems to be part of the issue. The JDK uses >> GetDiskFreeSpaceExW. Also this test might have to do a few iterations so >> that it captures free space info while the system is in quiescence - I >> suspect some of the reliability issues has been concurrent activity that >> changes the volume space usage significantly while this test is running. > > There has definitely been a problem with quotas on Windows. I set up quotas > on actual Windows 11 hardware and replicated the same failure. I am not sure > how much lack of system quiescence is to blame, but there would be no harm in > building in some robustness for lack of quiescence while we're at it. Spawning `df` and `diskFree` processes have been replaced with native calls. For a reason as yet undetermined, the Windows function `GetDiskSpaceInformationW` fails to load on Windows Server 2016 so it is loaded dynamically and, if not found, a workaround is used. - PR: https://git.openjdk.org/jdk/pull/12397
Re: RFR: 8298619: java/io/File/GetXSpace.java is failing [v2]
> Modify the `Space` instances used for size comparison to be created with > total number of bytes derived from the Windows `diskFree` utility instead of > Cygwin’s `df`. Brian Burkhalter has updated the pull request incrementally with two additional commits since the last revision: - 8298619: Load GetDiskSpaceInformationW dynamically - 8298619: Replace df and diskFree with native calls - Changes: - all: https://git.openjdk.org/jdk/pull/12397/files - new: https://git.openjdk.org/jdk/pull/12397/files/9eef8cd9..42ad4cea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12397=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12397=00-01 Stats: 340 lines in 3 files changed: 206 ins; 97 del; 37 mod Patch: https://git.openjdk.org/jdk/pull/12397.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12397/head:pull/12397 PR: https://git.openjdk.org/jdk/pull/12397
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview)
On Wed, 22 Feb 2023 05:31:46 GMT, Martin Doerr wrote:
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
src/java.base/share/classes/jdk/internal/foreign/PlatformLayouts.java line 266:
> 264: * The {@code T*} native type.
> 265: */
> 266: public static final ValueLayout.OfAddress C_POINTER =
> ValueLayout.ADDRESS.withBitAlignment(64);
I think this is where the issue with the check in `MemorySegment::copy` comes
from. Note how other platforms add a call to `asUnbounded` for the created
layout, which makes any pointer boxed using this layout writable/readable (such
as the in memory return pointer for upcalls).
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8301119: Support for GB18030-2022 [v2]
On Wed, 22 Feb 2023 11:34:59 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/sun/nio/cs/StandardCharsets.java.template line
>> 217:
>>
>>> 215: if (VM.initLevel() < 1) {
>>> 216: // Cannot get the system property yet. Assumes non-2000
>>> 217: GB18030_2000 = "";
>>
>> curious - what scenario triggers this call at initLevel < 1 ? would it be
>> better to simply return "false" at that time and leave the GB18030_2000
>> variable to be set once we're at initLevel >=1 ? -- or perhaps that would
>> invalidate the workflow of the original caller (which called in at initLevel
>> <1)
>
>> curious - what scenario triggers this call at initLevel < 1 ?
>
> It's not supported, but it is possible that someone might run with
> -Dfile.encoding=GB18030, in which case the default charset is used before the
> system properties are initialized in initPhase1. Checking the init level
> breaks the circularity, the only downside is that can't switch to
> GB18030-2000 at the same time.
`Charset` class is initialized *before* system properties are set up, in order
to check the JNU encoding (used for file path name) is a supported charset or
not. In some OS environments, GB18030 is the native encoding so we need to
avoid checking the system property in such a case.
-
PR: https://git.openjdk.org/jdk/pull/12518
Re: RFR: 8301119: Support for GB18030-2022 [v2]
> Upgrading the GB18030 charset in the JDK to the latest 2022 standard. Since > this is not a compatible upgrade to the existing mapping, a new system > property `jdk.charset.GB18030` is introduced. If it is set to "2000", the > mapping falls back to the existing mapping based on the 2000 standard, > otherwise, it defaults to 2022 mapping. Refer to the corresponding CSR for > more detail. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision: - Add @Stable annotation - Merge branch 'master' into JDK-8301119-GB18030-2022 - Check initPhase and don't call System.getProperty if in phase 1 - Some clean-up - Some more fixes - removed unnecessary method name composition - Removed unnecessary imports - aliases fix - Move GB18030 into standard charsets provider - indentation - ... and 6 more: https://git.openjdk.org/jdk/compare/0981ec17...b5379b69 - Changes: - all: https://git.openjdk.org/jdk/pull/12518/files - new: https://git.openjdk.org/jdk/pull/12518/files/0f3c25ce..b5379b69 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12518=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12518=00-01 Stats: 8209 lines in 333 files changed: 4912 ins; 1488 del; 1809 mod Patch: https://git.openjdk.org/jdk/pull/12518.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12518/head:pull/12518 PR: https://git.openjdk.org/jdk/pull/12518
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
On Wed, 22 Feb 2023 17:20:21 GMT, Daniel Fuchs wrote: > Hmmm... so now shutdown no longer requires permission. You should probably > call super.shutdown() before invoking cleaner.clean(), assuming that > shutdown() can be called twice. You could modify the cleanup action to only > call shutdown() if isShutdown() returns false. That's right. It will be a non-issue once the SM goes away but while that execution mode is supported then the shutdown method and the cleaner action have to be different (as it was initially). - PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 04:03:02 GMT, David Holmes wrote: >> Change the java/lang/float.java and the corresponding shared runtime >> constant expression evaluation to generate QNaN. >> The HW instructions generate QNaNs and not SNaNs for floating point >> instructions. This happens across double, float, and float16 data types. The >> most significant bit of mantissa is set to 1 for QNaNs. > > I'm also a bit concerned that we are rushing in to "fix" this. IIUC we have > three mechanisms for implementing this functionality: > > 1. The interpreted Java code > 2. The compiled non-intrinisc sharedRuntime code > 3. The compiler intrinsic that uses a hardware instruction. > > Unless the hardware instructions for all relevant CPUs behave exactly the > same, then I don't see how we can have parity of behaviour across these three > mechanisms. > > The observed behaviour may be surprising but it seems not to be a bug. And is > this even a real concern - would real programs actually need to peek at the > raw bits and so see the difference, or does it suffice to handle Nan's > opaquely? @dholmes-ora @jddarcy @TobiHartmann @vnkozlov From @dean-long 's comment in the JBS entry, he sees the same result on AARCH64 and Intel, i.e. the output has the QNaN bit set. Please let me know if we want to proceed with this PR or if it would be good to withdraw this. I am open to either suggestion. Please advice. - PR: https://git.openjdk.org/jdk/pull/12704
Integrated: 8302667: Improve message format when failing to load symbols or libraries
On Thu, 16 Feb 2023 15:02:38 GMT, Julian Waters wrote: > DLL_ERROR4 is a macro expanding to an error message when a failure to load a > generic item (shared libraries or an exported symbol from said libraries for > example) occurs. "Error: loading:" is not a very pretty message, so this > small change results in "Error: Failed to load %s" instead, which looks > better and also means the message makes more sense if we want to append a > reason behind as well, such as "Error: Failed to load libjvm.so because xxx" This pull request has now been integrated. Changeset: 8de841dd Author:Julian Waters URL: https://git.openjdk.org/jdk/commit/8de841dd19a77f9ff6273a74366c08f33e0cac94 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod 8302667: Improve message format when failing to load symbols or libraries Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/12596
Re: RFR: 8302667: Improve message format when failing to load symbols or libraries [v2]
On Tue, 21 Feb 2023 16:59:18 GMT, Mandy Chung wrote: >> Julian Waters has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into patch-6 >> - Error: Failed to load > > What about: > > > #define ARG_ERROR18 "Error: reading %s" @mlchung Thanks for the review! :) - PR: https://git.openjdk.org/jdk/pull/12596
Re: RFR: 8302667: Improve message format when failing to load symbols or libraries [v4]
> DLL_ERROR4 is a macro expanding to an error message when a failure to load a > generic item (shared libraries or an exported symbol from said libraries for > example) occurs. "Error: loading:" is not a very pretty message, so this > small change results in "Error: Failed to load %s" instead, which looks > better and also means the message makes more sense if we want to append a > reason behind as well, such as "Error: Failed to load libjvm.so because xxx" Julian Waters has updated the pull request incrementally with one additional commit since the last revision: src/java.base/share/native/libjli/emessages.h Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/12596/files - new: https://git.openjdk.org/jdk/pull/12596/files/6b54816a..d923ef45 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12596=03 - incr: https://webrevs.openjdk.org/?repo=jdk=12596=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12596.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12596/head:pull/12596 PR: https://git.openjdk.org/jdk/pull/12596
Re: RFR: 8302267: [jittester] Improve separation of test generation and execution [v2]
> Please review a set of improvements that should improve working with other > fuzzing generators and usage of JitTesterDriver with tests generated not by > the JITTester: > > - Provide better separation of individual test generation from java file > writing, compiling, executing, etc.; > - Introduce distinct Phases of the generation process (Generation, > Compilation, GoldRun and VerificationRun); > - Extract JItTesterDriver headers generation so that it would be possible to > provide other header generators; > - Introduce error tolerance to not get distracted by OOMEs, intrinsics > missing in the compiled code, etc.; > - Make it possible to specify time limit for an individual test generation; > - Give better control over temp/workdir creation and cleaning; > - Unify external process running; > - Introduce UTF-8 support in external processes' output and human-readable > escaping of it; Evgeny Nikitin has updated the pull request incrementally with one additional commit since the last revision: Ignore large files - Changes: - all: https://git.openjdk.org/jdk/pull/12527/files - new: https://git.openjdk.org/jdk/pull/12527/files/417e05d7..0d1543a7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12527=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12527=00-01 Stats: 98 lines in 5 files changed: 77 ins; 8 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/12527.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12527/head:pull/12527 PR: https://git.openjdk.org/jdk/pull/12527
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview)
On Wed, 22 Feb 2023 17:03:16 GMT, Jorn Vernee wrote: > (I'd be happy to implement the needed changes in shared code if you want, > since it touches `BindingSpecializer` which is pretty dense) FYI: https://github.com/openjdk/jdk/compare/master...JornVernee:jdk:I2L (assuming `I2L` has the same semantics as `extsw`). Then just add a `.cast(int.class, long.class)` wherever an `int` is stored. - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
On Wed, 22 Feb 2023 16:17:11 GMT, Alan Bateman wrote:
>> Executors.newSingleThreadExecutor returns a delegating ExecutorService that
>> has finalizer to shutdown the underlying TPE when the wrapper is
>> finalizable. It goes back to JDK 6 and JDK-6399443. This is the last
>> non-empty finalizer in java.base. Removing it will likely lead to bug
>> reports/complaints as the current behavior goes back to 2006. So the
>> proposal is to just replace it with a Cleaner, trivially done in this case.
>> As part of the changes, I've replaced the existing test with a more modern
>> test that exercises more scenarios.
>
> Alan Bateman has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Keep reference to Cleanable
> - Merge
> - Fix typo in comment, remove blank line
> - Replace older test
> - Initial commit
src/java.base/share/classes/java/util/concurrent/Executors.java line 844:
> 842: @Override
> 843: public void shutdown() {
> 844: cleaner.clean();
Hmmm... so now shutdown no longer requires permission. You should probably call
super.shutdown() before invoking cleaner.clean(), assuming that shutdown() can
be called twice.
-
PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]
On Thu, 9 Feb 2023 18:11:18 GMT, Volker Simonis wrote: >> Prior to >> [JDK-8239384](https://bugs.openjdk.org/browse/JDK-8239384)/[JDK-8238358](https://bugs.openjdk.org/browse/JDK-8238358) >> LambdaMetaFactory has created VM-anonymous classes which could easily be >> unloaded once they were not referenced any more. Starting with JDK 15 and >> the new "hidden class" based implementation, this is not the case any more, >> because the hidden classes will be strongly tied to their defining class >> loader. If this is the default application class loader, these hidden >> classes can never be unloaded which can easily lead to Metaspace exhaustion >> (see the [test case in the JBS >> issue](https://bugs.openjdk.org/secure/attachment/102601/LambdaClassLeak.java)). >> This is a regression compared to previous JDK versions which some of our >> applications have been affected from when migrating to JDK 17. >> >> The reason why the newly created hidden classes are strongly linked to their >> defining class loader is not clear to me. JDK-8239384 mentions it as an >> "implementation detail": >> >>> *4. the lambda proxy class has the strong relationship with the class >>> loader (that will share the VM metaspace for its defining loader - >>> implementation details)* >> >> From my current understanding the strong link between a hidden class created >> by `LambdaMetaFactory` and its defining class loader is not strictly >> required. In order to prevent potential OOMs and fix the regression compared >> the JDK 14 and earlier I propose to create these hidden classes without the >> `STRONG` option. >> >> I'll be happy to add the test case as JTreg test to this PR if you think >> that would be useful. > > Volker Simonis has updated the pull request incrementally with two additional > commits since the last revision: > > - Remove assertions which insist on Lambda proxy classes being strongly > linked to their class loader > - Removed unused import of STRONG und updated copyright year In all the places I've seen LambdaMetaFactory used, it is because of performance over reflection/non-static method handles. See, for example, https://www.optaplanner.org/blog/2018/01/09/JavaReflectionButMuchFaster.html. I believe Optaplanner is still using it. There is also quite a number of posts on StackOverflow on people trying to use LambdaMetafactory: https://stackoverflow.com/search?q=LambdaMetafactory - PR: https://git.openjdk.org/jdk/pull/12493
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview)
On Wed, 22 Feb 2023 05:31:46 GMT, Martin Doerr wrote:
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
I will do a more thorough review soon.
Some preliminary comments:
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
FWIW, we have to do this for Windows vararg floats as well
([here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java#L231-L239))
This can be done by `dup`-ing the value, and using 2 `vmStore`s. (each
`vmStore` corresponding to a single register/stack location). Doing something
similar might be simpler than the `INTEGER_AND_FLOAT` and `STACK_AND_FLOAT`
storage types you're using right now. I'm not sure if that is related to the
other limitations you mention? Might be interesting to look into. (perhaps as a
separate RFE. I don't have a big issue since the current approach stays in
PPC-only code)
> I had to make changes to shared code and code for other platforms:
>
> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
> This is needed for the following reasons:
>
>
> * PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
>
> * Floating point load / store instructions need the correct width to
> select between the correct IEEE 754 formats. The register representation in
> single FP registers is always IEEE 754 double precision on PPC64.
>
> * Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
I think supplying the `BasicType` is fine. `VMReg` doesn't have any width
information attached to it, and that's why a complementary `BasicType` is
needed. I'm glad to see that you could make it work with the register masks for
`VMStorage` :)
WRT the extension of int -> long. This could potentially also be handled in
Java by adding the conversion as a `Cast` binding variant, and then adding the
widening casts in `CallArranger`. (I'd be happy to implement the needed changes
in shared code if you want, since it touches `BindingSpecializer` which is
pretty dense). Since the extension seems to be a figment of the C ABI, that
could be preferable, since it
Re: RFR: 8302667: Improve message format when failing to load symbols or libraries [v3]
On Tue, 21 Feb 2023 17:14:57 GMT, Julian Waters wrote: >> DLL_ERROR4 is a macro expanding to an error message when a failure to load a >> generic item (shared libraries or an exported symbol from said libraries for >> example) occurs. "Error: loading:" is not a very pretty message, so this >> small change results in "Error: Failed to load %s" instead, which looks >> better and also means the message makes more sense if we want to append a >> reason behind as well, such as "Error: Failed to load libjvm.so because xxx" > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - expandArgFile > - ARG_ERROR18 Thanks in making the change. src/java.base/share/native/libjli/emessages.h line 60: > 58: #define ARG_ERROR16 "Error: Option %s in %s is not allowed in this > context" > 59: #define ARG_ERROR17 "Error: Cannot specify main class in this context" > 60: #define ARG_ERROR18 "Error: Could not read %s" Suggestion: #define ARG_ERROR18 "Error: Failed to read %s" - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.org/jdk/pull/12596
Re: RFR: 8292914: Drop the counter from lambda class names [v8]
On Fri, 17 Feb 2023 19:37:59 GMT, David M. Lloyd wrote: >> The class generated for lambda proxies is now defined as a hidden class. >> This means that the counter, which was used to ensure a unique class name >> and avoid clashes, is now redundant. In addition to performing redundant >> work, this also impacts build reproducibility for native image generators >> which might already have a strategy to cope with hidden classes but cannot >> cope with indeterminate definition order for lambda proxy classes. >> >> This solves JDK-8292914 by making lambda proxy names always be stable >> without any configuration needed. This would also replace #10024. > > David M. Lloyd has updated the pull request incrementally with two additional > commits since the last revision: > > - Many tests have patterns for lambda class names; update them > - Update comments and javadoc showin the old pattern I'm on vacation. I'll review later this week. - PR: https://git.openjdk.org/jdk/pull/12579
Re: RFR: 8292914: Drop the counter from lambda class names [v8]
On Fri, 17 Feb 2023 19:37:59 GMT, David M. Lloyd wrote: >> The class generated for lambda proxies is now defined as a hidden class. >> This means that the counter, which was used to ensure a unique class name >> and avoid clashes, is now redundant. In addition to performing redundant >> work, this also impacts build reproducibility for native image generators >> which might already have a strategy to cope with hidden classes but cannot >> cope with indeterminate definition order for lambda proxy classes. >> >> This solves JDK-8292914 by making lambda proxy names always be stable >> without any configuration needed. This would also replace #10024. > > David M. Lloyd has updated the pull request incrementally with two additional > commits since the last revision: > > - Many tests have patterns for lambda class names; update them > - Update comments and javadoc showin the old pattern Could I please trouble someone for a review? - PR: https://git.openjdk.org/jdk/pull/12579
Re: RFR: 8292914: Drop the counter from lambda class names [v8]
On Tue, 21 Feb 2023 19:08:30 GMT, Joe Darcy wrote: >> David M. Lloyd has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Many tests have patterns for lambda class names; update them >> - Update comments and javadoc showin the old pattern > > Should this have a CSR for any observable behavioral changes? @jddarcy The class name is effectively invisible (it's a hidden class) to ordinary code. The classfile dumper is intended solely for debugging of the code generator itself and its behavior is unspecified. So I would think that no CSR-able surface exists here. - PR: https://git.openjdk.org/jdk/pull/12579
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v12]
> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying > 'the oldest ASCII trick in the book'. > > The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two > latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is > updated to use `equalsIgnoreCase` > > To verify the correctness of `equalsIgnoreCase`, a new test is added to > `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1 > code point pairs have an `equalsIgnoreCase` consistent with > Character.toUpperCase, Character.toLowerCase. > > Performance is tested for matching and mismatching cases of code point pairs > picked from the ASCII letter, ASCII number and latin1 letter ranges. Results > in the first comment below. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Make the loop variables chars to avoid casting - Changes: - all: https://git.openjdk.org/jdk/pull/12632/files - new: https://git.openjdk.org/jdk/pull/12632/files/ea2f9fa3..cc185293 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12632=11 - incr: https://webrevs.openjdk.org/?repo=jdk=12632=10-11 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12632.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12632/head:pull/12632 PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v10]
On Wed, 22 Feb 2023 16:25:41 GMT, Martin Buchholz wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Replace 'oldest ASCII trick in the book' use in toUpperCase, toLowerCase
>> with "by removing (setting) a single bit"
>> - Align local variable naming in toLowerCase, toUpperCase with
>> equalsIgnoreCase by using 'lower' and 'upper'
>
> test/jdk/java/lang/String/CompactString/EqualsIgnoreCase.java line 89:
>
>> 87: for (int ab = 0; ab < 256; ab++) {
>> 88: for (int bb = 0; bb < 256; bb++) {
>> 89: char a = (char) ab, b = (char) bb;
>
> char is an unsigned numeric type, so cleaner is
>
> for (char a = 0; a < 256; a++)
> for (char b = 0; b < 256; b++)
Thanks, fixed. Might have been copied over from processing of code points in
the higher planes. Not needed here.
-
PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v7]
On Wed, 22 Feb 2023 16:10:42 GMT, Martin Buchholz wrote:
>> Thanks Martin, David, Alan. This was instructive (and fun!)
>>
>> I suggest we condense the comment to something like this:
>>
>>
>> // Uppercase b1 by removing a single bit
>> int upper = b1 & 0xDF;
>> if (upper < 'A') {
>> return false; // Low ASCII
>> }
>> ...
>>
>>
>> The similar methods `toLowerCase` `toUpperCase` just above have been updated
>> to follow the same style. (I also updated local variable names there to
>> align better with equalsIgnoreCase)
>
> // ASCII and Latin-1 were designed to optimize case-twiddling operations
Thanks! This expresses the higher-level benefit succinctly, without getting
into the details. I like it!
-
PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v10]
On Wed, 22 Feb 2023 07:11:16 GMT, Eirik Bjorsnos wrote:
>> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying
>> 'the oldest ASCII trick in the book'.
>>
>> The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two
>> latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is
>> updated to use `equalsIgnoreCase`
>>
>> To verify the correctness of `equalsIgnoreCase`, a new test is added to
>> `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1
>> code point pairs have an `equalsIgnoreCase` consistent with
>> Character.toUpperCase, Character.toLowerCase.
>>
>> Performance is tested for matching and mismatching cases of code point pairs
>> picked from the ASCII letter, ASCII number and latin1 letter ranges. Results
>> in the first comment below.
>
> Eirik Bjorsnos has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Replace 'oldest ASCII trick in the book' use in toUpperCase, toLowerCase
> with "by removing (setting) a single bit"
> - Align local variable naming in toLowerCase, toUpperCase with
> equalsIgnoreCase by using 'lower' and 'upper'
Marked as reviewed by martin (Reviewer).
test/jdk/java/lang/String/CompactString/EqualsIgnoreCase.java line 89:
> 87: for (int ab = 0; ab < 256; ab++) {
> 88: for (int bb = 0; bb < 256; bb++) {
> 89: char a = (char) ab, b = (char) bb;
char is an unsigned numeric type, so cleaner is
for (char a = 0; a < 256; a++)
for (char b = 0; b < 256; b++)
-
PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v11]
> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying > 'the oldest ASCII trick in the book'. > > The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two > latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is > updated to use `equalsIgnoreCase` > > To verify the correctness of `equalsIgnoreCase`, a new test is added to > `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1 > code point pairs have an `equalsIgnoreCase` consistent with > Character.toUpperCase, Character.toLowerCase. > > Performance is tested for matching and mismatching cases of code point pairs > picked from the ASCII letter, ASCII number and latin1 letter ranges. Results > in the first comment below. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Use improved case-twiddling comment as suggested by Martin - Changes: - all: https://git.openjdk.org/jdk/pull/12632/files - new: https://git.openjdk.org/jdk/pull/12632/files/44d91544..ea2f9fa3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12632=10 - incr: https://webrevs.openjdk.org/?repo=jdk=12632=09-10 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12632.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12632/head:pull/12632 PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
> Executors.newSingleThreadExecutor returns a delegating ExecutorService that > has finalizer to shutdown the underlying TPE when the wrapper is finalizable. > It goes back to JDK 6 and JDK-6399443. This is the last non-empty finalizer > in java.base. Removing it will likely lead to bug reports/complaints as the > current behavior goes back to 2006. So the proposal is to just replace it > with a Cleaner, trivially done in this case. As part of the changes, I've > replaced the existing test with a more modern test that exercises more > scenarios. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Keep reference to Cleanable - Merge - Fix typo in comment, remove blank line - Replace older test - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/12675/files - new: https://git.openjdk.org/jdk/pull/12675/files/3bb4d0cd..3b135f09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12675=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12675=01-02 Stats: 2384 lines in 99 files changed: 1621 ins; 425 del; 338 mod Patch: https://git.openjdk.org/jdk/pull/12675.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12675/head:pull/12675 PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
On Tue, 21 Feb 2023 19:38:08 GMT, Brent Christian wrote:
>> Alan Bateman has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Keep reference to Cleanable
>> - Merge
>> - Fix typo in comment, remove blank line
>> - Replace older test
>> - Initial commit
>
> src/java.base/share/classes/java/util/concurrent/Executors.java line 837:
>
>> 835: implements ScheduledExecutorService {
>> 836: private final ScheduledExecutorService e;
>> 837: DelegatedScheduledExecutorService(ScheduledExecutorService
>> executor) {
>
> You might consider keeping a dedicated subclass
> ("CleanableDelegatedExecutorService"?). Such a class could save the Cleanable
> from Cleaner.register(), and override the shutdown() method to call
> Cleanable.clean(). This would reduce GC reference tracking, for instance in
> places where newSingleThreadExecutor() is used in a try-with-resources.
That might be better as that would clear the reference (so it won't be queued)
when explicit shutdown (or closed). We do that in several places (as you know)
at the cost of a reference to the Cleanable allowing "this" to escape during
construction.
-
PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v7]
On Wed, 22 Feb 2023 07:12:35 GMT, Eirik Bjorsnos wrote:
>>> Do you have an opinion on the appropriate level of documentation / comments
>>> for this kind of 'tricky' code?
>>
>> This code is not that tricky! And the proposed level of documentation is
>> excessive! A couple of lines of explanation and perhaps a link to an
>> external document would be good.
>>
>> It often happens to me that I will write such exhaustive notes for myself
>> when learning a new technology. A year later I pare it all back because
>> much of it is "obvious in retrospect".
>
> Thanks Martin, David, Alan. This was instructive (and fun!)
>
> I suggest we condense the comment to something like this:
>
>
> // Uppercase b1 by removing a single bit
> int upper = b1 & 0xDF;
> if (upper < 'A') {
> return false; // Low ASCII
> }
> ...
>
>
> The similar methods `toLowerCase` `toUpperCase` just above have been updated
> to follow the same style. (I also updated local variable names there to align
> better with equalsIgnoreCase)
// ASCII and Latin-1 were designed to optimize case-twiddling operations
-
PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302815 Use new Math.clamp method in core libraries [v3]
On Tue, 21 Feb 2023 20:39:53 GMT, Tagir F. Valeev wrote: >> For cleanup and dogfooding the new method, it would be nice to use >> Math.clamp where possible in java.base. See PR #12428. >> >> As Math.clamp performs an additional check that min is not greater than max, >> I conservatively replaced only those occurrences where I can see that this >> invariant is always held. There are more occurrences, where clamp can be >> potentially used but it's unclear whether min <= max is always true. > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year On second thought, maybe not; Math.clamp might actually look more clumsy here. Scratch my previous comment. - PR: https://git.openjdk.org/jdk/pull/12633
Re: RFR: 8302987: Add uniform and spatially equidistributed bounded float/double streams to RandomGenerator
On Wed, 22 Feb 2023 15:23:13 GMT, Raffaello Giulietti wrote: > The `default` method `nextDouble(double origin, double bound)` in > `java.util.random.RandomGenerator` aims at generating a uniformly and > spatially equidistributed random `double` in the left-closed and right-open > range [`origin`, `bound`). It does so by applying the affine transform > `origin + (bound - origin) * r` to a uniformly and spatially equidistributed > random `double` `r` in the range [0, 1). > > Since floating-point arithmetic suffers from small but noticeable rounding > errors, this ends up slightly deforming the distribution of `r` when applying > the affine transform. A similar implementation for `float`s will be added once this PR has elicited some interest and the algorithm has undergone a first review. A CSR and tests will then be added as well. - PR: https://git.openjdk.org/jdk/pull/12719
Re: RFR: 8302987: Add uniform and spatially equidistributed bounded float/double streams to RandomGenerator
On Wed, 22 Feb 2023 15:23:13 GMT, Raffaello Giulietti wrote: > The `default` method `nextDouble(double origin, double bound)` in > `java.util.random.RandomGenerator` aims at generating a uniformly and > spatially equidistributed random `double` in the left-closed and right-open > range [`origin`, `bound`). It does so by applying the affine transform > `origin + (bound - origin) * r` to a uniformly and spatially equidistributed > random `double` `r` in the range [0, 1). > > Since floating-point arithmetic suffers from small but noticeable rounding > errors, this ends up slightly deforming the distribution of `r` when applying > the affine transform. The effect of rounding errors in the affine transform is analyzed in: Goualard, "Drawing random floating-point numbers from an interval", ACM Transactions on Modeling and Computer Simulation, 2022, 32 (3) available [here] (https://hal.science/hal-03282794v4) The code in this PR is inspired by that paper, although it proposes another implementation preserving uniformity and equidistribution. - PR: https://git.openjdk.org/jdk/pull/12719
RFR: 8302987: Add uniform and spatially equidistributed bounded float/double streams to RandomGenerator
The `default` method `nextDouble(double origin, double bound)` in `java.util.random.RandomGenerator` aims at generating a uniformly and spatially equidistributed random `double` in the left-closed and right-open range [`origin`, `bound`). It does so by applying the affine transform `origin + (bound - origin) * r` to a uniformly and spatially equidistributed random `double` `r` in the range [0, 1). Since floating-point arithmetic suffers from small but noticeable rounding errors, this ends up slightly deforming the distribution of `r` when applying the affine transform. - Commit messages: - 8302987: Add uniform and spatially equidistributed bounded float/double streams to RandomGenerator Changes: https://git.openjdk.org/jdk/pull/12719/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12719=00 Issue: https://bugs.openjdk.org/browse/JDK-8302987 Stats: 253 lines in 1 file changed: 251 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12719.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12719/head:pull/12719 PR: https://git.openjdk.org/jdk/pull/12719
Re: RFR: 8302815 Use new Math.clamp method in core libraries [v3]
On Tue, 21 Feb 2023 20:39:53 GMT, Tagir F. Valeev wrote:
>> For cleanup and dogfooding the new method, it would be nice to use
>> Math.clamp where possible in java.base. See PR #12428.
>>
>> As Math.clamp performs an additional check that min is not greater than max,
>> I conservatively replaced only those occurrences where I can see that this
>> invariant is always held. There are more occurrences, where clamp can be
>> potentially used but it's unclear whether min <= max is always true.
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update copyright year
I only saw this PR after it has been integrated. A code location that
immediately came to mind but was missing in the change is this
java/util/concurrent/SubmissionPublisher.java:1273:
public final void request(long n) {
if (n > 0L) {
for (;;) {
long p = demand, d = p + n; // saturate
if (casDemand(p, d < p ? Long.MAX_VALUE : d))
break;
}
startOnSignal(RUN | ACTIVE | REQS);
}
else
onError(new IllegalArgumentException(
"non-positive subscription request"));
}
Seems like a poster child for the new Math.clamp functionality.
-
PR: https://git.openjdk.org/jdk/pull/12633
Re: RFR: 8301119: Support for GB18030-2022
On Fri, 10 Feb 2023 20:35:58 GMT, Naoto Sato wrote: > Upgrading the GB18030 charset in the JDK to the latest 2022 standard. Since > this is not a compatible upgrade to the existing mapping, a new system > property `jdk.charset.GB18030` is introduced. If it is set to "2000", the > mapping falls back to the existing mapping based on the 2000 standard, > otherwise, it defaults to 2022 mapping. Refer to the corresponding CSR for > more detail. Overall I think it looks very good, just StandardCharsets.isGB18030_2000 needs attention. Having GB18030 be in java.base in all builds, rather than everywhere except macOS, is okay and makes things a lot simpler. - PR: https://git.openjdk.org/jdk/pull/12518
Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]
On Thu, 9 Feb 2023 18:11:18 GMT, Volker Simonis wrote:
>> Prior to
>> [JDK-8239384](https://bugs.openjdk.org/browse/JDK-8239384)/[JDK-8238358](https://bugs.openjdk.org/browse/JDK-8238358)
>> LambdaMetaFactory has created VM-anonymous classes which could easily be
>> unloaded once they were not referenced any more. Starting with JDK 15 and
>> the new "hidden class" based implementation, this is not the case any more,
>> because the hidden classes will be strongly tied to their defining class
>> loader. If this is the default application class loader, these hidden
>> classes can never be unloaded which can easily lead to Metaspace exhaustion
>> (see the [test case in the JBS
>> issue](https://bugs.openjdk.org/secure/attachment/102601/LambdaClassLeak.java)).
>> This is a regression compared to previous JDK versions which some of our
>> applications have been affected from when migrating to JDK 17.
>>
>> The reason why the newly created hidden classes are strongly linked to their
>> defining class loader is not clear to me. JDK-8239384 mentions it as an
>> "implementation detail":
>>
>>> *4. the lambda proxy class has the strong relationship with the class
>>> loader (that will share the VM metaspace for its defining loader -
>>> implementation details)*
>>
>> From my current understanding the strong link between a hidden class created
>> by `LambdaMetaFactory` and its defining class loader is not strictly
>> required. In order to prevent potential OOMs and fix the regression compared
>> the JDK 14 and earlier I propose to create these hidden classes without the
>> `STRONG` option.
>>
>> I'll be happy to add the test case as JTreg test to this PR if you think
>> that would be useful.
>
> Volker Simonis has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Remove assertions which insist on Lambda proxy classes being strongly
> linked to their class loader
> - Removed unused import of STRONG und updated copyright year
I'd like to suggest maybe using another option for turning MethodHandles into
interface instances, such as using `MethodHandleProxies` or just lambda
expressions which capture the particular `MethodHandle`, which is an option if
the target type of the functional interface is statically known:
MethodHandle mh = ...
TargetType instance = (Widget w) -> {
try {
return (SomeType) mh.invokeExact(w);
} catch(Throwable t) {
throw new RuntimeException(t); // or something more nuanced
}
};
Most importantly, the above doesn't generate a new class every time, it
generates just one. It can be slower in specific situations due to lack of
inlining, but in the common use case it's not that bad (I've measure the extra
hop through the method handle to be 4ns in the past in microbenchmarks). You
could measure the impact for the particular app. (I guess the question is: do
you _really_ want a new class to be generated for each MethodHandle?)
-
PR: https://git.openjdk.org/jdk/pull/12493
RFR: JDK-8303072: Memory leak in exeNullCallerTest.cpp
Fix trivial memory leak that occurs during tests. - Commit messages: - Memory leak in exeNullCallerTest.cpp Changes: https://git.openjdk.org/jdk/pull/12715/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12715=00 Issue: https://bugs.openjdk.org/browse/JDK-8303072 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12715.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12715/head:pull/12715 PR: https://git.openjdk.org/jdk/pull/12715
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview)
On Wed, 22 Feb 2023 05:31:46 GMT, Martin Doerr wrote:
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
In the most recent version of the Panama FFM API, any memory layout (including
struct and padding layouts) are always byte aligned.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]
On Thu, 9 Feb 2023 18:11:18 GMT, Volker Simonis wrote: >> Prior to >> [JDK-8239384](https://bugs.openjdk.org/browse/JDK-8239384)/[JDK-8238358](https://bugs.openjdk.org/browse/JDK-8238358) >> LambdaMetaFactory has created VM-anonymous classes which could easily be >> unloaded once they were not referenced any more. Starting with JDK 15 and >> the new "hidden class" based implementation, this is not the case any more, >> because the hidden classes will be strongly tied to their defining class >> loader. If this is the default application class loader, these hidden >> classes can never be unloaded which can easily lead to Metaspace exhaustion >> (see the [test case in the JBS >> issue](https://bugs.openjdk.org/secure/attachment/102601/LambdaClassLeak.java)). >> This is a regression compared to previous JDK versions which some of our >> applications have been affected from when migrating to JDK 17. >> >> The reason why the newly created hidden classes are strongly linked to their >> defining class loader is not clear to me. JDK-8239384 mentions it as an >> "implementation detail": >> >>> *4. the lambda proxy class has the strong relationship with the class >>> loader (that will share the VM metaspace for its defining loader - >>> implementation details)* >> >> From my current understanding the strong link between a hidden class created >> by `LambdaMetaFactory` and its defining class loader is not strictly >> required. In order to prevent potential OOMs and fix the regression compared >> the JDK 14 and earlier I propose to create these hidden classes without the >> `STRONG` option. >> >> I'll be happy to add the test case as JTreg test to this PR if you think >> that would be useful. > > Volker Simonis has updated the pull request incrementally with two additional > commits since the last revision: > > - Remove assertions which insist on Lambda proxy classes being strongly > linked to their class loader > - Removed unused import of STRONG und updated copyright year Volker, i think you are mixing lambda metafactory and hidden classes. Hidden Classes have been envision has a way to generate adapters at runtime for languages that run on the JVM. They replace VM anonymous classes that were both unsafe and always not bound to a classloader. Lambda metafactory is the entry point to create lambda proxy for Java (the language). The current implementation is using hidden classes to represent lambda proxy. In the past, it was using VM anonymous classes that why lambdas were not tie to a classloader, the fact that the implementation was using VM anonymous class was leaking. To unload a Java class, its clasloader has to be unreachable. I see no reason this behavior should not be the same for Java lambdas classes. I believe it's what David is saying too. Now, for https://github.com/aws/aws-sdk-java-v2/issues/3701, you can fix it by using a ClassValue to cache the class and/or using a hidden class. For me, this has nothing to do with Java lambdas, i.e. nothing to do with the lambda metafactory. - PR: https://git.openjdk.org/jdk/pull/12493
Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]
On Wed, 22 Feb 2023 13:38:57 GMT, Volker Simonis wrote: >> I also have to disagree with the statement. The unit of unloading is the >> ClassLoader. > >> I also have to disagree with the statement. The unit of unloading is the >> ClassLoader. > > Hidden classes are not normal classes. They are not defined, created or > loaded by a class loader. The only reason why hidden classes maintain a link > to a *defining class loader* is because they need it to resolve types used by > the hidden class's own fields and methods. > > Some references from [JEP 371: Hidden Classes](https://openjdk.org/jeps/371): > >> Dynamically generated classes may only be needed for a limited time, so >> retaining them for the lifetime of the statically generated class might >> unnecessarily increase memory footprint. Existing workarounds for this >> situation, such as per-class class loaders, are cumbersome and inefficient. > >> A hidden class can be unloaded when it is no longer reachable, or it can >> share the lifetime of a class loader so that it is unloaded only when the >> class loader is garbage collected > >> A hidden class is not created by a class loader and has only a loose >> connection to the class loader deemed to be its defining loader. We can turn >> these facts to our advantage by allowing a hidden class to be unloaded even >> if its notional defining loader cannot be reclaimed by the garbage collector. > >> By default, Lookup::defineHiddenClass will create a hidden class that can be >> unloaded regardless of whether its notional defining loader is still alive. >> That is, when all instances of the hidden class are reclaimed and the hidden >> class is no longer reachable, it may be unloaded even though its notional >> defining loader is still reachable. This behavior is useful when a language >> runtime creates a hidden class to serve multiple classes defined by >> arbitrary class loaders: The runtime will see an improvement in footprint >> and performance relative to both `ClassLoader::defineClass()` and >> Unsafe::defineAnonymousClass()` > > The only reason why hidden classes created by `LambdaMetaFactory` are > strongly linked to a class loader (at least I haven't heard any other > argument until now in this thread) is to *save native memory* and not because > it is *logically required*! It's fine for me if you don't want to fix that. I > can just not understand why you are all still insisting that creating a new > ClassLoaderData object per hidden class is such a great decision and fixing > that would not be beneficial. > > Hidden classes were designed to be light-weight and easily unloadable (see > JEP references above). In the case of `LambdaMetaFactory` we unnecessarily > link them strongly to a class loader just because the current implementation > is too expensive otherwise. On a side note, the JDK already creates > non-strongly linked hidden classes as well, e.g. for > `java.lang.invoke.MethodHandles$Lookup.unreflect()`. > @simonis I want to ask a basic question -- what is the reason for your code > to call `LambdaMetafactory.metafactory()` directly? It looks like you want to > implement so sort of dynamic dispatch. Can equivalent functionality be > implemented by the app itself? > > If there's a real need for such a style of programming, and it requires some > sort of built-in support in by the JDK, maybe we should have a proper API > instead of piggy-backing on `LambdaMetafactory.metafactory()`. > > I think if you give us more background, we can make this a more productive > discussion than focusing on "did we make the right decision N versions ago" > without defining what "right" means :-) > > I would suggest re-booting this decision in the mailing lists rather than > continuing in this PR. The initial reason for this issue comes from here https://github.com/aws/aws-sdk-java-v2/issues/3701. That issue could be solved by caching. However, as I've outlined in my other answers above, I still think that one ClassLoaderData per hidden class is overly expensive and not really required. I'm fine if you don't want to fix that I just don't understand why you think it is a great solution? - PR: https://git.openjdk.org/jdk/pull/12493
Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]
On Tue, 21 Feb 2023 23:44:48 GMT, David Holmes wrote: > I also have to disagree with the statement. The unit of unloading is the > ClassLoader. Hidden classes are not normal classes. They are not defined, created or loaded by a class loader. The only reason why hidden classes maintain a link to a *defining class loader* is because they need it to resolve types used by the hidden class's own fields and methods. Some references from [JEP 371: Hidden Classes](https://openjdk.org/jeps/371): > Dynamically generated classes may only be needed for a limited time, so > retaining them for the lifetime of the statically generated class might > unnecessarily increase memory footprint. Existing workarounds for this > situation, such as per-class class loaders, are cumbersome and inefficient. > A hidden class can be unloaded when it is no longer reachable, or it can > share the lifetime of a class loader so that it is unloaded only when the > class loader is garbage collected > A hidden class is not created by a class loader and has only a loose > connection to the class loader deemed to be its defining loader. We can turn > these facts to our advantage by allowing a hidden class to be unloaded even > if its notional defining loader cannot be reclaimed by the garbage collector. > By default, Lookup::defineHiddenClass will create a hidden class that can be > unloaded regardless of whether its notional defining loader is still alive. > That is, when all instances of the hidden class are reclaimed and the hidden > class is no longer reachable, it may be unloaded even though its notional > defining loader is still reachable. This behavior is useful when a language > runtime creates a hidden class to serve multiple classes defined by arbitrary > class loaders: The runtime will see an improvement in footprint and > performance relative to both `ClassLoader::defineClass()` and > Unsafe::defineAnonymousClass()` The only reason why hidden classes created by `LambdaMetaFactory` are strongly linked to a class loader (at least I haven't heard any other argument until now in this thread) is to *save native memory* and not because it is *logically required*! It's fine for me if you don't want to fix that. I can just not understand why you are all still insisting that creating a new ClassLoaderData object per hidden class is such a great decision and fixing that would not be beneficial. Hidden classes were designed to be light-weight and easily unloadable (see JEP references above). In the case of `LambdaMetaFactory` we unnecessarily link them strongly to a class loader just because the current implementation is too expensive otherwise. On a side note, the JDK already creates non-strongly linked hidden classes as well, e.g. for `java.lang.invoke.MethodHandles$Lookup.unreflect()`. - PR: https://git.openjdk.org/jdk/pull/12493
Re: RFR: 8301119: Support for GB18030-2022
On Wed, 22 Feb 2023 10:46:10 GMT, Sean Coffey wrote: > curious - what scenario triggers this call at initLevel < 1 ? It's not supported, but it is possible that someone might run with -Dfile.encoding=GB18030, in which case the default charset is used before the system properties are initialized in initPhase1. Checking the init level breaks the circularity, the only downside is that can't switch to GB18030-2000 at the same time. - PR: https://git.openjdk.org/jdk/pull/12518
Re: RFR: 8302871: Speed up StringLatin1.regionMatchesCI [v10]
On Wed, 22 Feb 2023 07:11:16 GMT, Eirik Bjorsnos wrote: >> This PR suggests we can speed up `StringLatin1.regionMatchesCI` by applying >> 'the oldest ASCII trick in the book'. >> >> The new static method `CharacterDataLatin1.equalsIgnoreCase` compares two >> latin1 bytes for equality ignoring case. `StringLatin1.regionMatchesCI` is >> updated to use `equalsIgnoreCase` >> >> To verify the correctness of `equalsIgnoreCase`, a new test is added to >> `EqualsIgnoreCase` with an exhaustive verification that all 256x256 latin1 >> code point pairs have an `equalsIgnoreCase` consistent with >> Character.toUpperCase, Character.toLowerCase. >> >> Performance is tested for matching and mismatching cases of code point pairs >> picked from the ASCII letter, ASCII number and latin1 letter ranges. Results >> in the first comment below. > > Eirik Bjorsnos has updated the pull request incrementally with two additional > commits since the last revision: > > - Replace 'oldest ASCII trick in the book' use in toUpperCase, toLowerCase > with "by removing (setting) a single bit" > - Align local variable naming in toLowerCase, toUpperCase with > equalsIgnoreCase by using 'lower' and 'upper' Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12632
Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]
On Tue, 21 Feb 2023 19:49:24 GMT, Coleen Phillimore wrote: > The reason that non-strongly linked classes have their own ClassLoaderData is > because it implements the property that when the class loader is no longer > loaded, metadata for it can be removed at once. Even though Metaspace has > been redesigned to be less fragmented, this is simpler than removing selected > entities inside metaspace. Replacing it with a more sophisticated design > would not an be an improvement. So I have to disagree with your statement > above. How could it be not an improvement to save ~600 bytes of native memory per class? Sorry, but I start to have real problems to follow this discussion. First you don't want to change strongly linked Hidden Classes to weakly linked ones **because** of the native memory overhead and then you say that a more sophisticated implementation which will eliminate this overhead would **not** be an improvement. I'm puzzled. - PR: https://git.openjdk.org/jdk/pull/12493
Re: RFR: 8301119: Support for GB18030-2022
On Fri, 10 Feb 2023 20:35:58 GMT, Naoto Sato wrote:
> Upgrading the GB18030 charset in the JDK to the latest 2022 standard. Since
> this is not a compatible upgrade to the existing mapping, a new system
> property `jdk.charset.GB18030` is introduced. If it is set to "2000", the
> mapping falls back to the existing mapping based on the 2000 standard,
> otherwise, it defaults to 2022 mapping. Refer to the corresponding CSR for
> more detail.
src/java.base/share/classes/sun/nio/cs/StandardCharsets.java.template line 217:
> 215: if (VM.initLevel() < 1) {
> 216: // Cannot get the system property yet. Assumes non-2000
> 217: GB18030_2000 = "";
curious - what scenario triggers this call at initLevel < 1 ? would it be
better to simply return "false" at that time and leave the GB18030_2000
variable to be set once we're at initLevel >=1 ? -- or perhaps that would
invalidate the workflow of the original caller (which called in at initLevel <1)
-
PR: https://git.openjdk.org/jdk/pull/12518
Re: RFR: 8301119: Support for GB18030-2022
On Fri, 10 Feb 2023 20:35:58 GMT, Naoto Sato wrote: > Upgrading the GB18030 charset in the JDK to the latest 2022 standard. Since > this is not a compatible upgrade to the existing mapping, a new system > property `jdk.charset.GB18030` is introduced. If it is set to "2000", the > mapping falls back to the existing mapping based on the 2000 standard, > otherwise, it defaults to 2022 mapping. Refer to the corresponding CSR for > more detail. src/java.base/share/classes/sun/nio/cs/StandardCharsets.java.template line 211: > 209: > 210: // Lazily initialized system property value > 211: private static String GB18030_2000; I assume this should be a stable field. - PR: https://git.openjdk.org/jdk/pull/12518
Integrated: 8302815 Use new Math.clamp method in core libraries
On Sat, 18 Feb 2023 10:50:53 GMT, Tagir F. Valeev wrote: > For cleanup and dogfooding the new method, it would be nice to use Math.clamp > where possible in java.base. See PR #12428. > > As Math.clamp performs an additional check that min is not greater than max, > I conservatively replaced only those occurrences where I can see that this > invariant is always held. There are more occurrences, where clamp can be > potentially used but it's unclear whether min <= max is always true. This pull request has now been integrated. Changeset: 3f3a1f53 Author:Tagir F. Valeev URL: https://git.openjdk.org/jdk/commit/3f3a1f534b7f2f5be6d7ded9d9832fa9394e763c Stats: 45 lines in 11 files changed: 0 ins; 8 del; 37 mod 8302815: Use new Math.clamp method in core libraries Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/12633
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 05:21:48 GMT, Joe Darcy wrote: > That said, I think it is reasonable on a given JVM invocation if > Float.floatToFloat16(f) gave the same result for input f regardless of in > what context it was called. Yes, I'm under the impression that for math API methods like this, the stability of input to output must be preserved for a single JVM invocation. Or are there existing methods for which the interpreter and compiled code execution is allowed to differ? - PR: https://git.openjdk.org/jdk/pull/12704
