Re: RFR: 8282162: [vector] Optimize integral vector negation API [v3]
On Mon, 28 Mar 2022 09:56:22 GMT, Xiaohong Gong wrote: >> The current vector `"NEG"` is implemented with substraction a vector by zero >> in case the architecture does not support the negation instruction. And to >> fit the predicate feature for architectures that support it, the masked >> vector `"NEG" ` is implemented with pattern `"v.not(m).add(1, m)"`. They >> both can be optimized to a single negation instruction for ARM SVE. >> And so does the non-masked "NEG" for NEON. Besides, implementing the masked >> "NEG" with substraction for architectures that support neither negation >> instruction nor predicate feature can also save several instructions than >> the current pattern. >> >> To optimize the VectorAPI negation, this patch moves the implementation from >> Java side to hotspot. The compiler will generate different nodes according >> to the architecture: >> - Generate the (predicated) negation node if architecture supports it, >> otherwise, generate "`zero.sub(v)`" pattern for non-masked operation. >> - Generate `"zero.sub(v, m)"` for masked operation if the architecture >> does not have predicate feature, otherwise generate the original pattern >> `"v.xor(-1, m).add(1, m)"`. >> >> So with this patch, the following transformations are applied: >> >> For non-masked negation with NEON: >> >> moviv16.4s, #0x0 >> sub v17.4s, v16.4s, v17.4s ==> neg v17.4s, v17.4s >> >> and with SVE: >> >> mov z16.s, #0 >> sub z18.s, z16.s, z17.s ==> neg z16.s, p7/m, z16.s >> >> For masked negation with NEON: >> >> moviv17.4s, #0x1 >> mvn v19.16b, v18.16b >> mov v20.16b, v16.16b ==> neg v18.4s, v17.4s >> bsl v20.16b, v19.16b, v18.16b bsl v19.16b, v18.16b, v17.16b >> add v19.4s, v20.4s, v17.4s >> mov v18.16b, v16.16b >> bsl v18.16b, v19.16b, v20.16b >> >> and with SVE: >> >> mov z16.s, #-1 >> mov z17.s, #1==> neg z16.s, p0/m, z16.s >> eor z18.s, p0/m, z18.s, z16.s >> add z18.s, p0/m, z18.s, z17.s >> >> Here are the performance gains for benchmarks (see [1][2]) on ARM and x86 >> machines(note that the non-masked negation benchmarks do not have any >> improvement on X86 since no instructions are changed): >> >> NEON: >> BenchmarkGain >> Byte128Vector.NEG1.029 >> Byte128Vector.NEGMasked 1.757 >> Short128Vector.NEG 1.041 >> Short128Vector.NEGMasked 1.659 >> Int128Vector.NEG 1.005 >> Int128Vector.NEGMasked 1.513 >> Long128Vector.NEG1.003 >> Long128Vector.NEGMasked 1.878 >> >> SVE with 512-bits: >> BenchmarkGain >> ByteMaxVector.NEG1.10 >> ByteMaxVector.NEGMasked 1.165 >> ShortMaxVector.NEG 1.056 >> ShortMaxVector.NEGMasked 1.195 >> IntMaxVector.NEG 1.002 >> IntMaxVector.NEGMasked 1.239 >> LongMaxVector.NEG1.031 >> LongMaxVector.NEGMasked 1.191 >> >> X86 (non AVX-512): >> BenchmarkGain >> ByteMaxVector.NEGMasked 1.254 >> ShortMaxVector.NEGMasked 1.359 >> IntMaxVector.NEGMasked 1.431 >> LongMaxVector.NEGMasked 1.989 >> >> [1] >> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1881 >> [2] >> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1896 > > Xiaohong Gong has updated the pull request incrementally with one additional > commit since the last revision: > > Make "degenerate_vector_integral_negate" to be "NegVI" private Obvious performance improvement had ben observed on x86 for integral vector negation. So I think it's good to go. LGTM Thanks. Note: I didn't check the aarch64 code change. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19
On Mon, 28 Mar 2022 17:08:12 GMT, Lance Andersen wrote: > With this fix, I believe > [JDK-8282446](https://bugs.openjdk.java.net/browse/JDK-8282446) should also > be addressed. Thanks for mentioning this. I have uploaded another commit [41ae618e3a5ce696e3400a8654d98801226d7c1b](https://github.com/openjdk/jdk/pull/8000/commits/41ae618e3a5ce696e3400a8654d98801226d7c1b), which addresses this issue. - PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19 [v2]
> Please review this PR which is updating the ASM included in the JDK to ASM > 9.2. This version should be included in JDK19. There are basically two > commits here, one that was automatically generated and that mostly changes > file headers etc and another one, smaller, that make changes to the code to > adapt it to our needs, JDK version etc. In this second commit one can see > that two classes that were removed by the automatically generated change have > been copied back: > > jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter > jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter > > This is because package: `jdk.jfr.internal.instrument` needs them. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8000/files - new: https://git.openjdk.java.net/jdk/pull/8000/files/2e468674..41ae618e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8000=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8000=00-01 Stats: 83 lines in 2 files changed: 82 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8000/head:pull/8000 PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8283726: x86 intrinsics for compare method in Integer and Long
On Sun, 27 Mar 2022 06:15:34 GMT, Vamsi Parasa wrote:
> Implements x86 intrinsics for compare() method in java.lang.Integer and
> java.lang.Long.
src/hotspot/cpu/x86/x86_64.ad line 12107:
> 12105: instruct compareSignedI_rReg(rRegI dst, rRegI op1, rRegI op2, rRegI
> tmp, rFlagsReg cr)
> 12106: %{
> 12107: match(Set dst (CompareSignedI op1 op2));
Please also include these patterns in x86_32.ad
src/hotspot/cpu/x86/x86_64.ad line 12125:
> 12123: __ movl(tmp, 0);
> 12124: __ bind(done);
> 12125: __ movl(dest, tmp);
Please move this in macro-assembly routine.
src/hotspot/cpu/x86/x86_64.ad line 12178:
> 12176: __ movl(tmp, 0);
> 12177: __ bind(done);
> 12178: __ movl(dest, tmp);
Please move this into a macro-assembly routine.
src/hotspot/cpu/x86/x86_64.ad line 12204:
> 12202: __ movl(tmp, 0);
> 12203: __ bind(done);
> 12204: __ movl(dest, tmp);
Please move this into macro-assembly routine.
src/hotspot/share/classfile/vmIntrinsics.hpp line 239:
> 237: do_intrinsic(_compareUnsigned_i,java_lang_Integer,
> compare_unsigned_name, int2_int_signature, F_S) \
> 238:do_name( compare_unsigned_name,
> "compareUnsigned") \
> 239: do_intrinsic(_compareUnsigned_l,java_lang_Long,
> compare_unsigned_name, long2_int_signature, F_S) \
Creating these methods as intrinsic will create a box around the underneath
comparison logic, this shall prevent any regular constant folding which could
have optimized out certain control paths, I would suggest to to handle constant
folding for newly added nodes in associated Value routines.
src/hotspot/share/opto/comparenode.hpp line 67:
> 65: CompareUnsignedLNode(Node* in1, Node* in2) : CompareNode(in1, in2) {}
> 66: virtual int Opcode() const;
> 67: };
Intent here seems to be to enable further auto-vectorization of newly create IR
nodes.
test/micro/org/openjdk/bench/java/lang/CompareInteger.java line 78:
> 76: input2[i] = tmp;
> 77: }
> 78: }
Logic re-organization suggestion:-
for (int i = 0 ; i < BUFFER_SIZE; i++) {
input1[i] = rng.nextLong();
}
if (mode.equals("equals") {
GRADIANT = 0;
} else if (mode.equals("greaterThanEquals")) {
GRADIANT = 1;
} else {
assert mode.equals("lessThanEqual");
GRADIANT = -1;
}
for(int i = 0 ; i < BUFFER_SIZE; i++) {
input2[i] = input1[i] + i*GRADIANT;
}
test/micro/org/openjdk/bench/java/lang/CompareLong.java line 5:
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> 4: *
> 5: * This code is free software; you can redistribute it and/or modify it
We can unify this benchmark along with integer compare micro.
-
PR: https://git.openjdk.java.net/jdk/pull/7975
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Mon, 28 Mar 2022 10:55:30 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 refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > 8282648: Problems due to conflicting specification of Inflater::inflate(..) > and InflaterInputStream::read(..) Hello Volker, An additional thing that we might have to consider here is whether adding this javadoc change to `InflaterInputStream` is ever going to "show up" to end user applications. What I mean is, I think in many cases the end user applications won't even know they are dealing with an `InflaterInputStream`. For example, the following code:
Re: RFR: 8283715: Update ObjectStreamClass to be final
On Tue, 29 Mar 2022 01:07:33 GMT, Stuart Marks wrote: > Pretty much just what it says. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final
On Tue, 29 Mar 2022 01:07:33 GMT, Stuart Marks wrote: > Pretty much just what it says. Please review CSR request [JDK-8283811](https://bugs.openjdk.java.net/browse/JDK-8283811). - PR: https://git.openjdk.java.net/jdk/pull/8009
RFR: 8283715: Update ObjectStreamClass to be final
Pretty much just what it says. - Commit messages: - Make ObjectStreamClass final. Changes: https://git.openjdk.java.net/jdk/pull/8009/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8009=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283715 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8009/head:pull/8009 PR: https://git.openjdk.java.net/jdk/pull/8009
Integrated: 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS
On Fri, 18 Feb 2022 19:47:09 GMT, Ian Graves wrote: > Proposed change in behavior to correct inconsistencies between `\w` and `\b` > metacharacters This pull request has now been integrated. Changeset: f01cce23 Author:Ian Graves URL: https://git.openjdk.java.net/jdk/commit/f01cce235b62e378e91a3bae32942e2f3dfc5c7e Stats: 97 lines in 2 files changed: 79 ins; 6 del; 12 mod 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS Reviewed-by: lancea, bpb, naoto - PR: https://git.openjdk.java.net/jdk/pull/7539
Integrated: 8283806: [BACKOUT] JDK 19 L10n resource files update - msgdrop 10
On Mon, 28 Mar 2022 21:20:00 GMT, Alisen Chung wrote: > This reverts commit c0aecd15ae8d7abf37901f785fccaff2317c3b23. This pull request has now been integrated. Changeset: 634800a5 Author:Alisen Chung Committer: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/634800a536e7f9d148a4caa2663a60a2c5fc4929 Stats: 11316 lines in 139 files changed: 1252 ins; 9124 del; 940 mod 8283806: [BACKOUT] JDK 19 L10n resource files update - msgdrop 10 Reviewed-by: kcr, naoto - PR: https://git.openjdk.java.net/jdk/pull/8005
Re: RFR: 8283806: [BACKOUT] JDK 19 L10n resource files update - msgdrop 10
On Mon, 28 Mar 2022 21:20:00 GMT, Alisen Chung wrote: > This reverts commit c0aecd15ae8d7abf37901f785fccaff2317c3b23. LGTM - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8005
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Mon, 28 Mar 2022 21:06:35 GMT, Roger Riggs wrote: >> While it is true for completeness, I would limit the addition of new method >> as little as possible, because there are already several ways to obtain a >> Locale object. > > As with other varargs method calls, it is possible to pass an array with the > values. > I think it would be useful to describe the arguments as using the varargs > nomenclature and indicate > the values are in the array. For example, the `java.util.List.of(E... > elements)` method is explicit > about the array. Anther API using varargs EnumSet. Added the explanation following the `List.of(E... elements)` javadoc. - PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v6]
> Proposing to deprecate the constructors in the `java.util.Locale` class. > There is already a factory method and a builder to return singletons, so > there is no need to have constructors anymore unless one purposefully wants > to create `ill-formed` Locale objects, which is discouraged. We cannot > terminally deprecate those constructors for the compatibility to serialized > objects created with older JDKs. Please see the draft CSR for more detail. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added an @apiNote describing the single array argument - Changes: - all: https://git.openjdk.java.net/jdk/pull/7947/files - new: https://git.openjdk.java.net/jdk/pull/7947/files/86c7b586..b1d36b52 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7947=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7947=04-05 Stats: 4 lines in 2 files changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7947.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7947/head:pull/7947 PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8283800: Simplify String.indexOf/lastIndexOf calls
On Sun, 20 Mar 2022 12:45:34 GMT, Andrey Turbanov wrote: > In a few places String.indexOf/lastIndexOf methods are called with default > parameter for index: `0` for `indexOf`, length() for `lastIndexOf`. > I propose to cleanup such calls. It makes code a bit easier to read. In case > of `indexOf` it even could be faster, as there is separate intrinsic for > `indexOf` call without index argument. Marked as reviewed by lmesnik (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7877
RFR: 8283806: [BACKOUT] JDK 19 L10n resource files update - msgdrop 10
This reverts commit c0aecd15ae8d7abf37901f785fccaff2317c3b23. - Commit messages: - Revert "8280400: JDK 19 L10n resource files update - msgdrop 10" Changes: https://git.openjdk.java.net/jdk/pull/8005/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8005=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283806 Stats: 11316 lines in 139 files changed: 1252 ins; 9124 del; 940 mod Patch: https://git.openjdk.java.net/jdk/pull/8005.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8005/head:pull/8005 PR: https://git.openjdk.java.net/jdk/pull/8005
Re: RFR: 8283806: [BACKOUT] JDK 19 L10n resource files update - msgdrop 10
On Mon, 28 Mar 2022 21:20:00 GMT, Alisen Chung wrote: > This reverts commit c0aecd15ae8d7abf37901f785fccaff2317c3b23. I confirm that this is an exact backout of [JDK-8280400](https://bugs.openjdk.java.net/browse/JDK-8280400). - Marked as reviewed by kcr (Author). PR: https://git.openjdk.java.net/jdk/pull/8005
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Mon, 28 Mar 2022 16:00:14 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/util/Locale.java line 819:
>>
>>> 817: * @since 19
>>> 818: */
>>> 819: public static Locale of(String... fields) {
>>
>> Arguably, there should be `Locale.of` overloads taking 0 to 4 arguments, so
>> that it’s not necessary to box the fields in a `String` array.
>
> While it is true for completeness, I would limit the addition of new method
> as little as possible, because there are already several ways to obtain a
> Locale object.
As with other varargs method calls, it is possible to pass an array with the
values.
I think it would be useful to describe the arguments as using the varargs
nomenclature and indicate
the values are in the array. For example, the `java.util.List.of(E...
elements)` method is explicit
about the array. Anther API using varargs EnumSet.
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v16]
On Mon, 28 Mar 2022 18:09:29 GMT, Jim Laskey wrote: >> We propose to provide a runtime anonymous carrier class object generator; >> java.lang.runtime.Carrier. This generator class is designed to share >> anonymous classes when shapes are similar. For example, if several clients >> require objects containing two integer fields, then Carrier will ensure that >> each client generates carrier objects using the same underlying anonymous >> class. >> >> See JBS for details. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Clean up @return src/java.base/share/classes/java/lang/runtime/Carrier.java line 330: > 328: * Constructor. > 329: * > 330: * @param primitiveCount slot count required for primitives This isn’t the slot count, but the `long[]` array length, which is half the slot count for `long`s. - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8283801: Cleanup confusing String.toString calls
On Sun, 20 Mar 2022 13:20:31 GMT, Andrey Turbanov wrote: > String.toString() calls doesn't make much sense. Only one place, where it > could be used - to generate NPE. But in a few places of JDK codebase it's > called, even when NPE will happen anyway. > I propose to cleanup such places. > Found by IntelliJ IDEA inspection `Redundant 'String' operation`. Likewise, please add a `noreg-` label to the issue, perhaps `noreg-cleanup`. Otherwise fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7878
Re: RFR: 8283800: Simplify String.indexOf/lastIndexOf calls
On Sun, 20 Mar 2022 12:45:34 GMT, Andrey Turbanov wrote: > In a few places String.indexOf/lastIndexOf methods are called with default > parameter for index: `0` for `indexOf`, length() for `lastIndexOf`. > I propose to cleanup such calls. It makes code a bit easier to read. In case > of `indexOf` it even could be faster, as there is separate intrinsic for > `indexOf` call without index argument. Please add a noreg- label to the bug. Otherwise, looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7877
Re: RFR: 8283800: Simplify String.indexOf/lastIndexOf calls
On Sun, 20 Mar 2022 12:45:34 GMT, Andrey Turbanov wrote: > In a few places String.indexOf/lastIndexOf methods are called with default > parameter for index: `0` for `indexOf`, length() for `lastIndexOf`. > I propose to cleanup such calls. It makes code a bit easier to read. In case > of `indexOf` it even could be faster, as there is separate intrinsic for > `indexOf` call without index argument. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7877
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19
On Mon, 28 Mar 2022 17:50:25 GMT, Rémi Forax wrote: > I suppose that you are raising commons.RemappingMethodAdapter and > commons.RemappingAnnotationAdapter from the dead because you want to fix the > code in jdk.jfr.internal.instrument later ? correct, I would prefer the team maintaining jdk.jfr.internal.instrument to do that update after this PR has been pushed > > Otherwise it's a little dangerous because that code as started to drift, the > new remapper has new code not supported by the old remapper. BTW, @deprecated > on ASM source is equivalent to. @deprecated + forRemoval in the JDK. > > The support of Java 19 is fine, the same code will be included in ASM soon. - PR: https://git.openjdk.java.net/jdk/pull/8000
RFR: 8283801: Cleanup confusing String.toString calls
String.toString() calls doesn't make much sense. Only one place, where it could be used - to generate NPE. But in a few places of JDK codebase it's called, even when NPE will happen anyway. I propose to cleanup such places. Found by IntelliJ IDEA inspection `Redundant 'String' operation`. - Commit messages: - [PATCH] Cleanup confusing String.toString calls Changes: https://git.openjdk.java.net/jdk/pull/7878/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7878=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283801 Stats: 14 lines in 4 files changed: 0 ins; 4 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/7878.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7878/head:pull/7878 PR: https://git.openjdk.java.net/jdk/pull/7878
RFR: 8283800: Simplify String.indexOf/lastIndexOf calls
In a few places String.indexOf/lastIndexOf methods are called with default parameter for index: `0` for `indexOf`, length() for `lastIndexOf`. I propose to cleanup such calls. It makes code a bit easier to read. In case of `indexOf` it even could be faster, as there is separate intrinsic for `indexOf` call without index argument. - Commit messages: - [PATCH] Simplify String.indexOf/lastIndexOf calls Changes: https://git.openjdk.java.net/jdk/pull/7877/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7877=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283800 Stats: 14 lines in 6 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/7877.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7877/head:pull/7877 PR: https://git.openjdk.java.net/jdk/pull/7877
Re: RFR: 8282819: Deprecate Locale class constructors [v5]
On Mon, 28 Mar 2022 18:51:30 GMT, Naoto Sato wrote: >> Proposing to deprecate the constructors in the `java.util.Locale` class. >> There is already a factory method and a builder to return singletons, so >> there is no need to have constructors anymore unless one purposefully wants >> to create `ill-formed` Locale objects, which is discouraged. We cannot >> terminally deprecate those constructors for the compatibility to serialized >> objects created with older JDKs. Please see the draft CSR for more detail. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Refined test cases Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v4]
On Mon, 28 Mar 2022 17:13:44 GMT, Lance Andersen wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> New unit test. IllegalArgumentException.
>
> test/jdk/java/util/Locale/TestOf.java line 79:
>
>> 77: @Test (expectedExceptions = IllegalArgumentException.class)
>> 78: public void test_IAE() {
>> 79: Locale.of("en", "", "", "", "");
>
> I would consider using `assertThrows(IllegalArgumentException.class, () ->
> Locale.of("en", "", "", "", "")); ` instead of the expectedExceptions
> annotation element as it is the preferred way forward
Thanks. Modified as suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v5]
> Proposing to deprecate the constructors in the `java.util.Locale` class. > There is already a factory method and a builder to return singletons, so > there is no need to have constructors anymore unless one purposefully wants > to create `ill-formed` Locale objects, which is discouraged. We cannot > terminally deprecate those constructors for the compatibility to serialized > objects created with older JDKs. Please see the draft CSR for more detail. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Refined test cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/7947/files - new: https://git.openjdk.java.net/jdk/pull/7947/files/9d9d3a69..86c7b586 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7947=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7947=03-04 Stats: 9 lines in 1 file changed: 4 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7947.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7947/head:pull/7947 PR: https://git.openjdk.java.net/jdk/pull/7947
Integrated: 8280400: JDK 19 L10n resource files update - msgdrop 10
On Wed, 9 Mar 2022 21:09:30 GMT, Alisen Chung wrote: > msg drop for jdk19, Mar 9, 2022 This pull request has now been integrated. Changeset: c0aecd15 Author:Alisen Chung Committer: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/c0aecd15ae8d7abf37901f785fccaff2317c3b23 Stats: 11316 lines in 139 files changed: 9124 ins; 1252 del; 940 mod 8280400: JDK 19 L10n resource files update - msgdrop 10 Reviewed-by: naoto, kizune - PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Mon, 28 Mar 2022 17:21:40 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/zip/InflaterInputStream.java line 133:
>>
>>> 131: * Unlike the {@link InputStream#read(byte[],int,int) overridden
>>> method}
>>> 132: * of {@code InputStream}, this method might write more bytes than
>>> the returned
>>> 133: * number of inflated bytes into the buffer {@code b}.
>>
>> Hello Volker, I think this comment should be a bit more clear and state what
>> exactly it means by writing more bytes than the returned value.
>> Specifically, I think it should clearly state that even though it might
>> write additional data, it will however not write/change data starting and
>> beyond `off + len` index in the passed array.
>
> I think this require a bit of word smithing. If the return value is 'n' then
> it should make it clear that the values in elements b[off + n] to b[off + len
> - 1] are undefined, their values may or may have been changed by the
> inflate/read operation. For completeness, the expectation for when inflate
> fails should be specified too, the contents of b[off] to b[off + len - 1]
> will be undefined.
Hi Volker,
I believe you are heading in the right direction. I agree with Alan and Jai
that we could use a bit more clarification
-
PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On Mon, Mar 28, 2022 at 7:33 PM Alan Bateman wrote: > > On 28/03/2022 11:02, Volker Simonis wrote: > > : > > As I wrote before, the extra data written into the output buffer isn't > > sensitive because it can only originate from the history buffer (aka > > "sliding window"). Also, this data is already exposed today if the > > `Inflater` class is being used stand-alone, because in contrast to > > `InflaterInputStream::read(..)`, `Inflater::inflate(..)` doesn't > > guarantee to leave the content beyond the last read byte unaffected. > > Finally, the referenced zlib-chromium implementation with the > > mentioned behavior is the default zlib implementation in on Android > > and Chrome browsers. > If you are satisfied that flipping bits during an inflate operation > cannot never lead to something bad happening then okay. I'ts important > to ask about such as matters. > Sure. Thanks for your comments. > -Alan
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v16]
> We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Clean up @return - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/024d24cb..e577e16a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=14-15 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744 PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v15]
> We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Add carrierClass accessor - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/c5d03e82..024d24cb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=13-14 Stats: 30 lines in 2 files changed: 23 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744 PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19
On Mon, 28 Mar 2022 16:49:58 GMT, Vicente Romero wrote: > Please review this PR which is updating the ASM included in the JDK to ASM > 9.2. This version should be included in JDK19. There are basically two > commits here, one that was automatically generated and that mostly changes > file headers etc and another one, smaller, that make changes to the code to > adapt it to our needs, JDK version etc. In this second commit one can see > that two classes that were removed by the automatically generated change have > been copied back: > > jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter > jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter > > This is because package: `jdk.jfr.internal.instrument` needs them. > > TIA I suppose that you are raising commons.RemappingMethodAdapter and commons.RemappingAnnotationAdapter from the dead because you want to fix the code in jdk.jfr.internal.instrument later ? Otherwise it's a little dangerous because that code as started to drift, the new remapper has new code not supported by the old remapper. BTW, @deprecated on ASM source is equivalent to. @Deprecated + forRemoval in the JDK. The support of Java 19 is fine, the same code will be included in ASM soon. - PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On 28/03/2022 11:02, Volker Simonis wrote: : As I wrote before, the extra data written into the output buffer isn't sensitive because it can only originate from the history buffer (aka "sliding window"). Also, this data is already exposed today if the `Inflater` class is being used stand-alone, because in contrast to `InflaterInputStream::read(..)`, `Inflater::inflate(..)` doesn't guarantee to leave the content beyond the last read byte unaffected. Finally, the referenced zlib-chromium implementation with the mentioned behavior is the default zlib implementation in on Android and Chrome browsers. If you are satisfied that flipping bits during an inflate operation cannot never lead to something bad happening then okay. I'ts important to ask about such as matters. -Alan
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Mon, 28 Mar 2022 15:01:30 GMT, Jaikiran Pai wrote:
>> Volker Simonis has refreshed the contents of this pull request, and previous
>> commits have been removed. The incremental views will show differences
>> compared to the previous content of the PR. The pull request contains one
>> new commit since the last revision:
>>
>> 8282648: Problems due to conflicting specification of
>> Inflater::inflate(..) and InflaterInputStream::read(..)
>
> src/java.base/share/classes/java/util/zip/InflaterInputStream.java line 133:
>
>> 131: * Unlike the {@link InputStream#read(byte[],int,int) overridden
>> method}
>> 132: * of {@code InputStream}, this method might write more bytes than
>> the returned
>> 133: * number of inflated bytes into the buffer {@code b}.
>
> Hello Volker, I think this comment should be a bit more clear and state what
> exactly it means by writing more bytes than the returned value. Specifically,
> I think it should clearly state that even though it might write additional
> data, it will however not write/change data starting and beyond `off + len`
> index in the passed array.
I think this require a bit of word smithing. If the return value is 'n' then it
should make it clear that the values in elements b[off + n] to b[off + len - 1]
are undefined, their values may or may have been changed by the inflate/read
operation. For completeness, the expectation for when inflate fails should be
specified too, the contents of b[off] to b[off + len - 1] will be undefined.
-
PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282819: Deprecate Locale class constructors [v4]
On Mon, 28 Mar 2022 17:02:33 GMT, Naoto Sato wrote:
>> Proposing to deprecate the constructors in the `java.util.Locale` class.
>> There is already a factory method and a builder to return singletons, so
>> there is no need to have constructors anymore unless one purposefully wants
>> to create `ill-formed` Locale objects, which is discouraged. We cannot
>> terminally deprecate those constructors for the compatibility to serialized
>> objects created with older JDKs. Please see the draft CSR for more detail.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> New unit test. IllegalArgumentException.
Changes look fine.
I would suggest adding a comment describing the new tests. Also one additional
suggestion below
test/jdk/java/util/Locale/TestOf.java line 79:
> 77: @Test (expectedExceptions = IllegalArgumentException.class)
> 78: public void test_IAE() {
> 79: Locale.of("en", "", "", "", "");
I would consider using `assertThrows(IllegalArgumentException.class, () ->
Locale.of("en", "", "", "", "")); ` instead of the expectedExceptions
annotation element as it is the preferred way forward
-
Marked as reviewed by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19
On Mon, 28 Mar 2022 16:49:58 GMT, Vicente Romero wrote: > Please review this PR which is updating the ASM included in the JDK to ASM > 9.2. This version should be included in JDK19. There are basically two > commits here, one that was automatically generated and that mostly changes > file headers etc and another one, smaller, that make changes to the code to > adapt it to our needs, JDK version etc. In this second commit one can see > that two classes that were removed by the automatically generated change have > been copied back: > > jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter > jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter > > This is because package: `jdk.jfr.internal.instrument` needs them. > > TIA With this fix, I believe [JDK-8282446](https://bugs.openjdk.java.net/browse/JDK-8282446) should also be addressed. - PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8282819: Deprecate Locale class constructors [v4]
> Proposing to deprecate the constructors in the `java.util.Locale` class. > There is already a factory method and a builder to return singletons, so > there is no need to have constructors anymore unless one purposefully wants > to create `ill-formed` Locale objects, which is discouraged. We cannot > terminally deprecate those constructors for the compatibility to serialized > objects created with older JDKs. Please see the draft CSR for more detail. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: New unit test. IllegalArgumentException. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7947/files - new: https://git.openjdk.java.net/jdk/pull/7947/files/b4d040af..9d9d3a69 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7947=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7947=02-03 Stats: 87 lines in 2 files changed: 83 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7947.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7947/head:pull/7947 PR: https://git.openjdk.java.net/jdk/pull/7947
RFR: 8282508: Updating ASM to 9.2 for JDK 19
Please review this PR which is updating the ASM included in the JDK to ASM 9.2. This version should be included in JDK19. TIA - Commit messages: - additional adaptations after the automatic conversion - 8282508: Updating ASM to 9.2 for JDK 19 Changes: https://git.openjdk.java.net/jdk/pull/8000/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8000=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282508 Stats: 1762 lines in 136 files changed: 544 ins; 765 del; 453 mod Patch: https://git.openjdk.java.net/jdk/pull/8000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8000/head:pull/8000 PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8283774: TestZoneOffset::test_immutable should ignore ZoneOffset::rules
On Mon, 28 Mar 2022 10:35:04 GMT, Claes Redestad wrote: > - Add capability to ignore fields explicitly when checking for immutability > of classes in java.time > - Use this to avoid a test failure due the new `rules` cache in `ZoneOffset` > - Remove `TestZoneOffset` from problem list. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7989
Re: RFR: 8283781: Avoid allocating unused lastRulesCaches
On Mon, 28 Mar 2022 11:52:38 GMT, Claes Redestad wrote: > This address a minor inefficiency in that the `ZoneRules` of any `ZoneOffset` > (and some `ZoneRegion`s) allocate `lastRulesCache` unnecessarily. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7990
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Sun, 27 Mar 2022 08:45:01 GMT, ExE Boss wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed a build failure
>
> src/java.base/share/classes/java/util/Locale.java line 819:
>
>> 817: * @since 19
>> 818: */
>> 819: public static Locale of(String... fields) {
>
> Arguably, there should be `Locale.of` overloads taking 0 to 4 arguments, so
> that it’s not necessary to box the fields in a `String` array.
While it is true for completeness, I would limit the addition of new method as
little as possible, because there are already several ways to obtain a Locale
object.
> src/java.base/share/classes/java/util/Locale.java line 825:
>
>> 823: case 2 -> getInstance(fields[0], "", fields[1], "", null);
>> 824: case 3 -> getInstance(fields[0], "", fields[1], fields[2],
>> null);
>> 825: default -> getInstance(fields[0], fields[3], fields[1],
>> fields[2], null);
>
> This should probably throw `IllegalArgumentException` when more than 4 fields
> are passed:
> Suggestion:
>
> case 4 -> getInstance(fields[0], fields[3], fields[1], fields[2],
> null);
> default -> throw new IllegalArgumentException(/* TODO: message
> */);
Thanks for the suggestion. Will incorporate the exception in the spec/impl.
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Sat, 26 Mar 2022 11:30:53 GMT, Lance Andersen wrote: > One suggestion As part of the PR, we should probably update > test/jdk/java/util/Locale/LocaleTest.java. or perhaps add a new test for > Locale.of() for completeness. Thanks, Lance. Sure, I will add some new unit tests for the new method. - PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8283774: TestZoneOffset::test_immutable should ignore ZoneOffset::rules
On Mon, 28 Mar 2022 10:35:04 GMT, Claes Redestad wrote: > - Add capability to ignore fields explicitly when checking for immutability > of classes in java.time > - Use this to avoid a test failure due the new `rules` cache in `ZoneOffset` > - Remove `TestZoneOffset` from problem list. Update Copyright dates before push. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7989
Integrated: 8279488: ProcessBuilder inherits contextClassLoader when spawning a process reaper thread
On Tue, 18 Jan 2022 15:57:58 GMT, Roger Riggs wrote: > The thread factory used to create the process reaper threads unnecessarily > inherits the callers thread context classloader. > The result is retention of the class loader. > > The thread factory used for the pool of process reaper threads is modified to > use an InnocuousThread with a given stacksize. > The test verifies that the process reaper threads have a null context > classloader. This pull request has now been integrated. Changeset: f0282d7d Author:Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/f0282d7def8c043d95e9b86da926b7d45224c31c Stats: 109 lines in 3 files changed: 91 ins; 6 del; 12 mod 8279488: ProcessBuilder inherits contextClassLoader when spawning a process reaper thread Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/7131
Re: RFR: 8279488: ProcessBuilder inherits contextClassLoader when spawning a process reaper thread [v2]
On Fri, 18 Mar 2022 15:23:27 GMT, Roger Riggs wrote: >> The thread factory used to create the process reaper threads unnecessarily >> inherits the callers thread context classloader. >> The result is retention of the class loader. >> >> The thread factory used for the pool of process reaper threads is modified >> to use an InnocuousThread with a given stacksize. >> The test verifies that the process reaper threads have a null context >> classloader. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Folded lines to reduce max line length Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7131
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Mon, 28 Mar 2022 10:55:30 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 refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8282648: Problems due to conflicting specification of Inflater::inflate(..)
> and InflaterInputStream::read(..)
src/java.base/share/classes/java/util/zip/InflaterInputStream.java line 133:
> 131: * Unlike the {@link InputStream#read(byte[],int,int) overridden
> method}
> 132: * of {@code InputStream}, this method might write more bytes than
> the returned
> 133: * number of inflated bytes into the buffer {@code b}.
Hello Volker, I think this
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v14]
On Mon, 28 Mar 2022 13:11:37 GMT, Jim Laskey wrote: >> We propose to provide a runtime anonymous carrier class object generator; >> java.lang.runtime.Carrier. This generator class is designed to share >> anonymous classes when shapes are similar. For example, if several clients >> require objects containing two integer fields, then Carrier will ensure that >> each client generates carrier objects using the same underlying anonymous >> class. >> >> See JBS for details. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Update javadoc The API has been changed to ensure consistency between the constructor and components. A factory method `of(MethodType)` has been added that produces a `Carrier` instance that provides a consistent constructor and components unit. `of(Class...)` is also added, and `Carrier` instances are now cached. - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8279488: ProcessBuilder inherits contextClassLoader when spawning a process reaper thread [v2]
On Fri, 18 Mar 2022 15:23:27 GMT, Roger Riggs wrote: >> The thread factory used to create the process reaper threads unnecessarily >> inherits the callers thread context classloader. >> The result is retention of the class loader. >> >> The thread factory used for the pool of process reaper threads is modified >> to use an InnocuousThread with a given stacksize. >> The test verifies that the process reaper threads have a null context >> classloader. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Folded lines to reduce max line length @AlanBateman Please review and approve - PR: https://git.openjdk.java.net/jdk/pull/7131
Re: RFR: JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder [v2]
On Mon, 28 Mar 2022 11:44:56 GMT, Olga Mikhaltsova wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refactored ArgCheck test to be more readable and easier to maintain and >> backport > > @RogerRiggs when do you plan to merge this patch approximately? @omikhaltsova Its taken more time than expected to get the reviews needed. Perhaps in a week or so. - PR: https://git.openjdk.java.net/jdk/pull/7709
Re: RFR: 8283781: Avoid allocating unused lastRulesCaches
On Mon, 28 Mar 2022 11:52:38 GMT, Claes Redestad wrote: > This address a minor inefficiency in that the `ZoneRules` of any `ZoneOffset` > (and some `ZoneRegion`s) allocate `lastRulesCache` unnecessarily. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7990
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v14]
> We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Update javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/b74e204e..c5d03e82 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=12-13 Stats: 10 lines in 1 file changed: 2 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744 PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v13]
> We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Update Carrier API - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/7f8de3b5..b74e204e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=11-12 Stats: 233 lines in 2 files changed: 73 ins; 108 del; 52 mod Patch: https://git.openjdk.java.net/jdk/pull/7744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744 PR: https://git.openjdk.java.net/jdk/pull/7744
RFR: 8283781: Avoid allocating unused lastRulesCaches
This address a minor inefficiency in that the `ZoneRules` of any `ZoneOffset` (and some `ZoneRegion`s) allocate `lastRulesCache` unnecessarily. - Commit messages: - Merge branch 'master' into lastRulesCache - Avoid allocating unused lastRulesCaches Changes: https://git.openjdk.java.net/jdk/pull/7990/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7990=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283781 Stats: 16 lines in 1 file changed: 9 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7990.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7990/head:pull/7990 PR: https://git.openjdk.java.net/jdk/pull/7990
Re: RFR: JDK-8282008: Incorrect handling of quoted arguments in ProcessBuilder [v2]
On Tue, 15 Mar 2022 21:11:31 GMT, Roger Riggs wrote: >> Quoting related changes in https://bugs.openjdk.java.net/browse/JDK-8250568 >> modified the way that >> process builder recognized argument strings, causing some arguments to be >> doubly quoted and malformed. >> >> ProcessBuilder encodes command arguments in two ways, a looser legacy >> encoding >> and stricter encoding that prevents quotes from being misinterpreted. >> The system property jdk.lang.Process.allowAmbiguousCommands controls which >> is in effect. >> >> When the property is "true" or not set, arguments are inserted into the >> Windows command line >> with minimal changes. Arguments containing space or tab are quoted to >> prevent them being split. >> Arguments that start and end with double-quote are left alone. >> Some executables interpret a backslash before the final quote as an escape; >> if the argument >> contains first and last quotes, backslashes are ignored. >> >> When the allowAmbigousCommands property is `false`, care is taken to ensure >> that >> the final quote of an argument is the closing quote for the argument and is >> not >> interpreted as a literal quote by a preceding quote (or an odd number of >> quotes). >> >> The PR includes a test matrix of the cases where an argument with spaces and >> a final backslash >> is passed with each combination of `allowAmbiguousCommands = true and false`, >> launched executable, java, .cmd, and .vbs and when the argument is >> surrounded with double-quotes. >> >> The priority for allowAmbiguousCommands = false is that no argument is split >> or joined to another argument. >> In some cases, backslashes are doubled to prevent a double-quote from being >> interpreted incorrectly. >> The trailing backslash in an argument occurs rarely exception when the >> argument is a directory. >> In that case, the addition of trailing backslashes is benign when the string >> is used as a filesystem path. >> >> See also PR#7504, for background and a proposal. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Refactored ArgCheck test to be more readable and easier to maintain and > backport @RogerRiggs when do you plan to merge this patch approximately? - PR: https://git.openjdk.java.net/jdk/pull/7709
Integrated: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. This pull request has now been integrated. Changeset: 85672667 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/8567266795cd1171f5b353d0e0813e7eeff319c2 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8283683: Make ThreadLocalRandom a final class Reviewed-by: smarks, chegar - PR: https://git.openjdk.java.net/jdk/pull/7958
Re: RFR: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. Thank you everyone for the reviews of the PR and the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7958
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On Mon, 28 Mar 2022 10:18:32 GMT, Lance Andersen wrote: > Hi Volker, > > I believe your PR should point to the [JBS > issue](https://bugs.openjdk.java.net/browse/JDK-8282648) in the title, which > references the CSR and not the CSR directly in the title. Sorry, you're right of course :) - PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
> 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) - Changes: - all: https://git.openjdk.java.net/jdk/pull/7986/files - new: https://git.openjdk.java.net/jdk/pull/7986/files/128166ff..b55fc332 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7986=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7986=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 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:
RFR: 8283774: TestZoneOffset::test_immutable should ignore ZoneOffset::rules
- Add capability to ignore fields explicitly when checking for immutability of classes in java.time - Use this to avoid a test failure due the new `rules` cache in `ZoneOffset` - Remove `TestZoneOffset` from problem list. - Commit messages: - TestZoneOffset::test_immutable should ignore ZoneOffset::rules Changes: https://git.openjdk.java.net/jdk/pull/7989/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7989=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283774 Stats: 12 lines in 3 files changed: 6 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7989.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7989/head:pull/7989 PR: https://git.openjdk.java.net/jdk/pull/7989
Re: RFR: 8283758: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On Mon, 28 Mar 2022 09:37:58 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)). Hi Volker, I believe your PR should point to the [JBS issue](https://bugs.openjdk.java.net/browse/JDK-8282648) in the title, which references the CSR and not the CSR directly in the title. - PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On Mon, Mar 28, 2022 at 10:53 AM Alan Bateman wrote: > > On 22/03/2022 12:28, Volker Simonis wrote: > > : > > I don't really understand this concern? Do you mean what happens if > > another thread is changing the content of the output buffer during an > > inflate? I think such a use case has never been well-defined and > > amending the specification won't change anything for such a situation. > The setup means that user code has access to temporary storage used by > the inflater library. It's important that nothing sensitive leaks, also > important that flipping bits in any of the bytes in that temporary > buffer doesn't lead to something that is considered a security issue. If > you are confident that nothing bad can happen they great, I'm just > pointing out things to consider when allow for the behavior discussed here. As I wrote before, the extra data written into the output buffer isn't sensitive because it can only originate from the history buffer (aka "sliding window"). Also, this data is already exposed today if the `Inflater` class is being used stand-alone, because in contrast to `InflaterInputStream::read(..)`, `Inflater::inflate(..)` doesn't guarantee to leave the content beyond the last read byte unaffected. Finally, the referenced zlib-chromium implementation with the mentioned behavior is the default zlib implementation in on Android and Chrome browsers. I've created a pull request and an associated CSR for this issue under: https://github.com/openjdk/jdk/pull/7986 https://bugs.openjdk.java.net/browse/JDK-8283758 Finally I've also created a PR to fix the ZipFSOutputStreamTest mentioned in my initial mail: https://github.com/openjdk/jdk/pull/7984 Can you please kindly take a look and review? Thank you and best regards, Volker PS: just saw you've already approved https://github.com/openjdk/jdk/pull/7984 :) > > -Alan
Re: RFR: 8282162: [vector] Optimize integral vector negation API [v3]
> The current vector `"NEG"` is implemented with substraction a vector by zero > in case the architecture does not support the negation instruction. And to > fit the predicate feature for architectures that support it, the masked > vector `"NEG" ` is implemented with pattern `"v.not(m).add(1, m)"`. They both > can be optimized to a single negation instruction for ARM SVE. > And so does the non-masked "NEG" for NEON. Besides, implementing the masked > "NEG" with substraction for architectures that support neither negation > instruction nor predicate feature can also save several instructions than the > current pattern. > > To optimize the VectorAPI negation, this patch moves the implementation from > Java side to hotspot. The compiler will generate different nodes according to > the architecture: > - Generate the (predicated) negation node if architecture supports it, > otherwise, generate "`zero.sub(v)`" pattern for non-masked operation. > - Generate `"zero.sub(v, m)"` for masked operation if the architecture does > not have predicate feature, otherwise generate the original pattern > `"v.xor(-1, m).add(1, m)"`. > > So with this patch, the following transformations are applied: > > For non-masked negation with NEON: > > moviv16.4s, #0x0 > sub v17.4s, v16.4s, v17.4s ==> neg v17.4s, v17.4s > > and with SVE: > > mov z16.s, #0 > sub z18.s, z16.s, z17.s ==> neg z16.s, p7/m, z16.s > > For masked negation with NEON: > > moviv17.4s, #0x1 > mvn v19.16b, v18.16b > mov v20.16b, v16.16b ==> neg v18.4s, v17.4s > bsl v20.16b, v19.16b, v18.16b bsl v19.16b, v18.16b, v17.16b > add v19.4s, v20.4s, v17.4s > mov v18.16b, v16.16b > bsl v18.16b, v19.16b, v20.16b > > and with SVE: > > mov z16.s, #-1 > mov z17.s, #1==> neg z16.s, p0/m, z16.s > eor z18.s, p0/m, z18.s, z16.s > add z18.s, p0/m, z18.s, z17.s > > Here are the performance gains for benchmarks (see [1][2]) on ARM and x86 > machines(note that the non-masked negation benchmarks do not have any > improvement on X86 since no instructions are changed): > > NEON: > BenchmarkGain > Byte128Vector.NEG1.029 > Byte128Vector.NEGMasked 1.757 > Short128Vector.NEG 1.041 > Short128Vector.NEGMasked 1.659 > Int128Vector.NEG 1.005 > Int128Vector.NEGMasked 1.513 > Long128Vector.NEG1.003 > Long128Vector.NEGMasked 1.878 > > SVE with 512-bits: > BenchmarkGain > ByteMaxVector.NEG1.10 > ByteMaxVector.NEGMasked 1.165 > ShortMaxVector.NEG 1.056 > ShortMaxVector.NEGMasked 1.195 > IntMaxVector.NEG 1.002 > IntMaxVector.NEGMasked 1.239 > LongMaxVector.NEG1.031 > LongMaxVector.NEGMasked 1.191 > > X86 (non AVX-512): > BenchmarkGain > ByteMaxVector.NEGMasked 1.254 > ShortMaxVector.NEGMasked 1.359 > IntMaxVector.NEGMasked 1.431 > LongMaxVector.NEGMasked 1.989 > > [1] > https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1881 > [2] > https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1896 Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision: Make "degenerate_vector_integral_negate" to be "NegVI" private - Changes: - all: https://git.openjdk.java.net/jdk/pull/7782/files - new: https://git.openjdk.java.net/jdk/pull/7782/files/97c8119a..48f4d6be Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7782=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7782=01-02 Stats: 15 lines in 2 files changed: 6 ins; 1 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7782.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7782/head:pull/7782 PR: https://git.openjdk.java.net/jdk/pull/7782
RFR: 8283758: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
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)). - Commit messages: - 8283758: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) Changes: https://git.openjdk.java.net/jdk/pull/7986/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7986=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283758 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 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: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops [v2]
On Fri, 4 Mar 2022 17:44:44 GMT, Ludovic Henry wrote: >> Despite the hash value being cached for Strings, computing the hash still >> represents a significant CPU usage for applications handling lots of text. >> >> Even though it would be generally better to do it through an enhancement to >> the autovectorizer, the complexity of doing it by hand is trivial and the >> gain is sizable (2x speedup) even without the Vector API. The algorithm has >> been proposed by Richard Startin and Paul Sandoz [1]. >> >> Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` >> >> >> Benchmark(size) Mode Cnt Score >>Error Units >> StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 >> ± 0.210 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 >> ± 0.127 ns/op >> StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 >> ± 0.099 ns/op >> StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 >> ± 0.444 ns/op >> StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 >> ± 0.846 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 >> ± 4.071 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 >> ± 0.092 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 >> ± 0.159 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 >> ± 1.218 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 >> ± 1.225 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 >> ± 14.621 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 >> ± 0.097 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 >> ± 0.158 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 >> ± 0.065 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 >> ± 0.251 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 >> ± 1.667 ns/op >> StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 >> ± 0.191 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 >> ± 0.362 ns/op >> StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 >> ± 0.112 ns/op >> StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 >> ± 0.702 ns/op >> StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 >> ± 3.290 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 >> ± 43.847 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 >> ± 0.038 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 >> ± 0.422 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 >> ± 0.178 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 >> ± 0.089 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 >> ± 1.834 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 >> ± 2.927 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 >> ± 0.396 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 >> ± 0.189 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 >> ± 0.340 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 >> ± 2.699 ns/op >> >> >> At Datadog, we handle a great amount of text (through logs management for >> example), and hashing String represents a large part of our CPU usage. It's >> very unlikely that we are the only one as String.hashCode is such a core >> feature of the JVM-based languages with its use in HashMap for example. >> Having even only a 2x speedup would allow us to save thousands of CPU cores >> per month and improve correspondingly the energy/carbon impact. >> >> [1] >> https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf > > Ludovic Henry has updated the pull request incrementally with one additional > commit since
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On 22/03/2022 12:28, Volker Simonis wrote: : I don't really understand this concern? Do you mean what happens if another thread is changing the content of the output buffer during an inflate? I think such a use case has never been well-defined and amending the specification won't change anything for such a situation. The setup means that user code has access to temporary storage used by the inflater library. It's important that nothing sensitive leaks, also important that flipping bits in any of the bytes in that temporary buffer doesn't lead to something that is considered a security issue. If you are confident that nothing bad can happen they great, I'm just pointing out things to consider when allow for the behavior discussed here. -Alan
Re: RFR: 8282162: [vector] Optimize vector negation API
On Tue, 15 Mar 2022 02:47:20 GMT, Xiaohong Gong wrote:
> > Note that in terms of Java semantics, negation of floating point values
> > needs to be implemented as subtraction from negative zero rather than
> > positive zero:
> > double negate(double arg) {return -0.0 - arg; }
> > This is to handle signed zeros correctly.
>
> Hi @jddarcy ,thanks for looking at this PR and thanks for the notes on the
> floating point negation! Yeah, this really makes sense to me. Kindly note
> that this patch didn't touch the negation of the floating point values. For
> Vector API, the vector floating point negation has been intrinsified to
> `NegVF/D` node by compiler that we directly generate the negation
> instructions for them. Thanks!
I would suggest changing the JBS title like `[vector] Optimize non-floating
vector negation API` .
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API [v2]
On Mon, 28 Mar 2022 07:43:29 GMT, Jie Fu wrote:
>> Xiaohong Gong has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add a superclass for vector negation
>
> src/hotspot/share/opto/vectornode.cpp line 1592:
>
>> 1590:
>> 1591: // Generate other vector nodes to implement the masked/non-masked
>> vector negation.
>> 1592: Node* VectorNode::degenerate_vector_integral_negate(Node* n, int vlen,
>> BasicType bt, PhaseGVN* phase, bool is_predicated) {
>
> Shall we move this declaration in `class NegVNode` since it is only used by
> NegVNode::Ideal ?
I think it can be. Thanks for pointing out this. I will change this later.
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API [v2]
On Tue, 22 Mar 2022 09:58:23 GMT, Xiaohong Gong wrote:
>> The current vector `"NEG"` is implemented with substraction a vector by zero
>> in case the architecture does not support the negation instruction. And to
>> fit the predicate feature for architectures that support it, the masked
>> vector `"NEG" ` is implemented with pattern `"v.not(m).add(1, m)"`. They
>> both can be optimized to a single negation instruction for ARM SVE.
>> And so does the non-masked "NEG" for NEON. Besides, implementing the masked
>> "NEG" with substraction for architectures that support neither negation
>> instruction nor predicate feature can also save several instructions than
>> the current pattern.
>>
>> To optimize the VectorAPI negation, this patch moves the implementation from
>> Java side to hotspot. The compiler will generate different nodes according
>> to the architecture:
>> - Generate the (predicated) negation node if architecture supports it,
>> otherwise, generate "`zero.sub(v)`" pattern for non-masked operation.
>> - Generate `"zero.sub(v, m)"` for masked operation if the architecture
>> does not have predicate feature, otherwise generate the original pattern
>> `"v.xor(-1, m).add(1, m)"`.
>>
>> So with this patch, the following transformations are applied:
>>
>> For non-masked negation with NEON:
>>
>> moviv16.4s, #0x0
>> sub v17.4s, v16.4s, v17.4s ==> neg v17.4s, v17.4s
>>
>> and with SVE:
>>
>> mov z16.s, #0
>> sub z18.s, z16.s, z17.s ==> neg z16.s, p7/m, z16.s
>>
>> For masked negation with NEON:
>>
>> moviv17.4s, #0x1
>> mvn v19.16b, v18.16b
>> mov v20.16b, v16.16b ==> neg v18.4s, v17.4s
>> bsl v20.16b, v19.16b, v18.16b bsl v19.16b, v18.16b, v17.16b
>> add v19.4s, v20.4s, v17.4s
>> mov v18.16b, v16.16b
>> bsl v18.16b, v19.16b, v20.16b
>>
>> and with SVE:
>>
>> mov z16.s, #-1
>> mov z17.s, #1==> neg z16.s, p0/m, z16.s
>> eor z18.s, p0/m, z18.s, z16.s
>> add z18.s, p0/m, z18.s, z17.s
>>
>> Here are the performance gains for benchmarks (see [1][2]) on ARM and x86
>> machines(note that the non-masked negation benchmarks do not have any
>> improvement on X86 since no instructions are changed):
>>
>> NEON:
>> BenchmarkGain
>> Byte128Vector.NEG1.029
>> Byte128Vector.NEGMasked 1.757
>> Short128Vector.NEG 1.041
>> Short128Vector.NEGMasked 1.659
>> Int128Vector.NEG 1.005
>> Int128Vector.NEGMasked 1.513
>> Long128Vector.NEG1.003
>> Long128Vector.NEGMasked 1.878
>>
>> SVE with 512-bits:
>> BenchmarkGain
>> ByteMaxVector.NEG1.10
>> ByteMaxVector.NEGMasked 1.165
>> ShortMaxVector.NEG 1.056
>> ShortMaxVector.NEGMasked 1.195
>> IntMaxVector.NEG 1.002
>> IntMaxVector.NEGMasked 1.239
>> LongMaxVector.NEG1.031
>> LongMaxVector.NEGMasked 1.191
>>
>> X86 (non AVX-512):
>> BenchmarkGain
>> ByteMaxVector.NEGMasked 1.254
>> ShortMaxVector.NEGMasked 1.359
>> IntMaxVector.NEGMasked 1.431
>> LongMaxVector.NEGMasked 1.989
>>
>> [1]
>> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1881
>> [2]
>> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1896
>
> Xiaohong Gong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add a superclass for vector negation
src/hotspot/share/opto/vectornode.cpp line 1592:
> 1590:
> 1591: // Generate other vector nodes to implement the masked/non-masked
> vector negation.
> 1592: Node* VectorNode::degenerate_vector_integral_negate(Node* n, int vlen,
> BasicType bt, PhaseGVN* phase, bool is_predicated) {
Shall we move this declaration in `class NegVNode` since it is only used by
NegVNode::Ideal ?
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API [v2]
On Mon, 28 Mar 2022 07:40:48 GMT, Jie Fu wrote:
>> The compiler can get the real type info from `Op_NegVI` that can also handle
>> the `BYTE ` and `SHORT ` basic type. I just don't want to add more new IRs
>> which also need more match rules in the ad files.
>>
>>> Is there any performance drop for byte/short negation operation if both of
>>> them are handled as a NegVI vector?
>>
>> From the benchmark results I showed in the commit message, I didn't see not
>> any performance drop for byte/short. Thanks!
>
>> The compiler can get the real type info from `Op_NegVI` that can also handle
>> the `BYTE ` and `SHORT ` basic type. I just don't want to add more new IRs
>> which also need more match rules in the ad files.
>>
>> > Is there any performance drop for byte/short negation operation if both of
>> > them are handled as a NegVI vector?
>>
>> From the benchmark results I showed in the commit message, I didn't see not
>> any performance drop for byte/short. Thanks!
>
> There seems no vectorized negation instructions for {byte, short, int, long}
> on x86, so this should be fine on x86.
> I tested the patch on x86 and the performance number looks good.
Thanks for doing this! Yeah, I think the performance for masked negation
operations might improve on non avx-512 systems.
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API [v2]
On Mon, 21 Mar 2022 01:19:57 GMT, Xiaohong Gong wrote:
> The compiler can get the real type info from `Op_NegVI` that can also handle
> the `BYTE ` and `SHORT ` basic type. I just don't want to add more new IRs
> which also need more match rules in the ad files.
>
> > Is there any performance drop for byte/short negation operation if both of
> > them are handled as a NegVI vector?
>
> From the benchmark results I showed in the commit message, I didn't see not
> any performance drop for byte/short. Thanks!
There seems no vectorized negation instructions for {byte, short, int, long} on
x86, so this should be fine on x86.
I tested the patch on x86 and the performance number looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/7782
