Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v2]
> Hi All, > > Patch adds the planned support for new vector operations and APIs targeted > for [JEP 426: Vector API (Fourth > Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173) > > Following is the brief summary of changes:- > > 1) Extends the scope of existing lanewise API for following new vector > operations. >- VectorOperations.BIT_COUNT: counts the number of one-bits >- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero > bits >- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing > zero bits >- VectorOperations.REVERSE: reversing the order of bits >- VectorOperations.REVERSE_BYTES: reversing the order of bytes >- compress and expand bits: Semantics are based on Hacker's Delight > section 7-4 Compress, or Generalized Extract. > > 2) Adds following new APIs to perform cross lane vector compress and > expansion operations under the influence of a mask. >- Vector.compress >- Vector.expand >- VectorMask.compress > > 3) Adds predicated and non-predicated versions of following new APIs to load > and store the contents of vector from foreign MemorySegments. > - Vector.fromMemorySegment > - Vector.intoMemorySegment > > 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support > for each newly added operation. > > > Patch has been regressed over AARCH64 and X86 targets different AVX levels. > > Kindly review and share your feedback. > > Best Regards, > Jatin Jatin Bhateja has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - 8284960: Correcting a typo. - 8284960: Integrating changes from panama-vector (Add @since 19 tags). - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - 8284960: AARCH64 backend changes. - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 - 8284960: Integration of JEP 426: Vector API (Fourth Incubator) - Changes: https://git.openjdk.java.net/jdk/pull/8425/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8425=01 Stats: 37900 lines in 214 files changed: 16527 ins; 16923 del; 4450 mod Patch: https://git.openjdk.java.net/jdk/pull/8425.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8425/head:pull/8425 PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8284050: [vectorapi] Optimize masked store for non-predicated architectures [v2]
On Thu, 5 May 2022 03:17:35 GMT, Xiaohong Gong wrote: >> src/hotspot/share/opto/vectorIntrinsics.cpp line 1363: >> >>> 1361: // Use the vector blend to implement the masked store. The >>> biased elements are the original >>> 1362: // values in the memory. >>> 1363: Node* mem_val = gvn().transform(LoadVectorNode::make(0, >>> control(), memory(addr), addr, addr_type, mem_num_elem, mem_elem_bt)); >> >> I'm sorry to say it, but I am pretty sure this is an invalid optimization. >> See top-level comment for more details. > > Thanks for your comments! Yeah, this actually influences something due to the > Java Memory Model rules which I missed to consider more. I will try the > scatter ways instead. Thanks so much! Yes, phantom store can write back stale unintended value and may create problem in multithreded applications since blending is done with an older loaded value. - PR: https://git.openjdk.java.net/jdk/pull/8544
Re: RFR: 8284050: [vectorapi] Optimize masked store for non-predicated architectures [v2]
On Thu, 5 May 2022 02:09:39 GMT, Xiaohong Gong wrote:
>> Currently the vectorization of masked vector store is implemented by the
>> masked store instruction only on architectures that support the predicate
>> feature. The compiler will fall back to the java scalar code for
>> non-predicate supported architectures like ARM NEON. However, for these
>> systems, the masked store can be vectorized with the non-masked vector
>> `"load + blend + store"`. For example, storing a vector` "v"` controlled by
>> a mask` "m"` into a memory with address` "addr" (i.e. "store(addr, v, m)")`
>> can be implemented with:
>>
>>
>> 1) mem_v = load(addr) ; non-masked load from the same memory
>> 2) v = blend(mem_v, v, m) ; blend with the src vector with the mask
>> 3) store(addr, v) ; non-masked store into the memory
>>
>>
>> Since the first full loading needs the array offset must be inside of the
>> valid array bounds, we make the compiler do the vectorization only when the
>> offset is in range of the array boundary. And the compiler will still fall
>> back to the java scalar code if not all offsets are valid. Besides, the
>> original offset check for masked lanes are only applied when the offset is
>> not always inside of the array range. This also improves the performance for
>> masked store when the offset is always valid. The whole process is similar
>> to the masked load API.
>>
>> Here is the performance data for the masked vector store benchmarks on a X86
>> non avx-512 system, which improves about `20x ~ 50x`:
>>
>> Benchmark beforeafter Units
>> StoreMaskedBenchmark.byteStoreArrayMask 221.733 11094.126 ops/ms
>> StoreMaskedBenchmark.doubleStoreArrayMask 41.086 1034.408 ops/ms
>> StoreMaskedBenchmark.floatStoreArrayMask 73.820 1985.015 ops/ms
>> StoreMaskedBenchmark.intStoreArrayMask 75.028 2027.557 ops/ms
>> StoreMaskedBenchmark.longStoreArrayMask40.929 1032.928 ops/ms
>> StoreMaskedBenchmark.shortStoreArrayMask 135.794 5307.567 ops/ms
>>
>> Similar performance gain can also be observed on ARM NEON system.
>>
>> And here is the performance data on X86 avx-512 system, which improves about
>> `1.88x - 2.81x`:
>>
>> Benchmark before after Units
>> StoreMaskedBenchmark.byteStoreArrayMask 11185.956 21012.824 ops/ms
>> StoreMaskedBenchmark.doubleStoreArrayMask 1480.644 3911.720 ops/ms
>> StoreMaskedBenchmark.floatStoreArrayMask 2738.352 7708.365 ops/ms
>> StoreMaskedBenchmark.intStoreArrayMask 4191.904 9300.428 ops/ms
>> StoreMaskedBenchmark.longStoreArrayMask2025.031 4604.504 ops/ms
>> StoreMaskedBenchmark.shortStoreArrayMask 8339.389 17817.128 ops/ms
>>
>> Similar performance gain can also be observed on ARM SVE system.
>
> Xiaohong Gong has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains one commit:
>
> 8284050: [vectorapi] Optimize masked store for non-predicated architectures
> _Mailing list message from [Hans Boehm](mailto:hbo...@google.com) on
> [hotspot-dev](mailto:hotspot-...@mail.openjdk.java.net):_
>
> Naive question: What happens if one of the vector elements that should not
> have been updated is concurrently being written by another thread? Aren't you
> generating writes to vector elements that should not have been written?
>
> Hans
>
> On Wed, May 4, 2022 at 7:08 PM Xiaohong Gong
> wrote:
Yeah, this is the similar concern with what @rose00 mentioned above. The
current solution cannot work well for multi-thread progresses. I will consider
other better solutions. Thanks for the comments!
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java
line 3483:
> 3481: ByteSpecies vsp = vspecies();
> 3482: if (offset >= 0 && offset <= (a.length - vsp.length())) {
> 3483: intoBooleanArray0(a, offset, m, /* offsetInRange */
> true);
The offset check could save the `checkMaskFromIndexSize` for cases that offset
are in the valid array bounds, which also improves the performance. @rose00 ,
do you think this part of change is ok at least? Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/8544
Re: RFR: 8284050: [vectorapi] Optimize masked store for non-predicated architectures [v2]
On Thu, 5 May 2022 02:27:03 GMT, John R Rose wrote: >> Xiaohong Gong has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains one commit: >> >> 8284050: [vectorapi] Optimize masked store for non-predicated architectures > > src/hotspot/share/opto/vectorIntrinsics.cpp line 1363: > >> 1361: // Use the vector blend to implement the masked store. The >> biased elements are the original >> 1362: // values in the memory. >> 1363: Node* mem_val = gvn().transform(LoadVectorNode::make(0, >> control(), memory(addr), addr, addr_type, mem_num_elem, mem_elem_bt)); > > I'm sorry to say it, but I am pretty sure this is an invalid optimization. > See top-level comment for more details. Thanks for your comments! Yeah, this actually influences something due to the Java Memory Model rules which I missed to consider more. I will try the scatter ways instead. Thanks so much! - PR: https://git.openjdk.java.net/jdk/pull/8544
Re: RFR: 8284050: [vectorapi] Optimize masked store for non-predicated architectures [v2]
On Thu, 5 May 2022 02:09:39 GMT, Xiaohong Gong wrote: >> Currently the vectorization of masked vector store is implemented by the >> masked store instruction only on architectures that support the predicate >> feature. The compiler will fall back to the java scalar code for >> non-predicate supported architectures like ARM NEON. However, for these >> systems, the masked store can be vectorized with the non-masked vector >> `"load + blend + store"`. For example, storing a vector` "v"` controlled by >> a mask` "m"` into a memory with address` "addr" (i.e. "store(addr, v, m)")` >> can be implemented with: >> >> >> 1) mem_v = load(addr) ; non-masked load from the same memory >> 2) v = blend(mem_v, v, m) ; blend with the src vector with the mask >> 3) store(addr, v) ; non-masked store into the memory >> >> >> Since the first full loading needs the array offset must be inside of the >> valid array bounds, we make the compiler do the vectorization only when the >> offset is in range of the array boundary. And the compiler will still fall >> back to the java scalar code if not all offsets are valid. Besides, the >> original offset check for masked lanes are only applied when the offset is >> not always inside of the array range. This also improves the performance for >> masked store when the offset is always valid. The whole process is similar >> to the masked load API. >> >> Here is the performance data for the masked vector store benchmarks on a X86 >> non avx-512 system, which improves about `20x ~ 50x`: >> >> Benchmark beforeafter Units >> StoreMaskedBenchmark.byteStoreArrayMask 221.733 11094.126 ops/ms >> StoreMaskedBenchmark.doubleStoreArrayMask 41.086 1034.408 ops/ms >> StoreMaskedBenchmark.floatStoreArrayMask 73.820 1985.015 ops/ms >> StoreMaskedBenchmark.intStoreArrayMask 75.028 2027.557 ops/ms >> StoreMaskedBenchmark.longStoreArrayMask40.929 1032.928 ops/ms >> StoreMaskedBenchmark.shortStoreArrayMask 135.794 5307.567 ops/ms >> >> Similar performance gain can also be observed on ARM NEON system. >> >> And here is the performance data on X86 avx-512 system, which improves about >> `1.88x - 2.81x`: >> >> Benchmark before after Units >> StoreMaskedBenchmark.byteStoreArrayMask 11185.956 21012.824 ops/ms >> StoreMaskedBenchmark.doubleStoreArrayMask 1480.644 3911.720 ops/ms >> StoreMaskedBenchmark.floatStoreArrayMask 2738.352 7708.365 ops/ms >> StoreMaskedBenchmark.intStoreArrayMask 4191.904 9300.428 ops/ms >> StoreMaskedBenchmark.longStoreArrayMask2025.031 4604.504 ops/ms >> StoreMaskedBenchmark.shortStoreArrayMask 8339.389 17817.128 ops/ms >> >> Similar performance gain can also be observed on ARM SVE system. > > Xiaohong Gong has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > 8284050: [vectorapi] Optimize masked store for non-predicated architectures The JIT (in all other circumstances AFAIK) never produces "phantom stores", stores into Java variables which are not specified as the target of a JVM store instruction (putfield, dastore, etc.). The fact that a previously-read value is used by the phantom store does not make it any better. Yes, the memory states may be correct after the blend and store is done, but the effect on the Java Memory Model is to issue the extra phantom stores of the unselected array elements. Under certain circumstances, this will create race conditions after the optimization where there were no race conditions before the optimization. Other threads could (under Java Memory Model rules) witness the effects of the phantom stores. If the Java program is properly synchronized, the introduction of an illegitimate race condition can cause another thread, now in an illegal race, to see an old value in a variable (the recopied unselected array element) which the JMM says is impossible. Yes, this only shows up in multi-threaded programs, and ones where two threads step on one array, but Java is a multi-threaded language, and it must conform to its own specification as such. This blend technique would be very reasonable if there is no race condition. (Except at the very beginning or end of arrays.) And the JMM leaves room for many optimizations. And yet I think this is a step too far. I'd like to be wrong about this, but I don't think I am. So, I think you need to use a different technique, other than blend-and-unpredicated-store, for masked stores on non-predicated architectures. For example, you could simulate a masked store instruction on an architecture that supports scatter (scattering values of the array type). Do this by setting up two vectors of machine pointers. One vector points to each potentially-affected element of the array (some kind of index computation plus a scaled iota vector). The other vector
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v11]
On Wed, 4 May 2022 12:12:48 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. We >> can add additional labels, if required, as the PR progresses. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - Refresh cf561073f48fad58e931a5285b92629aa83c9bd1 > - Merge with jdk-19+19 > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/4b2c8220...f06aff75 I've completed review of new vthread-related tests in the folder serviceability/jvmti. It includes sub-folders: serviceability/jvmti/events serviceability/jvmti/negative serviceability/jvmti/thread Thank you, Leonid, for resolving my comments! - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Thu, 31 Mar 2022 02:15:26 GMT, Quan Anh Mai wrote: >> I'm afraid not. "Load + Blend" makes the elements of unmasked lanes to be >> `0`. Then a full store may change the values in the unmasked memory to be 0, >> which is different with the mask store API definition. > > The blend should be with the intended-to-store vector, so that masked lanes > contain the need-to-store elements and unmasked lanes contain the loaded > elements, which would be stored back, which results in unchanged values. Hi @merykitty @jatin-bhateja , could you please help to take a review at the similar store masked PR https://github.com/openjdk/jdk/pull/8544 ? Any feedback is welcome! Thanks so much! - PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Thu, 28 Apr 2022 00:13:49 GMT, Sandhya Viswanathan
wrote:
>> Xiaohong Gong has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rename the "usePred" to "offsetInRange"
>
> src/hotspot/share/opto/vectorIntrinsics.cpp line 1232:
>
>> 1230: // out when current case uses the predicate feature.
>> 1231: if (!supports_predicate) {
>> 1232: bool use_predicate = false;
>
> If we rename this to needs_predicate it will be easier to understand.
Thanks for the comment! This local variable will be removed after adding the
similar intrinsify for store masked. Please help to see the PR
https://github.com/openjdk/jdk/pull/8544. Thanks so much!
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8284050: [vectorapi] Optimize masked store for non-predicated architectures [v2]
> Currently the vectorization of masked vector store is implemented by the > masked store instruction only on architectures that support the predicate > feature. The compiler will fall back to the java scalar code for > non-predicate supported architectures like ARM NEON. However, for these > systems, the masked store can be vectorized with the non-masked vector `"load > + blend + store"`. For example, storing a vector` "v"` controlled by a mask` > "m"` into a memory with address` "addr" (i.e. "store(addr, v, m)")` can be > implemented with: > > > 1) mem_v = load(addr) ; non-masked load from the same memory > 2) v = blend(mem_v, v, m) ; blend with the src vector with the mask > 3) store(addr, v) ; non-masked store into the memory > > > Since the first full loading needs the array offset must be inside of the > valid array bounds, we make the compiler do the vectorization only when the > offset is in range of the array boundary. And the compiler will still fall > back to the java scalar code if not all offsets are valid. Besides, the > original offset check for masked lanes are only applied when the offset is > not always inside of the array range. This also improves the performance for > masked store when the offset is always valid. The whole process is similar to > the masked load API. > > Here is the performance data for the masked vector store benchmarks on a X86 > non avx-512 system, which improves about `20x ~ 50x`: > > Benchmark beforeafter Units > StoreMaskedBenchmark.byteStoreArrayMask 221.733 11094.126 ops/ms > StoreMaskedBenchmark.doubleStoreArrayMask 41.086 1034.408 ops/ms > StoreMaskedBenchmark.floatStoreArrayMask 73.820 1985.015 ops/ms > StoreMaskedBenchmark.intStoreArrayMask 75.028 2027.557 ops/ms > StoreMaskedBenchmark.longStoreArrayMask40.929 1032.928 ops/ms > StoreMaskedBenchmark.shortStoreArrayMask 135.794 5307.567 ops/ms > > Similar performance gain can also be observed on ARM NEON system. > > And here is the performance data on X86 avx-512 system, which improves about > `1.88x - 2.81x`: > > Benchmark before after Units > StoreMaskedBenchmark.byteStoreArrayMask 11185.956 21012.824 ops/ms > StoreMaskedBenchmark.doubleStoreArrayMask 1480.644 3911.720 ops/ms > StoreMaskedBenchmark.floatStoreArrayMask 2738.352 7708.365 ops/ms > StoreMaskedBenchmark.intStoreArrayMask 4191.904 9300.428 ops/ms > StoreMaskedBenchmark.longStoreArrayMask2025.031 4604.504 ops/ms > StoreMaskedBenchmark.shortStoreArrayMask 8339.389 17817.128 ops/ms > > Similar performance gain can also be observed on ARM SVE system. Xiaohong Gong has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: 8284050: [vectorapi] Optimize masked store for non-predicated architectures - Changes: https://git.openjdk.java.net/jdk/pull/8544/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8544=01 Stats: 915 lines in 43 files changed: 405 ins; 80 del; 430 mod Patch: https://git.openjdk.java.net/jdk/pull/8544.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8544/head:pull/8544 PR: https://git.openjdk.java.net/jdk/pull/8544
RFR: 8284050: [vectorapi] Optimize masked store for non-predicated architectures
Currently the vectorization of masked vector store is implemented by the masked store instruction only on architectures that support the predicate feature. The compiler will fall back to the java scalar code for non-predicate supported architectures like ARM NEON. However, for these systems, the masked store can be vectorized with the non-masked vector `"load + blend + store"`. For example, storing a vector` "v"` controlled by a mask` "m"` into a memory with address` "addr" (i.e. "store(addr, v, m)")` can be implemented with: 1) mem_v = load(addr) ; non-masked load from the same memory 2) v = blend(mem_v, v, m) ; blend with the src vector with the mask 3) store(addr, v) ; non-masked store into the memory Since the first full loading needs the array offset must be inside of the valid array bounds, we make the compiler do the vectorization only when the offset is in range of the array boundary. And the compiler will still fall back to the java scalar code if not all offsets are valid. Besides, the original offset check for masked lanes are only applied when the offset is not always inside of the array range. This also improves the performance for masked store when the offset is always valid. The whole process is similar to the masked load API. Here is the performance data for the masked vector store benchmarks on a X86 non avx-512 system, which improves about `20x ~ 50x`: Benchmark beforeafter Units StoreMaskedBenchmark.byteStoreArrayMask 221.733 11094.126 ops/ms StoreMaskedBenchmark.doubleStoreArrayMask 41.086 1034.408 ops/ms StoreMaskedBenchmark.floatStoreArrayMask 73.820 1985.015 ops/ms StoreMaskedBenchmark.intStoreArrayMask 75.028 2027.557 ops/ms StoreMaskedBenchmark.longStoreArrayMask40.929 1032.928 ops/ms StoreMaskedBenchmark.shortStoreArrayMask 135.794 5307.567 ops/ms Similar performance gain can also be observed on ARM NEON system. And here is the performance data on X86 avx-512 system, which improves about `1.88x - 2.81x`: Benchmark before after Units StoreMaskedBenchmark.byteStoreArrayMask 11185.956 21012.824 ops/ms StoreMaskedBenchmark.doubleStoreArrayMask 1480.644 3911.720 ops/ms StoreMaskedBenchmark.floatStoreArrayMask 2738.352 7708.365 ops/ms StoreMaskedBenchmark.intStoreArrayMask 4191.904 9300.428 ops/ms StoreMaskedBenchmark.longStoreArrayMask2025.031 4604.504 ops/ms StoreMaskedBenchmark.shortStoreArrayMask 8339.389 17817.128 ops/ms Similar performance gain can also be observed on ARM SVE system. - Depends on: https://git.openjdk.java.net/jdk/pull/8035 Commit messages: - 8284050: [vectorapi] Optimize masked store for non-predicated architectures - 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature Changes: https://git.openjdk.java.net/jdk/pull/8544/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8544=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284050 Stats: 1708 lines in 44 files changed: 710 ins; 188 del; 810 mod Patch: https://git.openjdk.java.net/jdk/pull/8544.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8544/head:pull/8544 PR: https://git.openjdk.java.net/jdk/pull/8544
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Thu, 5 May 2022 01:42:48 GMT, Xiaohong Gong wrote:
> > > Yeah, I agree that it's not good by adding a branch checking for
> > > `offsetInRange`. But actually I met the constant issue that passing the
> > > values all the way cannot guarantee the argument a constant in compiler
> > > at the compile time. Do you have any better idea to fixing this?
> >
> >
> > That's odd, `boolean` constants are passed that are then converted to `int`
> > constants. Did you try passing integer constants all the way through?
>
> I will try again. I remember the main cause is the calling of `fromArray0`
> from `fromArray`, it is not annotated with `ForceInline`. The arguments might
> not be compiled to a constant for cases that the offset is not in the array
> range like tail loop.
I tried to pass the integer constant all the way, and unfortunate that the
`offsetInRange` is not compiled to a constant. The following assertion in the
`vectorIntrinsics.cpp` will fail:
--- a/src/hotspot/share/opto/vectorIntrinsics.cpp
+++ b/src/hotspot/share/opto/vectorIntrinsics.cpp
@@ -1236,6 +1236,7 @@ bool
LibraryCallKit::inline_vector_mem_masked_operation(bool is_store) {
} else {
// Masked vector load with IOOBE always uses the predicated load.
const TypeInt* offset_in_range = gvn().type(argument(8))->isa_int();
+ assert(offset_in_range->is_con(), "not a constant");
if (!offset_in_range->is_con()) {
if (C->print_intrinsics()) {
tty->print_cr(" ** missing constant: offsetInRange=%s",
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Thu, 5 May 2022 01:21:40 GMT, Paul Sandoz wrote: > > Yeah, I agree that it's not good by adding a branch checking for > > `offsetInRange`. But actually I met the constant issue that passing the > > values all the way cannot guarantee the argument a constant in compiler at > > the compile time. Do you have any better idea to fixing this? > > That's odd, `boolean` constants are passed that are then converted to `int` > constants. Did you try passing integer constants all the way through? I will try again. I remember the main cause is the calling of `fromArray0` from `fromArray`, it is not annotated with `ForceInline`. The arguments might not be compiled to a constant for cases that the offset is not in the array range like tail loop. - PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v4]
On Wed, 4 May 2022 17:09:32 GMT, Ichiroh Takiguchi
wrote:
>> On JDK19 with Linux ja_JP.eucjp locale,
>> System.getenv() returns unexpected value if environment variable has
>> Japanese EUC characters.
>> It seems this issue happens because of JEP 400.
>> Arguments for ProcessBuilder have same kind of issue.
>
> Ichiroh Takiguchi has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8285517: System.getenv() returns unexpected value if environment variable
> has non ASCII character
src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 73:
> 71: @SuppressWarnings("removal")
> 72: String jnuEncoding =
> AccessController.doPrivileged((PrivilegedAction) ()
> 73: -> System.getProperty("sun.jnu.encoding"));
No need to issue `doPrivileged()`, which is deprecated
src/java.base/unix/classes/java/lang/ProcessImpl.java line 149:
> 147: @SuppressWarnings("removal")
> 148: String jnuEncoding =
> AccessController.doPrivileged((PrivilegedAction) ()
> 149: -> System.getProperty("sun.jnu.encoding"));
Same here.
test/jdk/java/lang/System/i18nEnvArg.java line 70:
> 68: Map environ = pb.environment();
> 69: environ.clear();
> 70: environ.put("LANG", "ja_JP.eucjp");
There are many duplicate pieces of code here and in the `else` block below. Can
you simplify this `if` statement more?
test/jdk/java/lang/System/i18nEnvArg.java line 110:
> 108: String s = System.getenv(EUC_JP_TEXT);
> 109: ByteArrayOutputStream baos = new ByteArrayOutputStream();
> 110: PrintStream ps = new PrintStream(baos);
Can utilize try-with-resources pattern.
-
PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Thu, 5 May 2022 01:13:23 GMT, Xiaohong Gong wrote: > Yeah, I agree that it's not good by adding a branch checking for > `offsetInRange`. But actually I met the constant issue that passing the > values all the way cannot guarantee the argument a constant in compiler at > the compile time. Do you have any better idea to fixing this? That's odd, `boolean` constants are passed that are then converted to `int` constants. Did you try passing integer constants all the way through? - PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature [v2]
On Fri, 29 Apr 2022 21:34:13 GMT, Paul Sandoz wrote: >> Xiaohong Gong has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Rename the "usePred" to "offsetInRange" > > IIUC when the hardware does not support predicated loads then any false > `offsetIntRange` value causes the load intrinsic to fail resulting in the > fallback, so it would not be materially any different to the current > behavior, just more uniformly implemented. > > Why can't the intrinsic support the passing a boolean directly? Is it > something to do with constants? If that is not possible I recommend creating > named constant values and pass those all the way through rather than > converting a boolean to an integer value. Then there is no need for a branch > checking `offsetInRange`. > > Might be better to hold off until the JEP is integrated and then update, > since this will conflict (`byte[]` and `ByteBuffer` load methods are removed > and `MemorySegment` load methods are added). You could prepare for that now > by branching off `vectorIntrinsics`. Thanks for your comments @PaulSandoz ! > IIUC when the hardware does not support predicated loads then any false > `offsetIntRange` value causes the load intrinsic to fail resulting in the > fallback, so it would not be materially any different to the current > behavior, just more uniformly implemented. Yes, it's true that this patch doesn't have any different to the hardware that does not support the predicated loads. It only benefits the predicated feature supported systems like ARM SVE and X86 AVX-512. > Why can't the intrinsic support the passing a boolean directly? Is it > something to do with constants? If that is not possible I recommend creating > named constant values and pass those all the way through rather than > converting a boolean to an integer value. Then there is no need for a branch > checking offsetInRange. Yeah, I agree that it's not good by adding a branch checking for `offsetInRange`. But actually I met the constant issue that passing the values all the way cannot guarantee the argument a constant in compiler at the compile time. Do you have any better idea to fixing this? > Might be better to hold off until the JEP is integrated and then update, > since this will conflict (byte[] and ByteBuffer load methods are removed and > MemorySegment load methods are added). You could prepare for that now by > branching off vectorIntrinsics. Agree. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:29:53 GMT, Maurizio Cimadamore wrote: >> Good points. Regarding `ClassLoader` being null, I think we can still return >> something using the `BootLoader`'s `NativeLibraries` object - that would >> allow this method to be called internally. @mlchung Can you please confirm? >> >> As for the caller sensitive having no real caller (e.g. because method is >> called directly from native code), I think we should add a check in >> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such >> a check does not compromise upcall stub created via the Linker interface >> (e.g. a Java upcall might require to perform restricted operations - but >> those operation always happen inside the upcall MH, which is always >> associated with some class). > > Another option would be to treat calls to `ensureNativeAccess` with `null` > caller class as coming from unnamed module. Looking deeper, `System::loadLibrary` seems to search the boot loader libraries when invoked with a `null` caller class, so replicating that behavior should be safe. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:20:21 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>>
>>> 151: static SymbolLookup loaderLookup() {
>>> 152: Class caller = Reflection.getCallerClass();
>>> 153: ClassLoader loader =
>>> Objects.requireNonNull(caller.getClassLoader());
>>
>> Shouldn’t this be changed to throw `IllegalCallerException` instead of
>> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
>> `null`[^1] or when `caller` itself is `null`[^2]?
>>
>> [^1]: This happens when `caller` is on the bootstrap classpath.
>> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
>> code and no **Java** code is on the call stack.
>
> Good points. Regarding `ClassLoader` being null, I think we can still return
> something using the `BootLoader`'s `NativeLibraries` object - that would
> allow this method to be called internally. @mlchung Can you please confirm?
>
> As for the caller sensitive having no real caller (e.g. because method is
> called directly from native code), I think we should add a check in
> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
> check does not compromise upcall stub created via the Linker interface (e.g.
> a Java upcall might require to perform restricted operations - but those
> operation always happen inside the upcall MH, which is always associated with
> some class).
Another option would be to treat calls to `ensureNativeAccess` with `null`
caller class as coming from unnamed module.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne
On Wed, 4 May 2022 22:27:48 GMT, Paul Sandoz wrote: > The changes to `Float` and `Double` look good. I don't think we need > additional tests, see test/jdk/java/lang/Math/IeeeRecommendedTests.java. Thank you, Paul for pointing the test. It means we need to run tier4 (which runs these tests with -Xcomp) to make sure methods are compiled by C2. - PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v38]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak examples in SymbolLookup javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/41d055ab..853f06b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=37 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=36-37 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 16:47:28 GMT, ExE Boss wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 57 commits:
>>
>> - Merge branch 'master' into foreign-preview
>> - Update
>> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - Tweak support for Linker lookup
>>Improve javadoc of SymbolLookup
>> - Tweak Addressable javadoc
>> - Downcall and upcall tests use wrong layout which is missing some trailing
>> padding
>> - Simplify libraryLookup impl
>> - Address CSR comments
>> - Merge branch 'master' into foreign-preview
>> - Add ValueLayout changes
>> - Tweak support for array element var handle
>> - ... and 47 more:
>> https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab
>
> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>
>> 151: static SymbolLookup loaderLookup() {
>> 152: Class caller = Reflection.getCallerClass();
>> 153: ClassLoader loader =
>> Objects.requireNonNull(caller.getClassLoader());
>
> Shouldn’t this be changed to throw `IllegalCallerException` instead of
> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
> `null`[^1] or when `caller` itself is `null`[^2]?
>
> [^1]: This happens when `caller` is on the bootstrap classpath.
> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
> code and no **Java** code is on the call stack.
Good points. Regarding `ClassLoader` being null, I think we can still return
something using the `BootLoader`'s `NativeLibraries` object - that would allow
this method to be called internally. @mlchung Can you please confirm?
As for the caller sensitive having no real caller (e.g. because method is
called directly from native code), I think we should add a check in
`Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
check does not compromise upcall stub created via the Linker interface (e.g. a
Java upcall might require to perform restricted operations - but those
operation always happen inside the upcall MH, which is always associated with
some class).
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne
On Wed, 4 May 2022 19:32:41 GMT, Vladimir Kozlov wrote:
>> Hi,
>>
>> This patch optimises the matching rules for floating-point comparison with
>> respects to eq/ne on x86-64
>>
>> 1, When the inputs of a comparison is the same (i.e `isNaN` patterns), `ZF`
>> is always set, so we don't need `cmpOpUCF2` for the eq/ne cases, which
>> improves the sequence of `If (CmpF x x) (Bool ne)` from
>>
>> ucomiss xmm0, xmm0
>> jp label
>> jne label
>>
>> into
>>
>> ucomiss xmm0, xmm0
>> jp label
>>
>> 2, The move rules for `cmpOpUCF2` is missing, which makes patterns such as
>> `x == y ? 1 : 0` to fall back to `cmpOpU`, which have a really high cost of
>> fixing the flags, such as
>>
>> xorlecx, ecx
>> ucomiss xmm0, xmm1
>> jnp done
>> pushf
>> andq[rsp], 0xff2b
>> popf
>> done:
>> movleax, 1
>> cmovel eax, ecx
>>
>> The patch changes this sequence into
>>
>> xorlecx, ecx
>> ucomiss xmm0, xmm1
>> movleax, 1
>> cmovpl eax, ecx
>> cmovnel eax, ecx
>>
>> 3, The patch also changes the pattern of `isInfinite` to be more optimised
>> by using `Math.abs` to reduce 1 comparison and compares the result with
>> `MAX_VALUE` since `>` is more optimised than `==` for floating-point types.
>>
>> The benchmark results are as follow:
>>
>> Before:
>> Benchmark Mode Cnt Score Error Units
>> FPComparison.equalDouble avgt5 2876.242 ± 58.875 ns/op
>> FPComparison.equalFloatavgt5 3062.430 ± 31.371 ns/op
>> FPComparison.isFiniteDoubleavgt5 475.749 ± 19.027 ns/op
>> FPComparison.isFiniteFloat avgt5 506.525 ± 14.417 ns/op
>> FPComparison.isInfiniteDouble avgt5 1232.800 ± 31.677 ns/op
>> FPComparison.isInfiniteFloat avgt5 1234.708 ± 70.239 ns/op
>> FPComparison.isNanDouble avgt5 2255.847 ± 7.238 ns/op
>> FPComparison.isNanFloatavgt5 2567.044 ± 36.078 ns/op
>>
>> After:
>> Benchmark Mode Cnt Score Error Units
>> FPComparison.equalDouble avgt5 594.636 ± 8.922 ns/op
>> FPComparison.equalFloatavgt5 663.849 ± 3.656 ns/op
>> FPComparison.isFiniteDoubleavgt5 518.309 ± 107.352 ns/op
>> FPComparison.isFiniteFloat avgt5 515.576 ± 14.669 ns/op
>> FPComparison.isInfiniteDouble avgt5 621.185 ± 11.935 ns/op
>> FPComparison.isInfiniteFloat avgt5 623.566 ± 15.206 ns/op
>> FPComparison.isNanDouble avgt5 400.124 ± 0.762 ns/op
>> FPComparison.isNanFloatavgt5 546.486 ± 1.509 ns/op
>>
>> Thank you very much.
>
> src/hotspot/cpu/x86/x86_64.ad line 6998:
>
>> 6996: ins_encode %{
>> 6997: __ cmovl(Assembler::parity, $dst$$Register, $src$$Register);
>> 6998: __ cmovl(Assembler::notEqual, $dst$$Register, $src$$Register);
>
> Should this be `equal`?
I see that you swapped `src, dst` in `match()` but `format` is sill incorrect
and the code is confusing.
-
PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v6]
On Mon, 2 May 2022 20:31:18 GMT, Joe Darcy wrote: >> Add a new system property, java.specification.maintenance.version, to return >> the maintenance release number of the Java SE specification being >> implemented. The property is unset, optional in the terminology of >> System.getProperties, for an initial release of a specification. >> >> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 >> >> I'll update copyright years before an integration. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 10 additional commits since > the last revision: > > - Update wording to address review feedback. > - Merge branch 'master' into JDK-8285497 > - Change punctuation from review feedback. > - Respond to review feedback; make sequence of values explicit. > - Respond to review feedback. > - Respond to review feedback. > - Respond to CSR feedback. > - Merge branch 'master' into JDK-8285497 > - Update comment in template. > - JDK-8285497: Add system property for Java SE specification maintenance > version PR and CSR updated as suggested. - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v7]
> Add a new system property, java.specification.maintenance.version, to return > the maintenance release number of the Java SE specification being > implemented. The property is unset, optional in the terminology of > System.getProperties, for an initial release of a specification. > > Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 > > I'll update copyright years before an integration. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Respond to mbreinhold review feedback. - Merge branch 'master' into JDK-8285497 - Update wording to address review feedback. - Merge branch 'master' into JDK-8285497 - Change punctuation from review feedback. - Respond to review feedback; make sequence of values explicit. - Respond to review feedback. - Respond to review feedback. - Respond to CSR feedback. - Merge branch 'master' into JDK-8285497 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/f4e6f016...7b7720cf - Changes: - all: https://git.openjdk.java.net/jdk/pull/8437/files - new: https://git.openjdk.java.net/jdk/pull/8437/files/741ececa..7b7720cf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8437=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8437=05-06 Stats: 7420 lines in 158 files changed: 4834 ins; 1036 del; 1550 mod Patch: https://git.openjdk.java.net/jdk/pull/8437.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8437/head:pull/8437 PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne
On Wed, 4 May 2022 01:59:17 GMT, Quan Anh Mai wrote: > Hi, > > This patch optimises the matching rules for floating-point comparison with > respects to eq/ne on x86-64 > > 1, When the inputs of a comparison is the same (i.e `isNaN` patterns), `ZF` > is always set, so we don't need `cmpOpUCF2` for the eq/ne cases, which > improves the sequence of `If (CmpF x x) (Bool ne)` from > > ucomiss xmm0, xmm0 > jp label > jne label > > into > > ucomiss xmm0, xmm0 > jp label > > 2, The move rules for `cmpOpUCF2` is missing, which makes patterns such as `x > == y ? 1 : 0` to fall back to `cmpOpU`, which have a really high cost of > fixing the flags, such as > > xorlecx, ecx > ucomiss xmm0, xmm1 > jnp done > pushf > andq[rsp], 0xff2b > popf > done: > movleax, 1 > cmovel eax, ecx > > The patch changes this sequence into > > xorlecx, ecx > ucomiss xmm0, xmm1 > movleax, 1 > cmovpl eax, ecx > cmovnel eax, ecx > > 3, The patch also changes the pattern of `isInfinite` to be more optimised by > using `Math.abs` to reduce 1 comparison and compares the result with > `MAX_VALUE` since `>` is more optimised than `==` for floating-point types. > > The benchmark results are as follow: > > Before: > Benchmark Mode Cnt Score Error Units > FPComparison.equalDouble avgt5 2876.242 ± 58.875 ns/op > FPComparison.equalFloatavgt5 3062.430 ± 31.371 ns/op > FPComparison.isFiniteDoubleavgt5 475.749 ± 19.027 ns/op > FPComparison.isFiniteFloat avgt5 506.525 ± 14.417 ns/op > FPComparison.isInfiniteDouble avgt5 1232.800 ± 31.677 ns/op > FPComparison.isInfiniteFloat avgt5 1234.708 ± 70.239 ns/op > FPComparison.isNanDouble avgt5 2255.847 ± 7.238 ns/op > FPComparison.isNanFloatavgt5 2567.044 ± 36.078 ns/op > > After: > Benchmark Mode Cnt Score Error Units > FPComparison.equalDouble avgt5 594.636 ± 8.922 ns/op > FPComparison.equalFloatavgt5 663.849 ± 3.656 ns/op > FPComparison.isFiniteDoubleavgt5 518.309 ± 107.352 ns/op > FPComparison.isFiniteFloat avgt5 515.576 ± 14.669 ns/op > FPComparison.isInfiniteDouble avgt5 621.185 ± 11.935 ns/op > FPComparison.isInfiniteFloat avgt5 623.566 ± 15.206 ns/op > FPComparison.isNanDouble avgt5 400.124 ± 0.762 ns/op > FPComparison.isNanFloatavgt5 546.486 ± 1.509 ns/op > > Thank you very much. The changes to `Float` and `Double` look good. I don't think we need additional tests, see test/jdk/java/lang/Math/IeeeRecommendedTests.java. At first i thought we no longer need PR #8459 but it seems both PRs are complimentary, albeit PR #8459 has more modest performance gains for the intrinsics. - PR: https://git.openjdk.java.net/jdk/pull/8525
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v6]
On Mon, 2 May 2022 20:31:18 GMT, Joe Darcy wrote: >> Add a new system property, java.specification.maintenance.version, to return >> the maintenance release number of the Java SE specification being >> implemented. The property is unset, optional in the terminology of >> System.getProperties, for an initial release of a specification. >> >> Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8285764 >> >> I'll update copyright years before an integration. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 10 additional commits since > the last revision: > > - Update wording to address review feedback. > - Merge branch 'master' into JDK-8285497 > - Change punctuation from review feedback. > - Respond to review feedback; make sequence of values explicit. > - Respond to review feedback. > - Respond to review feedback. > - Respond to CSR feedback. > - Merge branch 'master' into JDK-8285497 > - Update comment in template. > - JDK-8285497: Add system property for Java SE specification maintenance > version Changes requested by mr (Lead). - PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: JDK-8285497: Add system property for Java SE specification maintenance version [v5]
On Tue, 3 May 2022 01:20:10 GMT, Joe Darcy wrote:
>> In the latest push, to address the concerns raised updated the proposed
>> wording to, in plain text:
>>
>> Java Runtime Environment specification maintenance version, not defined
>> before the specification implemented by this runtime has undergone a
>> maintenance release (optional). When defined, the value of this property may
>> be interpreted as a positive integer. The value indicates the latest
>> maintenance release the runtime is known to support. A later release may be
>> supported by the environment. To indicate the first maintenance release this
>> property will have the value "1"; to indicate the second maintenance
>> release, this property will have the value "2", and so on.
>
> PS CSR Updated to reflect this push; please review:
> https://bugs.openjdk.java.net/browse/JDK-8285764
The negative definition above permits the property always to be undefined, but
we do want it to be defined when meaningful. It’s also getting to be an awful
lot of text to add to the otherwise terse tabular summary of system properties
in the [`System::getProperties()`
specification](https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/System.html#getProperties()).
Consider using this text in the table:
> Java Runtime Environment specification maintenance version, which may be
> interpreted as a positive integer (optional, see below)
and then this in a paragraph following the table:
> The {@code java.specification.maintenance.version} property is defined if the
> specification implemented by this runtime at the time of its construction had
> undergone a https://jcp.org/en/procedures/jcp2#3.6.4;>maintenance
> release. When defined, its value identifies that maintenance release. To
> indicate the first maintenance release this property will have the value
> {@code "1"}; to indicate the second maintenance release this property will
> have the value {@code "2"}, and so on.
-
PR: https://git.openjdk.java.net/jdk/pull/8437
Re: RFR: 8285973: x86_64: Improve fp comparison and cmove for eq/ne
On Wed, 4 May 2022 01:59:17 GMT, Quan Anh Mai wrote:
> Hi,
>
> This patch optimises the matching rules for floating-point comparison with
> respects to eq/ne on x86-64
>
> 1, When the inputs of a comparison is the same (i.e `isNaN` patterns), `ZF`
> is always set, so we don't need `cmpOpUCF2` for the eq/ne cases, which
> improves the sequence of `If (CmpF x x) (Bool ne)` from
>
> ucomiss xmm0, xmm0
> jp label
> jne label
>
> into
>
> ucomiss xmm0, xmm0
> jp label
>
> 2, The move rules for `cmpOpUCF2` is missing, which makes patterns such as `x
> == y ? 1 : 0` to fall back to `cmpOpU`, which have a really high cost of
> fixing the flags, such as
>
> xorlecx, ecx
> ucomiss xmm0, xmm1
> jnp done
> pushf
> andq[rsp], 0xff2b
> popf
> done:
> movleax, 1
> cmovel eax, ecx
>
> The patch changes this sequence into
>
> xorlecx, ecx
> ucomiss xmm0, xmm1
> movleax, 1
> cmovpl eax, ecx
> cmovnel eax, ecx
>
> 3, The patch also changes the pattern of `isInfinite` to be more optimised by
> using `Math.abs` to reduce 1 comparison and compares the result with
> `MAX_VALUE` since `>` is more optimised than `==` for floating-point types.
>
> The benchmark results are as follow:
>
> Before:
> Benchmark Mode Cnt Score Error Units
> FPComparison.equalDouble avgt5 2876.242 ± 58.875 ns/op
> FPComparison.equalFloatavgt5 3062.430 ± 31.371 ns/op
> FPComparison.isFiniteDoubleavgt5 475.749 ± 19.027 ns/op
> FPComparison.isFiniteFloat avgt5 506.525 ± 14.417 ns/op
> FPComparison.isInfiniteDouble avgt5 1232.800 ± 31.677 ns/op
> FPComparison.isInfiniteFloat avgt5 1234.708 ± 70.239 ns/op
> FPComparison.isNanDouble avgt5 2255.847 ± 7.238 ns/op
> FPComparison.isNanFloatavgt5 2567.044 ± 36.078 ns/op
>
> After:
> Benchmark Mode Cnt Score Error Units
> FPComparison.equalDouble avgt5 594.636 ± 8.922 ns/op
> FPComparison.equalFloatavgt5 663.849 ± 3.656 ns/op
> FPComparison.isFiniteDoubleavgt5 518.309 ± 107.352 ns/op
> FPComparison.isFiniteFloat avgt5 515.576 ± 14.669 ns/op
> FPComparison.isInfiniteDouble avgt5 621.185 ± 11.935 ns/op
> FPComparison.isInfiniteFloat avgt5 623.566 ± 15.206 ns/op
> FPComparison.isNanDouble avgt5 400.124 ± 0.762 ns/op
> FPComparison.isNanFloatavgt5 546.486 ± 1.509 ns/op
>
> Thank you very much.
Thank you for including tests. But you need additional other float compare (not
just `ea` `ne`) tests since you removed some of `Cmp` instructions.
You need approval from core libs for Java methods changes (it affects all
platforms). And we will get intrinsics for them soon (I think): #8459.
Please add comments to `cmpOpU*` operands explaining changes in them.
You did not explain removal of some float compare instructions.
src/hotspot/cpu/x86/x86_64.ad line 6998:
> 6996: ins_encode %{
> 6997: __ cmovl(Assembler::parity, $dst$$Register, $src$$Register);
> 6998: __ cmovl(Assembler::notEqual, $dst$$Register, $src$$Register);
Should this be `equal`?
-
Changes requested by kvn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8525
Integrated: 8277090 : jsr166 refresh for jdk19
On Wed, 4 May 2022 12:30:53 GMT, Doug Lea wrote: > This is the revised jsr166 refresh for jdk19. See > https://bugs.openjdk.java.net/browse/JDK-8285450 and > https://bugs.openjdk.java.net/browse/JDK-8277090. Out of caution, this PR > removes the unrelated commits from original version. This pull request has now been integrated. Changeset: 00e6c63c Author:Doug Lea URL: https://git.openjdk.java.net/jdk/commit/00e6c63cd12e3f92d0c1d007aab4f74915616ffb Stats: 3367 lines in 13 files changed: 1731 ins; 645 del; 991 mod 8277090: jsr166 refresh for jdk19 Reviewed-by: alanb, psandoz - PR: https://git.openjdk.java.net/jdk/pull/8536
Re: RFR: 8285295: Need better testing for IdentityHashMap [v3]
On Wed, 4 May 2022 18:46:20 GMT, Stuart Marks wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Improve wording of comment. I've added a few more assertions to cover `equals` of `entrySet` and `keySet` which I think were missing some cases. (Note that `values` returns a `Collection` which has no notion of equality.) I think this is ready now. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Add some assertions for entrySet.equals and keySet.equals - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/bf4af51f..4bb25edf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8354=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8354=02-03 Stats: 32 lines in 1 file changed: 32 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator)
On Wed, 27 Apr 2022 11:03:48 GMT, Jatin Bhateja wrote: > Hi All, > > Patch adds the planned support for new vector operations and APIs targeted > for [JEP 426: Vector API (Fourth > Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173) > > Following is the brief summary of changes:- > > 1) Extends the scope of existing lanewise API for following new vector > operations. >- VectorOperations.BIT_COUNT: counts the number of one-bits >- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero > bits >- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing > zero bits >- VectorOperations.REVERSE: reversing the order of bits >- VectorOperations.REVERSE_BYTES: reversing the order of bytes >- compress and expand bits: Semantics are based on Hacker's Delight > section 7-4 Compress, or Generalized Extract. > > 2) Adds following new APIs to perform cross lane vector compress and > expansion operations under the influence of a mask. >- Vector.compress >- Vector.expand >- VectorMask.compress > > 3) Adds predicated and non-predicated versions of following new APIs to load > and store the contents of vector from foreign MemorySegments. > - Vector.fromMemorySegment > - Vector.intoMemorySegment > > 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support > for each newly added operation. > > > Patch has been regressed over AARCH64 and X86 targets different AVX levels. > > Kindly review and share your feedback. > > Best Regards, > Jatin src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 1340: > 1338: assert_different_registers(dst, src, vtmp1, vtmp2, vtmp3, vtmp4); > 1339: assert_different_registers(mask, ptmp, pgtmp); > 1340: // Example input: src = 88 77 66 45 44 33 22 11 Suggestion: // Example input: src = 88 77 66 55 44 33 22 11 - PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 14:55:25 GMT, Jaikiran Pai wrote: >> Stuart Marks has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Assertions over return values. Some refinement of equals() testing. >> - Add comment about Map.Entry identity not guaranteed. > > test/jdk/java/util/IdentityHashMap/Basic.java line 50: > >> 48: // that a map's entrySet contains exactly the expected mappings. In most >> cases, keys and >> 49: // values should be compared by identity, not equality. However, the >> identities of Map.Entry >> 50: // instances from an IdentityHashSet are not guaranteed; the keys and >> values they contain > > I think I understand what you are saying here, but it took me a few reads to > understand it. More so because of `IdentityHashSet` here, which I think is a > typo. > So what's being stated here is that you cannot do something like: > > IdentityHashMap m = new IdentityHashMap(); > ... > var e1 = m.entrySet(); > var e2 = m.entrySet(); > > assertSame(e1, e2); // this isn't guaranteed to pass > > Did I understand this right? Yeah that comment was poorly worded, and indeed I meant the IdentityHashMap's entrySet and not IdentityHashSet. I've reworded it. I was trying to make a statement about exactly what needs to be compared if you want to make assertions about two maps being "equal" in the sense of IdentityHashMap behaving correctly. Specifically, given two maps `m1` and `m2`, clearly we don't want either of assertSame(m1, m2); assertSame(m1.entrySet(), m2.entrySet()); Instead, we want the `Map.Entry` instances to "match". However, given two entries `entry1` and `entry2` that are supposed to match, we also do not want assertSame(entry1, entry2); Instead, we want assertSame(entry1.getKey(), entry2.getKey()); assertSame(entry1.getValue(), entry2,getValue()); The `checkEntries` method goes to some length to get this right. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 15:02:43 GMT, liach wrote:
>> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>>
>>> 498: Box newKey = new Box(k1a);
>>> 499: Box newVal = new Box(v1a);
>>> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
>>> return newVal; });
>>
>> More of a curiosity than a review comment - I see that various places in
>> this PR use a boolean array with one element instead of just a boolean type.
>> Is that a personal coding preference or is there something more to it?
>
> This just serves as a modifiable boolean like an AtomicBoolean. Remember
> lambdas can only use final local var references (due to how they work), and
> it cannot access or modify the local variable in the caller method.
Yes, that's it; you can't mutate a local variable captured by a lambda.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v3]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Improve wording of comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/d66ad0b8..bf4af51f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8354=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8354=01-02 Stats: 6 lines in 1 file changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc [v2]
On Wed, 4 May 2022 17:42:05 GMT, Sandhya Viswanathan wrote: >> Tyler Steele has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright header year > > Marked as reviewed by sviswanathan (Reviewer). Thank you @sviswa7. > src/jdk.incubator.vector/linux/native/libjsvml/globals_vectorApiSupport_linux.S.inc > line 4: > >> 2: * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >> 4: * > > Please update the copyright year to 2022. - PR: https://git.openjdk.java.net/jdk/pull/8508
Re: RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc [v2]
> Adds missing classpath exception to the header of two GPLv2 files. > > Requested > [here](https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2022-April/013988.html). Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Update copyright header year - Changes: - all: https://git.openjdk.java.net/jdk/pull/8508/files - new: https://git.openjdk.java.net/jdk/pull/8508/files/72f74f0e..cdae1312 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8508=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8508=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8508.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8508/head:pull/8508 PR: https://git.openjdk.java.net/jdk/pull/8508
Integrated: 8279598: Provide adapter from RandomGenerator to Random
On Sat, 8 Jan 2022 21:00:37 GMT, Yasser Bazzi wrote: > Hi, could i get a review on this implementation proposed by Stuart Marks, i > decided to use the > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html > interface to create the default method `asRandom()` that wraps around the > newer algorithms to be used on classes that do not accept the new interface. > > Some things to note as proposed by the bug report, the protected method > next(int bits) is not overrided and setSeed() method if left blank up to > discussion on what to do with it. > > Small test done on > https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 This pull request has now been integrated. Changeset: df8c2be5 Author:Yasser Bazzi Bordonal Committer: Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/df8c2be5fedd3a066b1d7371a3e3cbb7191977b0 Stats: 250 lines in 3 files changed: 225 ins; 3 del; 22 mod 8279598: Provide adapter from RandomGenerator to Random Reviewed-by: smarks, darcy - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
On Mon, 2 May 2022 20:05:36 GMT, Tyler Steele wrote: > Adds missing classpath exception to the header of two GPLv2 files. > > Requested > [here](https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2022-April/013988.html). src/jdk.incubator.vector/linux/native/libjsvml/globals_vectorApiSupport_linux.S.inc line 4: > 2: * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights > reserved. > 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > 4: * Please update the copyright year to 2022. - PR: https://git.openjdk.java.net/jdk/pull/8508
Re: RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
On Mon, 2 May 2022 20:05:36 GMT, Tyler Steele wrote: > Adds missing classpath exception to the header of two GPLv2 files. > > Requested > [here](https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2022-April/013988.html). Marked as reviewed by sviswanathan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8508
RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
Adds missing classpath exception to the header of two GPLv2 files. Requested [here](https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2022-April/013988.html). - Commit messages: - Add classpath excemption to copyright header Changes: https://git.openjdk.java.net/jdk/pull/8508/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8508=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286029 Stats: 6 lines in 2 files changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8508.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8508/head:pull/8508 PR: https://git.openjdk.java.net/jdk/pull/8508
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v8]
On Mon, 2 May 2022 12:32:25 GMT, Volker Simonis wrote: >> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` >> to highlight that it might write more bytes than the returned number of >> inflated bytes into the buffer `b`. >> >> The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, >> int len)` will leave the content beyond the last read byte in the read >> buffer `b` unaffected. However, the overridden `read` method in >> `InflaterInputStream` passes the read buffer `b` to >> `Inflater::inflate(byte[] b, int off, int len)` which doesn't provide this >> guarantee. Depending on implementation details, `Inflater::inflate` might >> write more than the returned number of inflated bytes into the buffer `b`. >> >> ### TL;DR >> >> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater >> functionality. `Inflater::inflate(byte[] output, int off, int len)` >> currently calls zlib's native `inflate(..)` function and passes the address >> of `output[off]` and `len` to it via JNI. >> >> The specification of zlib's `inflate(..)` function (i.e. the [API >> documentation in the original zlib >> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) >> doesn't give any guarantees with regard to usage of the output buffer. It >> only states that upon completion the function will return the number of >> bytes that have been written (i.e. "inflated") into the output buffer. >> >> The original zlib implementation only wrote as many bytes into the output >> buffer as it inflated. However, this is not a hard requirement and newer, >> more performant implementations of the zlib library like >> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) >> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes >> of the output buffer than they actually inflate as a scratch buffer. See >> https://github.com/simonis/zlib-chromium for a more detailed description of >> their approach and its performance benefit. >> >> These new zlib versions can still be used transparently from Java (e.g. by >> putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because >> they still fully comply to specification of `Inflater::inflate(..)`. >> However, we might run into problems when using the `Inflater` functionality >> from the `InflaterInputStream` class. `InflaterInputStream` is derived from >> from `InputStream` and as such, its `read(byte[] b, int off, int len)` >> method is quite constrained. It specifically specifies that if *k* bytes >> have been read, then "these bytes will be stored in elements `b[off]` >> through `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through >> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[] b, int >> off, int len)` (which is constrained by `InputStream::read(..)`'s >> specification) calls `Inflater::inflate(byte[] b, int off, int len)` and >> directly passes its output buffer down to the native zlib `inflate(..)` >> method which is free to change the bytes beyond `b[off+` *k*`]` (where *k* is the number of inflated bytes). >> >> From a practical point of view, I don't see this as a big problem, because >> callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never >> know how many bytes will be written into the output buffer `b` (and in fact >> its content can always be completely overwritten). It therefore makes no >> sense to depend on any data there being untouched after the call. Also, >> having used zlib-cloudflare productively for about two years, we haven't >> seen real-world issues because of this behavior yet. However, from a >> specification point of view it is easy to artificially construct a program >> which violates `InflaterInputStream::read(..)`'s postcondition if using one >> of the alterantive zlib implementations. A recently integrated JTreg test >> (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails >> with zlib-chromium but can fixed easily to run with alternative >> implementations as well (see >> [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Updated wording based on @JoeDarcy's CSR review @jddarcy requested in the [CSR](https://bugs.openjdk.java.net/browse/JDK-8283758) to directly update the API spec of `InputStream`. This considerably simplifies the whole patch because the derived classes now inherit the correct specification right from their base class and don't need to be updated anymore. Please comment right on the [CSR](https://bugs.openjdk.java.net/browse/JDK-8283758) should you have any objections or suggestions. - PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v9]
> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` to > highlight that it might write more bytes than the returned number of > inflated bytes into the buffer `b`. > > The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, > int len)` will leave the content beyond the last read byte in the read buffer > `b` unaffected. However, the overridden `read` method in > `InflaterInputStream` passes the read buffer `b` to `Inflater::inflate(byte[] > b, int off, int len)` which doesn't provide this guarantee. Depending on > implementation details, `Inflater::inflate` might write more than the > returned number of inflated bytes into the buffer `b`. > > ### TL;DR > > `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater > functionality. `Inflater::inflate(byte[] output, int off, int len)` currently > calls zlib's native `inflate(..)` function and passes the address of > `output[off]` and `len` to it via JNI. > > The specification of zlib's `inflate(..)` function (i.e. the [API > documentation in the original zlib > implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) > doesn't give any guarantees with regard to usage of the output buffer. It > only states that upon completion the function will return the number of bytes > that have been written (i.e. "inflated") into the output buffer. > > The original zlib implementation only wrote as many bytes into the output > buffer as it inflated. However, this is not a hard requirement and newer, > more performant implementations of the zlib library like > [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) > or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes > of the output buffer than they actually inflate as a scratch buffer. See > https://github.com/simonis/zlib-chromium for a more detailed description of > their approach and its performance benefit. > > These new zlib versions can still be used transparently from Java (e.g. by > putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because > they still fully comply to specification of `Inflater::inflate(..)`. However, > we might run into problems when using the `Inflater` functionality from the > `InflaterInputStream` class. `InflaterInputStream` is derived from from > `InputStream` and as such, its `read(byte[] b, int off, int len)` method is > quite constrained. It specifically specifies that if *k* bytes have been > read, then "these bytes will be stored in elements `b[off]` through > `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through `b[off+len-1]` > **unaffected**". But `InflaterInputStream::read(byte[] b, int off, int len)` > (which is constrained by `InputStream::read(..)`'s specification) calls > `Inflater::inflate(byte[] b, int off, int len)` and directly passes its > output buffer down to the native zlib `inflate(..)` method which is free to > change the bytes beyond `b[off+`* k*`]` (where *k* is the number of inflated bytes). > > From a practical point of view, I don't see this as a big problem, because > callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never > know how many bytes will be written into the output buffer `b` (and in fact > its content can always be completely overwritten). It therefore makes no > sense to depend on any data there being untouched after the call. Also, > having used zlib-cloudflare productively for about two years, we haven't seen > real-world issues because of this behavior yet. However, from a specification > point of view it is easy to artificially construct a program which violates > `InflaterInputStream::read(..)`'s postcondition if using one of the > alterantive zlib implementations. A recently integrated JTreg test > (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails > with zlib-chromium but can fixed easily to run with alternative > implementations as well (see > [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). Volker Simonis has updated the pull request incrementally with one additional commit since the last revision: Updated wording based on @JoeDarcy's second CSR review - Changes: - all: https://git.openjdk.java.net/jdk/pull/7986/files - new: https://git.openjdk.java.net/jdk/pull/7986/files/63ae3572..f77b3352 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7986=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7986=07-08 Stats: 42 lines in 4 files changed: 0 ins; 24 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/7986.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7986/head:pull/7986 PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v4]
On Tue, 26 Apr 2022 21:17:34 GMT, Naoto Sato wrote: >> Ichiroh Takiguchi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8285517: System.getenv() returns unexpected value if environment variable >> has non ASCII character > > Thanks for fixing the issue. > As to the test, it has lots of redundancy, and too complicated to me. Can you > simplify the test? Hello @naotoj . I removed `i18nEnvArg.sh` and modified `i18nEnvArg.java`. Could you review it again ? - PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v4]
> On JDK19 with Linux ja_JP.eucjp locale, > System.getenv() returns unexpected value if environment variable has Japanese > EUC characters. > It seems this issue happens because of JEP 400. > Arguments for ProcessBuilder have same kind of issue. Ichiroh Takiguchi has updated the pull request incrementally with one additional commit since the last revision: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character - Changes: - all: https://git.openjdk.java.net/jdk/pull/8378/files - new: https://git.openjdk.java.net/jdk/pull/8378/files/9db79874..84e6d639 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8378=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8378=02-03 Stats: 107 lines in 2 files changed: 40 ins; 60 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/8378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8378/head:pull/8378 PR: https://git.openjdk.java.net/jdk/pull/8378
Re: RFR: 8277090 : jsr166 refresh for jdk19
On Wed, 4 May 2022 12:30:53 GMT, Doug Lea wrote: > This is the revised jsr166 refresh for jdk19. See > https://bugs.openjdk.java.net/browse/JDK-8285450 and > https://bugs.openjdk.java.net/browse/JDK-8277090. Out of caution, this PR > removes the unrelated commits from original version. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8536
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Tue, 3 May 2022 10:40:02 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 57 commits:
>
> - Merge branch 'master' into foreign-preview
> - Update
> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Tweak support for Linker lookup
>Improve javadoc of SymbolLookup
> - Tweak Addressable javadoc
> - Downcall and upcall tests use wrong layout which is missing some trailing
> padding
> - Simplify libraryLookup impl
> - Address CSR comments
> - Merge branch 'master' into foreign-preview
> - Add ValueLayout changes
> - Tweak support for array element var handle
> - ... and 47 more:
> https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 116:
> 114: * Linker nativeLinker = Linker.nativeLinker();
> 115: * SymbolLookup stdlib = nativeLinker.defaultLookup();
> 116: * MemorySegment malloc = stdlib.lookup("malloc");
This should be:
Suggestion:
* Optional malloc = stdlib.lookup("malloc");
or
Suggestion:
* MemorySegment malloc = stdlib.lookup("malloc").orElseThrow();
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
> 151: static SymbolLookup loaderLookup() {
> 152: Class caller = Reflection.getCallerClass();
> 153: ClassLoader loader =
> Objects.requireNonNull(caller.getClassLoader());
Shouldn’t this be changed to throw `IllegalCallerException` instead of throwing
`NullPointerException` when the `caller`’s `ClassLoader` is `null`[^1] or when
`caller` itself is `null`[^2]?
[^1]: This happens when `caller` is on the bootstrap classpath.
[^2]: This happens when `SymbolLookup.loaderLookup()` is called by native code
and no **Java** code is on the call stack.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v11]
On Wed, 4 May 2022 12:12:48 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. We >> can add additional labels, if required, as the PR progresses. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - Refresh cf561073f48fad58e931a5285b92629aa83c9bd1 > - Merge with jdk-19+19 > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/4b2c8220...f06aff75 > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for > zero and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. So, does this PR pass current GHA checks? I see they are not enabled for this PR. It would be unfortunate for this large integration to break builds/tests for smaller PRs that would follow it. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v22]
> Hi, could i get a review on this implementation proposed by Stuart Marks, i > decided to use the > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html > interface to create the default method `asRandom()` that wraps around the > newer algorithms to be used on classes that do not accept the new interface. > > Some things to note as proposed by the bug report, the protected method > next(int bits) is not overrided and setSeed() method if left blank up to > discussion on what to do with it. > > Small test done on > https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 Yasser Bazzi has updated the pull request incrementally with one additional commit since the last revision: Update full name - Changes: - all: https://git.openjdk.java.net/jdk/pull/7001/files - new: https://git.openjdk.java.net/jdk/pull/7001/files/a28ba8e9..bc5516c0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=21 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=20-21 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7001.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7001/head:pull/7001 PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Fri, 29 Apr 2022 03:00:40 GMT, Stuart Marks wrote:
>> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch
>> in the bug report that breaks `IdentityHashMap` now causes several cases in
>> this new test to fail. There's more that could be done, but the new tests
>> cover most of the core functions of `IdentityHashMap`. Unfortunately it
>> seems difficult to merge this with the existing, comprehensive Collections
>> tests (e.g., MOAT.java) because those tests implicity rely on
>> `equals()`-based contract instead of the special-purpose `==`-based contract
>> used by `IdentityHashMap`.
>
> Stuart Marks has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Assertions over return values. Some refinement of equals() testing.
> - Add comment about Map.Entry identity not guaranteed.
Hello Stuart, I had a look at the updates. This looks good to me, thank you for
the changes. Just a couple of comments/questions that I've included at relevant
lines.
test/jdk/java/util/IdentityHashMap/Basic.java line 50:
> 48: // that a map's entrySet contains exactly the expected mappings. In most
> cases, keys and
> 49: // values should be compared by identity, not equality. However, the
> identities of Map.Entry
> 50: // instances from an IdentityHashSet are not guaranteed; the keys and
> values they contain
I think I understand what you are saying here, but it took me a few reads to
understand it. More so because of `IdentityHashSet` here, which I think is a
typo.
So what's being stated here is that you cannot do something like:
IdentityHashMap m = new IdentityHashMap();
...
var e1 = m.entrySet();
var e2 = m.entrySet();
assertSame(e1, e2); // this isn't guaranteed to pass
Did I understand this right?
test/jdk/java/util/IdentityHashMap/Basic.java line 500:
> 498: Box newKey = new Box(k1a);
> 499: Box newVal = new Box(v1a);
> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
> return newVal; });
More of a curiosity than a review comment - I see that various places in this
PR use a boolean array with one element instead of just a boolean type. Is that
a personal coding preference or is there something more to it?
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 14:57:19 GMT, Jaikiran Pai wrote:
>> Stuart Marks has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Assertions over return values. Some refinement of equals() testing.
>> - Add comment about Map.Entry identity not guaranteed.
>
> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>
>> 498: Box newKey = new Box(k1a);
>> 499: Box newVal = new Box(v1a);
>> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
>> return newVal; });
>
> More of a curiosity than a review comment - I see that various places in this
> PR use a boolean array with one element instead of just a boolean type. Is
> that a personal coding preference or is there something more to it?
This just serves as a modifiable boolean like an AtomicBoolean. Remember
lambdas can only use final local var references (due to how they work), and it
cannot access or modify the local variable in the caller method.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Integrated: JDK-8286114: [test] show real exception in bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java
On Wed, 4 May 2022 11:37:09 GMT, Matthias Baesken wrote: > There is one TestLibrary.bomb call in > sun/rmi/runtime/Log/checkLogging/CheckLogging.java that is not passing the > exception to bomb, that should be improved. This pull request has now been integrated. Changeset: 7424f475 Author:Matthias Baesken URL: https://git.openjdk.java.net/jdk/commit/7424f47557be54c781f64f1c0c9265e11fe40acf Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8286114: [test] show real exception in bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java Reviewed-by: rriggs, mdoerr - PR: https://git.openjdk.java.net/jdk/pull/8533
Re: RFR: 8277090 : jsr166 refresh for jdk19
On Wed, 4 May 2022 12:30:53 GMT, Doug Lea wrote: > This is the revised jsr166 refresh for jdk19. See > https://bugs.openjdk.java.net/browse/JDK-8285450 and > https://bugs.openjdk.java.net/browse/JDK-8277090. Out of caution, this PR > removes the unrelated commits from original version. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8536
Re: RFR: JDK-8286114: [test] show real exception in bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java
On Wed, 4 May 2022 11:37:09 GMT, Matthias Baesken wrote: > There is one TestLibrary.bomb call in > sun/rmi/runtime/Log/checkLogging/CheckLogging.java that is not passing the > exception to bomb, that should be improved. Hi Roger and Martin, thanks for the reviews ! - PR: https://git.openjdk.java.net/jdk/pull/8533
Re: RFR: JDK-8286114: [test] show real exception in bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java
On Wed, 4 May 2022 11:37:09 GMT, Matthias Baesken wrote: > There is one TestLibrary.bomb call in > sun/rmi/runtime/Log/checkLogging/CheckLogging.java that is not passing the > exception to bomb, that should be improved. Marked as reviewed by mdoerr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8533
Re: RFR: JDK-8286114: [test] show real exception in bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java
On Wed, 4 May 2022 11:37:09 GMT, Matthias Baesken wrote: > There is one TestLibrary.bomb call in > sun/rmi/runtime/Log/checkLogging/CheckLogging.java that is not passing the > exception to bomb, that should be improved. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8533
RFR: 8277090 : jsr166 refresh for jdk19 v2
This is the revised jsr166 refresh for jdk19. See https://bugs.openjdk.java.net/browse/JDK-8285450 and https://bugs.openjdk.java.net/browse/JDK-8277090. Out of caution, this PR removes the unrelated commits from original version. - Commit messages: - Redoing JDK-8277090 to avoid stray commits Changes: https://git.openjdk.java.net/jdk/pull/8536/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8536=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277090 Stats: 3367 lines in 13 files changed: 1731 ins; 645 del; 991 mod Patch: https://git.openjdk.java.net/jdk/pull/8536.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8536/head:pull/8536 PR: https://git.openjdk.java.net/jdk/pull/8536
Withdrawn: 8277090 : jsr166 refresh for jdk19
On Sun, 1 May 2022 10:53:55 GMT, Doug Lea wrote: > This is the jsr166 refresh for jdk19. See > https://bugs.openjdk.java.net/browse/JDK-8285450 and > https://bugs.openjdk.java.net/browse/JDK-8277090 This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/8490
Re: RFR: 8277090 : jsr166 refresh for jdk19 [v3]
On Tue, 3 May 2022 23:10:44 GMT, Doug Lea wrote: >> This is the jsr166 refresh for jdk19. See >> https://bugs.openjdk.java.net/browse/JDK-8285450 and >> https://bugs.openjdk.java.net/browse/JDK-8277090 > > Doug Lea has updated the pull request incrementally with one additional > commit since the last revision: > > Fix internal doc typos; thanks to Martin for finding these. Out of caution about the extraneous commits in this PR, I will close this, and replace with a v2 momentarily. - PR: https://git.openjdk.java.net/jdk/pull/8490
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v11]
> This is the implementation of JEP 425: Virtual Threads (Preview). > > We will refresh this PR periodically to pick up changes and fixes from the > loom repo. > > Most of the new mechanisms in the HotSpot VM are disabled by default and > require running with `--enable-preview` to enable. > > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for > zero and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. > > There are changes in many areas. To reduce notifications/mails, the labels > have been trimmed down for now to hotspot, serviceability and core-libs. We > can add additional labels, if required, as the PR progresses. > > The changes include a refresh of java.util.concurrent and ForkJoinPool from > Doug Lea's CVS. These changes will probably be proposed and integrated in > advance of this PR. > > The changes include some non-exposed and low-level infrastructure to support > the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to > make life a bit easier and avoid having to separate VM changes and juggle > branches at this time. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec - Merge with jdk-19+20 - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 - Refresh cf561073f48fad58e931a5285b92629aa83c9bd1 - Merge with jdk-19+19 - ... and 7 more: https://git.openjdk.java.net/jdk/compare/4b2c8220...f06aff75 - Changes: https://git.openjdk.java.net/jdk/pull/8166/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8166=10 Stats: 102779 lines in 1140 files changed: 92909 ins; 4219 del; 5651 mod Patch: https://git.openjdk.java.net/jdk/pull/8166.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166 PR: https://git.openjdk.java.net/jdk/pull/8166
RFR: JDK-8285987: executing shell scripts without #! fails on Alpine linux
A couple a tests like java/lang/ProcessBuilder/Basic.java#id0.Basic_id0 and jdk/jshell/ExternalEditorTest.java.ExternalEditorTest try to start small shell scripts without #! at the first line of the script. This fails with error=8, Exec format error when running on Alpine 3.15 . Looks like this is a known issue on musl / Alpine, see also https://www.openwall.com/lists/musl/2018/03/09/2 and https://github.com/scala-steward-org/scala-steward/issues/1374 (we see it on Alpine 3.15). - Commit messages: - JDK-8285987 Changes: https://git.openjdk.java.net/jdk/pull/8535/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8535=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285987 Stats: 35 lines in 3 files changed: 17 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/8535.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8535/head:pull/8535 PR: https://git.openjdk.java.net/jdk/pull/8535
RFR: JDK-8286114: [test] show real exception in bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java
There is one TestLibrary.bomb call in sun/rmi/runtime/Log/checkLogging/CheckLogging.java that is not passing the exception to bomb, that should be improved. - Commit messages: - JDK-8286114 Changes: https://git.openjdk.java.net/jdk/pull/8533/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8533=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286114 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8533.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8533/head:pull/8533 PR: https://git.openjdk.java.net/jdk/pull/8533
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda wrote:
> 8262889: Compiler implementation for Record Patterns
>
> A first version of a patch that introduces record patterns into javac as a
> preview feature. For the specification, please see:
> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>
> There are two notable tricky parts:
> -in the parser, it was necessary to improve the `analyzePattern` method to
> handle nested/record patterns, while still keeping error recovery reasonable
> -in the `TransPatterns`, the desugaring of the record patterns is very
> straightforward - effectivelly the record patterns are desugared into
> guards/conditions. This will likely be improved in some future version/preview
>
> `MatchException` has been extended to cover additional cases related to
> record patterns.
Changes look generally good. Personally, I found the changes in Flow the harder
to follow - but I that part is just hard (even in the spec), as it has to
perform a search in both breadth (as records have more than one component) and
in depth (as patterns can be nested). I've added some comments to check my
understanding.
src/java.base/share/classes/java/lang/MatchException.java line 41:
> 39: *constants at runtime than it had at compilation time, or the
> type hierarchy has changed
> 40: *in incompatible ways between compile time and run time.
> 41: *Null targets and sealed types. If an interface or abstract
> class {@code C} is sealed
Should this be `null targets and nested sealed type test patterns` ?
Also, not sure `null` target is the right expression - maybe `null and nested
xyz` would work better?
src/jdk.compiler/share/classes/com/sun/source/tree/DeconstructionPatternTree.java
line 43:
> 41: * @return the deconstructed type
> 42: */
> 43: Tree getDeconstructor();
Shouldn't this return an ExpressionTree?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180:
> 4178: type = attribTree(tree.var.vartype, env, varInfo);
> 4179: } else {
> 4180: type = resultInfo.pt;
Looks good - infers the binging var type from the record component being
processed. If not in a record, then I suspect resultInfo.pt is just the target
expression type (e.g. var in toplevel environment).
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 750:
> 748: private Set coveredSymbols(DiagnosticPosition pos, Type
> targetType,
> 749:Iterable JCCaseLabel> labels) {
> 750: Set constants = new HashSet<>();
Should this be called `constants` ?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 824:
> 822: }
> 823: for (Symbol currentType : nestedCovered) {
> 824: if (types.isSubtype(types.erasure(currentType.type),
Not 100% what this test does
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 836:
> 834: for (Entry> e :
> componentType2Patterns.entrySet()) {
> 835: if (coversDeconstructionStartingFromComponent(pos,
> targetType, e.getValue(), component + 1)) {
> 836: covered.add(e.getKey());
So... it seems to me that what this code is doing is that, for a component
index _i_, we get all its recursively covered symbols - then we add them to the
set of covered symbols of component _i_, but only if components _w_, where _w_
> _i_ are also covered? That is, if in `R(P1, P2, ... Pn)`, we see that `P1` is
covered, but `P2` is not, we also consider `P1` not covered (which in turn
makes `R` not covered).
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
788:
> 786: }
> 787: if (token.kind == LPAREN) {
> 788: ListBuffer nested = new ListBuffer<>();
should we add comment saying "deconstruction pattern"
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
808:
> 806: pattern = toP(F.at(pos).DeconstructionPattern(e,
> nested.toList(), var));
> 807: } else {
> 808: JCVariableDecl var = toP(F.at(token.pos).VarDef(mods,
> ident(), e, null));
and, here, a comment saying "type test pattern" ?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
1004:
> 1002: JCExpression type = unannotatedType(false);
> 1003: if (token.kind == IDENTIFIER) {
> 1004: checkSourceLevel(token.pos,
> Feature.PATTERN_MATCHING_IN_INSTANCEOF);
Two question/comments:
* I believe that `checkSourceLevel` is called here, but not in `analyzePattern`
because `analyzePattern` is also called in the `switch` code, in which case we
already check for source level which supports pattern in switch, correct?
* For type test pattern, this
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Wed, 4 May 2022 09:59:33 GMT, Maurizio Cimadamore
wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180:
>
>> 4178: type = attribTree(tree.var.vartype, env, varInfo);
>> 4179: } else {
>> 4180: type = resultInfo.pt;
>
> Looks good - infers the binging var type from the record component being
> processed. If not in a record, then I suspect resultInfo.pt is just the
> target expression type (e.g. var in toplevel environment).
That said, I'm not sure how this connects with `instanceof`. This patch doesn't
seem to alter `visitTypeTest`. In the current code I can see this:
if (tree.pattern.getTag() == BINDINGPATTERN ||
tree.pattern.getTag() == PARENTHESIZEDPATTERN) {
attribTree(tree.pattern, env, unknownExprInfo);
...
This seems wrong for two reasons:
* it doesn't take into account the new pattern tag
* it uses an "unknown" result info when attributing, meaning that any toplevel
`var` pattern will not be attributed correctly
But we seem to have tests covering record patterns and instanceof. so I'm
wondering if I'm missing some code update?
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure [v2]
On Wed, 4 May 2022 09:46:00 GMT, Сергей Цыпанов wrote: >> `Class.getInterfaces(false)` does not clone underlying array and can be used >> in cases when the returned array is only read from. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282701: Revert some irrelevant changes I've reverted changes to `getInterface(boolean)` - PR: https://git.openjdk.java.net/jdk/pull/7714
Re: RFR: 8282701: Use Class.getInterfaces(false) where possible to reduce allocation pressure [v2]
> `Class.getInterfaces(false)` does not clone underlying array and can be used > in cases when the returned array is only read from. Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8282701: Revert some irrelevant changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7714/files - new: https://git.openjdk.java.net/jdk/pull/7714/files/f13239c3..b79f1ddb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7714=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7714=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7714.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7714/head:pull/7714 PR: https://git.openjdk.java.net/jdk/pull/7714
Re: RFR: 8284950: CgroupV1 detection code should consider memory.swappiness [v15]
On Wed, 4 May 2022 01:37:27 GMT, xpbob wrote:
>> set memory.swappiness to 0,swap space will not be used
>> determine the value of memory.swappiness
>> https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt
>>
>>
>> Memory Limit: 50.00M
>> Memory Soft Limit: Unlimited
>> Memory & Swap Limit: 100.00M
>> Maximum Processes Limit: 4194305
>>
>> =>
>>
>> Memory Limit: 50.00M
>> Memory Soft Limit: Unlimited
>> Memory & Swap Limit: 50.00M
>> Maximum Processes Limit: 4194305
>
> xpbob has updated the pull request incrementally with one additional commit
> since the last revision:
>
> update code
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 154:
> 152: log_trace(os, container)("Memory and Swap Limit has been reset to
> " JULONG_FORMAT " because swappiness is 0", memlimit);
> 153: return memlimit;
> 154: }
It now occurs to me that we'd need the same treatment of this on line `143`.
Perhaps extract a function and call it in both places?
-
PR: https://git.openjdk.java.net/jdk/pull/8285
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
On Wed, 4 May 2022 08:34:43 GMT, David Holmes wrote: > I'm confused. > `src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp` doesn't set > _WIN32_WINNT so how is that later API being enabled? Does this mean that not > setting _WIN32_WINNT means :any API is allowed" ? I found this info here https://stackoverflow.com/questions/37668692/visual-studio-setting-winver-win32-winnt-to-windows-8-on-windows-7 "VS 2012 uses the Windows 8.0 SDK which defaults to _WIN32_WINNT=0x0602 (Windows 8). VS 2013 / 2015 uses the Windows 8.1 SDK which defaults to _WIN32_WINNT=0x0603 (Windows 8.1). If you use VS 2015 and the Windows 10 SDK, it defaults to _WIN32_WINNT=0x0A00 (Windows 10)." So it seems you get some more or less recent default when working with a not too old compiler/SDK. - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken wrote: >> Currently we set _WIN32_WINNT at various places in the codebase; this is >> used to target a minimum Windows version we want to support. See also for >> more detailled information : >> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt >> Macros for Conditional Declarations >> "Certain functions that depend on a particular version of Windows are >> declared using conditional code. This enables you to use the compiler to >> detect whether your application uses functions that are not supported on its >> target version(s) of Windows." >> >> However currently we have quite a lot of differing settings of _WIN32_WINNT >> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible >> would make sense because we have this setting already at java_props_md.c >> (so targeting older Windows versions at other places is most likely not >> useful). > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust API level to Windows 8 for security.cpp and do some cleanup I'm confused. `src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp` doesn't set _WIN32_WINNT so how is that later API being enabled? Does this mean that not setting _WIN32_WINNT means :any API is allowed" ? - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
> Currently we set _WIN32_WINNT at various places in the codebase; this is used > to target a minimum Windows version we want to support. See also for more > detailled information : > https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt > Macros for Conditional Declarations > "Certain functions that depend on a particular version of Windows are > declared using conditional code. This enables you to use the compiler to > detect whether your application uses functions that are not supported on its > target version(s) of Windows." > > However currently we have quite a lot of differing settings of _WIN32_WINNT > in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible > would make sense because we have this setting already at java_props_md.c > (so targeting older Windows versions at other places is most likely not > useful). Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: adjust API level to Windows 8 for security.cpp and do some cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/8428/files - new: https://git.openjdk.java.net/jdk/pull/8428/files/23b63c5b..721528c4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8428=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8428=02-03 Stats: 31 lines in 7 files changed: 1 ins; 26 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8428.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8428/head:pull/8428 PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v3]
On Tue, 3 May 2022 13:27:41 GMT, David Holmes wrote: > I agree with Erik - the source locations need to be modified to not define > it. If we want to keep a record perhaps add an assertion that the value is > what was expected? > > I still feel we have a disconnect between this and an actual check for what > the build and runtime platform version is ... > > and it isn't at all clear how someone using an API only in a later Windows > version, and so setting _WIN32_WINNT to a higher value, will know that this > is defined down in the build files ? Hi David, I agree the other locations as long as they are not 3rd party code should be cleaned up. Someone using an API that is only available in later Windows versions would get a redefinition warning as long as no undefine is done. But we have to set _WIN32_WINNT anyway to a higher value (windows8, I think that's 0x0602) to compile src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp . - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v3]
On Tue, 3 May 2022 07:10:58 GMT, Matthias Baesken wrote: >> Currently we set _WIN32_WINNT at various places in the codebase; this is >> used to target a minimum Windows version we want to support. See also for >> more detailled information : >> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt >> Macros for Conditional Declarations >> "Certain functions that depend on a particular version of Windows are >> declared using conditional code. This enables you to use the compiler to >> detect whether your application uses functions that are not supported on its >> target version(s) of Windows." >> >> However currently we have quite a lot of differing settings of _WIN32_WINNT >> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible >> would make sense because we have this setting already at java_props_md.c >> (so targeting older Windows versions at other places is most likely not >> useful). > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust Copyright year in wepoll.c Interesting fact : we run now into this compile error : d:\a\jdk\jdk\jdk\src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp(1262): error C2065: 'NCRYPT_CIPHER_KEY_BLOB': undeclared identifier d:\a\jdk\jdk\jdk\src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp(1280): error C2065: 'NCRYPT_PROTECTED_KEY_BLOB': undeclared identifier Reason seems that already for some time ( at least OpenJDK 11 (!) ) we have an implicit minimum requirement of Windows 8 / Windows 2012 APIs for this code, so enforcing Win 7 is too old : https://docs.microsoft.com/en-us/windows/win32/api/ncrypt/nf-ncrypt-ncryptexportkey NCRYPT_PROTECTED_KEY_BLOB Export a protected key in a NCRYPT_KEY_BLOB_HEADER structure. Windows 8 and Windows Server 2012: Support for this value begins. NCRYPT_CIPHER_KEY_BLOB Export a cipher key in a NCRYPT_KEY_BLOB_HEADER structure. Windows 8 and Windows Server 2012: Support for this value begins. - PR: https://git.openjdk.java.net/jdk/pull/8428
Integrated: 8285452: Add a new test library API to replace a file content using FileUtils.java
On Fri, 22 Apr 2022 12:16:07 GMT, Sibabrata Sahoo wrote: > A new API to support replacing selective lines with desired content. This pull request has now been integrated. Changeset: 0462d5a2 Author:Sibabrata Sahoo URL: https://git.openjdk.java.net/jdk/commit/0462d5a25258458ec7f40d76b9d910ee529f3647 Stats: 163 lines in 2 files changed: 162 ins; 0 del; 1 mod 8285452: Add a new test library API to replace a file content using FileUtils.java Co-authored-by: Weijun Wang Reviewed-by: weijun, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/8360
