Re: RFR: 8283060: RawNativeLibraries should allow multiple clients to load/unload the same library
On Tue, 29 Mar 2022 20:05:50 GMT, Mandy Chung wrote: > A small improvement to `RawNativeLibraries`. `RawNativeLibraries::load` > returns the same `NativeLibrary` instance of a given path if it's called > multiple times. This means that multiple clients have to coordinate that the > last one using the loaded library is the one to close the library by calling > `RawNativeLibraries::unload`; otherwise, an exception may be thrown. > > This patch changes `RawNativeLibraries::load` to delegate to the underlying > platform-specific library loading mechanism e.g. dlopen/dlclose that keeps > the reference count. So each call to `RawNativeLibraries::load` and `unload` > is simply a call to dlopen and dlclose respectively. Each client of calling > `RawNativeLibraries::load` is responsible for calling > `RawNativeLibraries::unload`. This will be consistent with the current > behavior when you call `load` and `unload` with a different > `RawNativeLibraries` instance. LGTM - Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8022
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19 [v3]
> 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: fixing files missing new line at the end - Changes: - all: https://git.openjdk.java.net/jdk/pull/8000/files - new: https://git.openjdk.java.net/jdk/pull/8000/files/41ae618e..f5e0d931 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8000=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8000=01-02 Stats: 252 lines in 129 files changed: 125 ins; 0 del; 127 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: 8240903: Add test to check that jmod hashes are reproducible [v5]
> This creates a regression test for > [JDK-8240734](https://bugs.openjdk.java.net/browse/JDK-8240734). This was > once blocked > by a time stamp issue which has been resolved by: [JDK-8276766 > ](https://bugs.openjdk.java.net/browse/JDK-8276766) > > We found the issue can be produced stably with at least 64 modules. > Note that we need to add the --date jmod option to avoid the timestamp issue. > > Testing: > Added test case fails without fix for JDK-8240734, and passes otherwise. > Tested with tier2 on linux x86. Dongbo He has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'master' into 8240903 - Delete extra blank links - Change shell test io java test - Get date by 'date +%Y-%m-%dT%H:%M:%S%:z' - Update Copyright - 8240903: Add a regression test for JDK-8240734 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7948/files - new: https://git.openjdk.java.net/jdk/pull/7948/files/6b185934..34b8008d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7948=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7948=03-04 Stats: 3635 lines in 166 files changed: 2489 ins; 556 del; 590 mod Patch: https://git.openjdk.java.net/jdk/pull/7948.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7948/head:pull/7948 PR: https://git.openjdk.java.net/jdk/pull/7948
Integrated: 8282162: [vector] Optimize integral vector negation API
On Fri, 11 Mar 2022 06:29: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 This pull request has now been integrated. Changeset: d0668568 Author:Xiaohong Gong Committer: Jie Fu URL: https://git.openjdk.java.net/jdk/commit/d06685680c17583d56dc3d788d9a2ecea8812bc8 Stats: 325 lines in 15 files changed: 275 ins; 25 del; 25 mod 8282162: [vector] Optimize integral vector negation API Reviewed-by: jiefu, psandoz, njian - PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize integral vector negation API [v3]
On Tue, 29 Mar 2022 18:05:56 GMT, Paul Sandoz wrote: >> 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 > > Java changes are good. Thanks for the review @PaulSandoz @nsjian ! - PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8240903: Add a regression test for JDK-8240734 [v3]
On Fri, 25 Mar 2022 11:54:52 GMT, Alan Bateman wrote: > The existing tests for the jmod tool are in test/jdk/tools/jmod. > HashesTest.java might provide inspiration to avoid introducing a shell test. Hi, Alan I have rewritten this test in Java in the latest commit. - PR: https://git.openjdk.java.net/jdk/pull/7948
Re: RFR: 8240903: Add a regression test for JDK-8240734 [v4]
> This creates a regression test for > [JDK-8240734](https://bugs.openjdk.java.net/browse/JDK-8240734). This was > once blocked > by a time stamp issue which has been resolved by: [JDK-8276766 > ](https://bugs.openjdk.java.net/browse/JDK-8276766) > > We found the issue can be produced stably with at least 64 modules. > Note that we need to add the --date jmod option to avoid the timestamp issue. > > Testing: > Added test case fails without fix for JDK-8240734, and passes otherwise. > Tested with tier2 on linux x86. Dongbo He has updated the pull request incrementally with one additional commit since the last revision: Change shell test io java test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7948/files - new: https://git.openjdk.java.net/jdk/pull/7948/files/aea79a40..6b185934 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7948=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7948=02-03 Stats: 279 lines in 4 files changed: 136 ins; 142 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7948.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7948/head:pull/7948 PR: https://git.openjdk.java.net/jdk/pull/7948
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 AArch64 changes look good to me. - Marked as reviewed by njian (Committer). PR: https://git.openjdk.java.net/jdk/pull/7782
RFR: 8283805: [REDO] JDK 19 L10n resource files update - msgdrop 10
redo of 8280400 - Commit messages: - 8280400: JDK 19 L10n resource files update - msgdrop 10 Changes: https://git.openjdk.java.net/jdk/pull/8026/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8026=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283805 Stats: 11316 lines in 139 files changed: 9124 ins; 1252 del; 940 mod Patch: https://git.openjdk.java.net/jdk/pull/8026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8026/head:pull/8026 PR: https://git.openjdk.java.net/jdk/pull/8026
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Hello Stuart, the test change looks fine to me. Just a minor note - the copyright year of the test will need an update. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8253569: javax.xml.catalog.Catalog.matchURI() implementation should reset state variables [v2]
On Tue, 29 Mar 2022 21:41:52 GMT, Naoto Sato wrote: >> Joe Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove variable, check instance instead. > > src/java.xml/share/classes/javax/xml/catalog/GroupEntry.java line 44: > >> 42: >> 43: //Indicates whether this is the Catalog instance (vs a Catalog entry) >> 44: boolean isCatalogInstance = false; > > Can we eliminate this instance variable, by checking whether the type is > `Catalog` or not in `resetOnStart()`? Yes, thanks! - PR: https://git.openjdk.java.net/jdk/pull/8018
Re: RFR: 8253569: javax.xml.catalog.Catalog.matchURI() implementation should reset state variables [v2]
> Resets state of a Catalog instance on each matching call so that it can be > reused. Adjusts CatalogResolver's resolve routine so that it continues to > observes the PREFER feature in a resolution process. Without the adjustment, > PreferFeatureTest would fail. > > Mach5 XML tests passed: > https://mach5.us.oracle.com/mdash/jobs/huizwang-open-20220329-0148-30563542 Joe Wang has updated the pull request incrementally with one additional commit since the last revision: remove variable, check instance instead. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8018/files - new: https://git.openjdk.java.net/jdk/pull/8018/files/14513d7b..16bebbcf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8018=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8018=00-01 Stats: 6 lines in 2 files changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8018.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8018/head:pull/8018 PR: https://git.openjdk.java.net/jdk/pull/8018
Re: RFR: 8186958: Need method to create pre-sized HashMap [v7]
On Thu, 24 Mar 2022 17:43:31 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with two additional > commits since the last revision: > > - update jmh > - refine javadoc; refine implement when expectedSize < 0 OK, finally got some time to look at this. Here's a rewrite of the spec words, at least for HashMap::newHashMap. If this settles down, I'll write the CSR for this and LHM and WHM. /** * Creates a new, empty HashMap suitable for the expected number of mappings. * The returned map uses the default load factor of 0.75, and its initial capacity is * generally large enough so that the expected number of mappings can be added * without resizing the map. * * @param numMappings the expected number of mappings * @param the type of keys maintained by this map * @param the type of mapped values * @return the newly created map * @throws IllegalArgumentException if numMappings is negative * @since 19 */ The original wording was taken from CHM, which generally is a reasonable thing to do. Unfortunately, it's wrong. :-) "Table size" isn't accurate; HashMap uses "capacity" as the term for the number of buckets (== length of the internal table array). "Size" means "number of mappings" so its use of "table size" confuses these two concepts. The CHM wording also uses "elements" which applies to linear collections; the things inside a map are usually referred to as "mappings" or "entries". (I guess we should fix up CHM at some point too.) While "expectedSize" isn't inaccurate, it's not tied to the main text, so I've renamed it to numMappings. There are a couple other javadoc style rules operating here. The first sentence is generally a sentence fragment that is short and descriptive, as it will be pulled into a summary table. (It's often written as if it were a sentence that begins "This method..." but those words are elided.) Details are in the rest of the first paragraph. The text for `@param`, `@return`, and `@throws` are generally also sentence fragments, and they have no initial capital letter nor trailing period. -- On performance and benchmarking: this is a distraction from the main point of this effort, which is to add an API that allows callers a correct and convenient way to create a properly-sized HashMap. Any work spent on optimizing performance is almost certainly wasted. First, the benchmark: it's entirely unclear what this is measuring. It's performing the operation 2^31 times, but it sends the result to a black hole so that the JIT doesn't eliminate the computation. One of the actual results is 0.170 ops/sec. This includes both the operation and the black hole, so we don't actually have any idea what that result represents. _Maybe_ it's possible to infer some idea of relative performance of the different operations by comparing the results. It's certainly plausible that the integer code is faster than the float or double code. But the benchmark doesn't tell us how long the actual computation takes. Second, how significant is the cost of the computation? I'll assert that it's insignificant. The table length is computed once at HashMap creation time, and it's amortized over the addition of all the initial entries and subsequent queries and updates to the HashMap. Any of the computations (whether integer or floating point) are a handful of nanoseconds. This will be swamped by the first hashCode computation that causes a cache miss. Third: I'll stipulate that the integer computation is probably a few ns faster than the floating-point computation. But the computation is entirely non-obvious. To make up for it being non-obvious, there's a big comment that explains it all. It's not worth doing something that increases performance by an insignificant amount that also requires a big explanation. Finally, note that most of the callers are already doing a floating-point computation to compute the desired capacity, and it doesn't seem to be a problem. Sorry, you probably spent a bunch of time on this already, but trying to optimize the performance here just isn't worthwhile. Let's please just stick with our good old `(int) Math.ceil(numMappings / 0.75)`. -- There should be regression tests added for the three new methods. I haven't thought much about this. It might be possible to reuse some of the infrastructure in the WhiteBoxResizeTest we worked on previously. -- I think it would be good to include updates to some of the use sites in this PR. It's often useful to put new APIs into practice. One usually learns something from the effort. Even though this is a really simple API, looking at use sites can illuminating, to see how the code reads. This might affect the method name, for example. You don't need to update all of the use sites in the JDK, but it would be good to choose a
Re: RFR: 8283726: x86 intrinsics for compare method in Integer and Long
On Tue, 29 Mar 2022 21:56:18 GMT, Vamsi Parasa wrote: >> This is both complicated and inefficient, I would suggest building the >> intrinsic in the IR graph so that the compiler can simplify >> `Integer.compareUnsigned(x, y) < 0` into `x u< y`. Thanks. > >> This is both complicated and inefficient, I would suggest building the >> intrinsic in the IR graph so that the compiler can simplify >> `Integer.compareUnsigned(x, y) < 0` into `x u< y`. Thanks. > > Thank you for the suggestion! This intrinsic uses 1 cmp instruction instead > of two and shows ~10% improvement due to better branch prediction. Even > without the intrinsic, the compiler is currently able to reduce it to x u< y > but is still generating two cmp (unsigned) instructions as > Integer.compareUnsigned(x, y) is implemented as x u< y? -1 : (x ==y ? 0 : 1). @vamsi-parasa But normally the result of the `compare` methods is not used as a raw integer (it is useless to do so since the methods do not have any promise regarding the value of the result, only its sign). The idiom is to compare the result with 0, such as `Integer.compare(x, y) > 0`, the compiler can reduce this to `x > y` (last time I checked it does not do so but in principle this is possible). Your intrinsic prevents the compiler to do such transformations, hurting the performance of real programs. > Even without the intrinsic, the compiler is currently able to reduce it to x > u< y It is because the compiler can recognise the pattern `x + MIN_VALUE < y + MIN_VALUE` and transforms it into `x u< y`. This transformation is fragile however if one of the arguments is in the form `x + con`, in such cases constant propagation may lead to slight deviations from recognised patterns, defeat the transformations. As a result, it may be justifiable to have a dedicated intrinsic for that since unsigned comparisons are pretty basic operations that are needed for optimal range check performance. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7975
Re: RFR: 8283726: x86 intrinsics for compare method in Integer and Long
On Tue, 29 Mar 2022 02:24:21 GMT, Jatin Bhateja 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
Will update x86_32.ad as well.
> 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.
Sure, will refactor it into a macro-assembly
> 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.
Sure, will do that and update it soon.
> 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.
Sure, will do that and update it soon.
> 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.
Yes, that is the intention.
> 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;
> }
The suggested refactoring is definitely elegant but one rare possibility is
overflow due to the addition/subtraction. The swap logic doesn't have that
problem.
> 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.
Sure, will do the unification.
-
PR: https://git.openjdk.java.net/jdk/pull/7975
Re: RFR: 8283726: x86 intrinsics for compare method in Integer and Long
On Sun, 27 Mar 2022 06:57:58 GMT, Quan Anh Mai wrote: > This is both complicated and inefficient, I would suggest building the > intrinsic in the IR graph so that the compiler can simplify > `Integer.compareUnsigned(x, y) < 0` into `x u< y`. Thanks. Thank you for the suggestion! This intrinsic uses 1 cmp instruction instead of two and shows ~10% improvement due to better branch prediction. Even without the intrinsic, the compiler is currently able to reduce it to x u< y but is still generating two cmp (unsigned) instructions as Integer.compareUnsigned(x, y) is implemented as x u< y? -1 : (x ==y ? 0 : 1). - PR: https://git.openjdk.java.net/jdk/pull/7975
Re: RFR: 8253569: javax.xml.catalog.Catalog.matchURI() implementation should reset state variables
On Tue, 29 Mar 2022 16:39:50 GMT, Joe Wang wrote: > Resets state of a Catalog instance on each matching call so that it can be > reused. Adjusts CatalogResolver's resolve routine so that it continues to > observes the PREFER feature in a resolution process. Without the adjustment, > PreferFeatureTest would fail. > > Mach5 XML tests passed: > https://mach5.us.oracle.com/mdash/jobs/huizwang-open-20220329-0148-30563542 src/java.xml/share/classes/javax/xml/catalog/GroupEntry.java line 44: > 42: > 43: //Indicates whether this is the Catalog instance (vs a Catalog entry) > 44: boolean isCatalogInstance = false; Can we eliminate this instance variable, by checking whether the type is `Catalog` or not in `resetOnStart()`? - PR: https://git.openjdk.java.net/jdk/pull/8018
Re: RFR: 8282819: Deprecate Locale class constructors [v7]
On Tue, 29 Mar 2022 21:28:44 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: > > Changed from var-args to conventional args. Reflected suggestions. Per a comment in the CSR discussion (https://bugs.openjdk.java.net/browse/JDK-8283478?focusedCommentId=14485037=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14485037), I changed the arguments to var-args to conventional ones. - PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8253569: javax.xml.catalog.Catalog.matchURI() implementation should reset state variables
On Tue, 29 Mar 2022 16:39:50 GMT, Joe Wang wrote: > Resets state of a Catalog instance on each matching call so that it can be > reused. Adjusts CatalogResolver's resolve routine so that it continues to > observes the PREFER feature in a resolution process. Without the adjustment, > PreferFeatureTest would fail. > > Mach5 XML tests passed: > https://mach5.us.oracle.com/mdash/jobs/huizwang-open-20220329-0148-30563542 looks fine Joe - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8018
Re: RFR: 8282819: Deprecate Locale class constructors [v7]
> 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: Changed from var-args to conventional args. Reflected suggestions. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7947/files - new: https://git.openjdk.java.net/jdk/pull/7947/files/b1d36b52..8ac9d11d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7947=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7947=05-06 Stats: 132 lines in 4 files changed: 76 ins; 3 del; 53 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: 8283889: Fix Typo in open/src/java.sql/share/classes/java/sql/package-info.java
On Tue, 29 Mar 2022 18:23:31 GMT, Lance Andersen wrote: > Hi all, > > Please review this trivial fix which addresses a typo in > src/java.sql/share/classes/java/sql/package-info.java > > make docs is clean and I am also running mach5 tier1 as an extra sanity check > so there are no surprises when the patch is pushed > > Best > Lance This pull request has now been integrated. Changeset: 272d6531 Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/272d6531ef94534d2044377f126744b5139f7ae9 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8283889: Fix Typo in open/src/java.sql/share/classes/java/sql/package-info.java Reviewed-by: joehw, iris - PR: https://git.openjdk.java.net/jdk/pull/8020
RFR: 8283060: RawNativeLibraries should allow multiple clients to load/unload the same library
A small improvement to `RawNativeLibraries`. `RawNativeLibraries::load` returns the same `NativeLibrary` instance of a given path if it's called multiple times. This means that multiple clients have to coordinate that the last one using the loaded library is the one to close the library by calling `RawNativeLibraries::unload`; otherwise, an exception may be thrown. This patch changes `RawNativeLibraries::load` to delegate to the underlying platform-specific library loading mechanism e.g. dlopen/dlclose that keeps the reference count. So each call to `RawNativeLibraries::load` and `unload` is simply a call to dlopen and dlclose respectively. Each client of calling `RawNativeLibraries::load` is responsible for calling `RawNativeLibraries::unload`. This will be consistent with the current behavior when you call `load` and `unload` with a different `RawNativeLibraries` instance. - Commit messages: - 8283060: RawNativeLibraries should allow multiple clients to load/unload the same library Changes: https://git.openjdk.java.net/jdk/pull/8022/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8022=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283060 Stats: 66 lines in 3 files changed: 43 ins; 10 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/8022.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8022/head:pull/8022 PR: https://git.openjdk.java.net/jdk/pull/8022
Re: RFR: 8283889: Fix typo in package-info.java
On Tue, 29 Mar 2022 18:23:31 GMT, Lance Andersen wrote: > Hi all, > > Please review this trivial fix which addresses a typo in > src/java.sql/share/classes/java/sql/package-info.java > > make docs is clean and I am also running mach5 tier1 as an extra sanity check > so there are no surprises when the patch is pushed > > Best > Lance Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8020
Integrated: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver
On Wed, 9 Mar 2022 22:52:41 GMT, Mandy Chung wrote: > A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. This pull request has now been integrated. Changeset: 489b27d2 Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/489b27d2c0284f9248bfb0448950698a3f9dee36 Stats: 16 lines in 1 file changed: 10 ins; 0 del; 6 mod 8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v5]
> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8282776 - per feedback - Add exception message - Move the null check after isInstance check - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8282776 - JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver - Changes: - all: https://git.openjdk.java.net/jdk/pull/7766/files - new: https://git.openjdk.java.net/jdk/pull/7766/files/55c38cf8..5c1d777b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7766=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7766=03-04 Stats: 124526 lines in 1656 files changed: 92323 ins; 27724 del; 4479 mod Patch: https://git.openjdk.java.net/jdk/pull/7766.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7766/head:pull/7766 PR: https://git.openjdk.java.net/jdk/pull/7766
Integrated: 8283782: Redundant verification of year in LocalDate::ofEpochDay
On Tue, 29 Mar 2022 10:37:20 GMT, Claes Redestad wrote: > In `LocalDate::ofEpochDays` we validate the epoch day input, then we also > validate the year derived from that value. This second validation is > redundant since the minimum and maximum valid epoch day line up with the > first and last day of the minimum and maximum valid year, respectively. This > patch replace this redundant runtime validation with a test. > > This reduces code complexity (increasing chance for inlining to happen) and > removes a couple of branches from generated code. This pull request has now been integrated. Changeset: 072f2c46 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/072f2c461e8b0f10bdecadda31b3facfbe6da681 Stats: 39 lines in 2 files changed: 34 ins; 2 del; 3 mod 8283782: Redundant verification of year in LocalDate::ofEpochDay Reviewed-by: rriggs, naoto - PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay [v3]
On Tue, 29 Mar 2022 15:27:28 GMT, Claes Redestad wrote: >> In `LocalDate::ofEpochDays` we validate the epoch day input, then we also >> validate the year derived from that value. This second validation is >> redundant since the minimum and maximum valid epoch day line up with the >> first and last day of the minimum and maximum valid year, respectively. This >> patch replace this redundant runtime validation with a test. >> >> This reduces code complexity (increasing chance for inlining to happen) and >> removes a couple of branches from generated code. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > typo: to -> of Thanks for reviews! - PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v17]
On Tue, 29 Mar 2022 15:56:37 GMT, Jim Laskey wrote: >> doesn't the compiler (either javac or hotspot) automatically do that? > > Yes, the compilers handle this. I did however switch to > > > primitiveCount != 0 ? new long[(primitiveCount + 1) / > LONG_SLOTS] : null; > doesn't the compiler (either javac or hotspot) automatically do that? Only for multiplication, because `-1 >> 1 == -1`, `-1 >>> 1 == Integer.MAX_VALUE`, and `-1 / 2 == 0`. - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v18]
> 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: Use LONG_SLOTS - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/23122263..f9c8730a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=16-17 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: 8283889: Fix typo in package-info.java
On Tue, 29 Mar 2022 18:23:31 GMT, Lance Andersen wrote: > Hi all, > > Please review this trivial fix which addresses a typo in > src/java.sql/share/classes/java/sql/package-info.java > > make docs is clean and I am also running mach5 tier1 as an extra sanity check > so there are no surprises when the patch is pushed > > Best > Lance Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8020
RFR: 8283889: Fix typo in package-info.java
Hi all, Please review this trivial fix which addresses a typo in src/java.sql/share/classes/java/sql/package-info.java make docs is clean and I am also running mach5 tier1 as an extra sanity check so there are no surprises when the patch is pushed Best Lance - Commit messages: - Fix typo in package-info.java Changes: https://git.openjdk.java.net/jdk/pull/8020/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8020=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283889 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8020.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8020/head:pull/8020 PR: https://git.openjdk.java.net/jdk/pull/8020
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v13]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Switch to daemon threads for async upcalls - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/55aee872..43dc6be3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=11-12 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v12]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 27 additional commits since the last revision: - Use thread local storage to optimize attach of async threads - Drop support for Constable from MemoryLayout/FunctionDescriptor - Merge branch 'master' into foreign-preview - Revert changes to RunTests.gmk - Add --enable-preview to micro benchmark java options - Address more review comments - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Address review comments - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - ... and 17 more: https://git.openjdk.java.net/jdk/compare/02333d66...55aee872 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/504b564a..55aee872 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=10-11 Stats: 99257 lines in 1550 files changed: 79659 ins; 15544 del; 4054 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 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 Java changes are good. - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v4]
On Wed, 23 Mar 2022 23:22:31 GMT, Mandy Chung wrote: >> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null >> receiver check rather than NPE thrown by `Object::getClass`. The message of >> NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be >> helpful but not in this case. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > per feedback Moving the check under the `!isInstance(recv)` check is better. - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: 8283470: Update java.lang.invoke.VarHandle to use sealed classes
On Wed, 23 Mar 2022 16:27:29 GMT, Mandy Chung wrote: > This patch changes VarHandle and its implementation hierarchy to use sealed > classes. All VarHandle permitted classes are package-private and either > final or sealed abstract classes. > > Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8283540 Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7926
Re: RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay [v3]
On Tue, 29 Mar 2022 15:27:28 GMT, Claes Redestad wrote: >> In `LocalDate::ofEpochDays` we validate the epoch day input, then we also >> validate the year derived from that value. This second validation is >> redundant since the minimum and maximum valid epoch day line up with the >> first and last day of the minimum and maximum valid year, respectively. This >> patch replace this redundant runtime validation with a test. >> >> This reduces code complexity (increasing chance for inlining to happen) and >> removes a couple of branches from generated code. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > typo: to -> of Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Looks good. Thanks for improving the error reporting in CheckCSMs test. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Nice update to the test! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283846: Remove unused jdk.internal.reflect.SignatureIterator
On Tue, 29 Mar 2022 09:15:01 GMT, Andrey Turbanov wrote: > It was no longer used due to JDK-4479184 long ago. Nice clean-up. - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8013
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283846: Remove unused jdk.internal.reflect.SignatureIterator
On Tue, 29 Mar 2022 09:15:01 GMT, Andrey Turbanov wrote: > It was no longer used due to JDK-4479184 long ago. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8013
RFR: 8253569: javax.xml.catalog.Catalog.matchURI() implementation should reset state variables
Resets state of a Catalog instance on each matching call so that it can be reused. Adjusts CatalogResolver's resolve routine so that it continues to observes the PREFER feature in a resolution process. Without the adjustment, PreferFeatureTest would fail. Mach5 XML tests passed: https://mach5.us.oracle.com/mdash/jobs/huizwang-open-20220329-0148-30563542 - Commit messages: - 8253569: javax.xml.catalog.Catalog.matchURI() implementation should reset state variables Changes: https://git.openjdk.java.net/jdk/pull/8018/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8018=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253569 Stats: 220 lines in 5 files changed: 198 ins; 18 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8018.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8018/head:pull/8018 PR: https://git.openjdk.java.net/jdk/pull/8018
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Yes, the CheckCSMs test needed to be updated to remove ObjectStreamClass#forClass from the list of non-final CSMs, as that method is now part of a final class. The error message about ObjectStreamField#getType is misleading; I've fixed up the error reporting for this case. It would indeed be interesting to make adjustments to ObjectStreamField. However, it's publicly subclassable, and it's not serializable, so a different set of dynamics apply. I'd like to consider that effort separately from this PR. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
> Pretty much just what it says. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test - Changes: - all: https://git.openjdk.java.net/jdk/pull/8009/files - new: https://git.openjdk.java.net/jdk/pull/8009/files/4b9bf990..77b6d79f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8009=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8009=00-01 Stats: 8 lines in 1 file changed: 2 ins; 1 del; 5 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
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v17]
On Tue, 29 Mar 2022 15:49:51 GMT, liach wrote:
>> src/java.base/share/classes/java/lang/runtime/Carrier.java line 335:
>>
>>> 333: CarrierArray(int primitiveCount, int objectCount) {
>>> 334: this.primitives =
>>> 335: primitiveCount != 0 ? new long[(primitiveCount +
>>> 1) / 2] : null;
>>
>> It might be better to use an unsigned shift here:
>> Suggestion:
>>
>> primitiveCount != 0 ? new long[(primitiveCount + 1) >>>
>> 1] : null;
>
> doesn't the compiler (either javac or hotspot) automatically do that?
Yes, the compilers handle this. I did however switch to
primitiveCount != 0 ? new long[(primitiveCount + 1) /
LONG_SLOTS] : null;
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8283846: Remove unused jdk.internal.reflect.SignatureIterator
On Tue, 29 Mar 2022 09:15:01 GMT, Andrey Turbanov wrote: > It was no longer used due to JDK-4479184 long ago. It builds so looks fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8013
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v17]
On Tue, 29 Mar 2022 13:58:56 GMT, ExE Boss wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add condy static methods
>
> src/java.base/share/classes/java/lang/runtime/Carrier.java line 335:
>
>> 333: CarrierArray(int primitiveCount, int objectCount) {
>> 334: this.primitives =
>> 335: primitiveCount != 0 ? new long[(primitiveCount + 1)
>> / 2] : null;
>
> It might be better to use an unsigned shift here:
> Suggestion:
>
> primitiveCount != 0 ? new long[(primitiveCount + 1) >>>
> 1] : null;
doesn't the compiler (either javac or hotspot) automatically do that?
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay [v3]
> In `LocalDate::ofEpochDays` we validate the epoch day input, then we also > validate the year derived from that value. This second validation is > redundant since the minimum and maximum valid epoch day line up with the > first and last day of the minimum and maximum valid year, respectively. This > patch replace this redundant runtime validation with a test. > > This reduces code complexity (increasing chance for inlining to happen) and > removes a couple of branches from generated code. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: typo: to -> of - Changes: - all: https://git.openjdk.java.net/jdk/pull/8014/files - new: https://git.openjdk.java.net/jdk/pull/8014/files/44a11aa7..ef0ca1b3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8014=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8014=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8014.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8014/head:pull/8014 PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: 8282819: Deprecate Locale class constructors [v6]
On Mon, 28 Mar 2022 22:16:43 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:
>
> Added an @apiNote describing the single array argument
src/java.base/share/classes/sun/util/locale/provider/JRELocaleConstants.java
line 38:
> 36: */
> 37: public class JRELocaleConstants {
> 38: public static final Locale JA_JP_JP =
> Locale.forLanguageTag("ja-JP-x-lvariant-JP");
You might use the new factory here.
src/java.base/share/classes/sun/util/resources/LocaleData.java line 249:
> 247: // TODO: avoid hard-coded Locales
> 248: private static final Set JAVA_BASE_LOCALES
> 249: = Set.of(Locale.ROOT, Locale.ENGLISH, Locale.US,
> Locale.forLanguageTag("en-US-POSIX"));
Use new factory?
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay [v2]
On Tue, 29 Mar 2022 14:07:41 GMT, Claes Redestad wrote: >> In `LocalDate::ofEpochDays` we validate the epoch day input, then we also >> validate the year derived from that value. This second validation is >> redundant since the minimum and maximum valid epoch day line up with the >> first and last day of the minimum and maximum valid year, respectively. This >> patch replace this redundant runtime validation with a test. >> >> This reduces code complexity (increasing chance for inlining to happen) and >> removes a couple of branches from generated code. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Sparse out the range of offsets tested Thanks - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8014
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. Close by is `java.io.ObjectStreamField` which seems also useful to make final. Though it does have a public constructor so is more likely to raise a compatibility concern. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay [v2]
On Tue, 29 Mar 2022 13:56:09 GMT, Roger Riggs wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Sparse out the range of offsets tested
>
> test/jdk/java/time/test/java/time/TestLocalDate.java line 432:
>
>> 430: long minYear = ChronoField.YEAR.range().getMinimum();
>> 431: long maxYear = ChronoField.YEAR.range().getMinimum();
>> 432: for (int i = 0; i < 500; i++) {
>
> The 500 seems like it would burn a lot of cycles mostly redundant.
> How about { 1, 2, 3, 28, 29, 30, 31, 32, 363, 364, 365, 366, 367}.
Yes, makes sense to avoid useless work. Added 0 to the list.
-
PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay [v2]
> In `LocalDate::ofEpochDays` we validate the epoch day input, then we also > validate the year derived from that value. This second validation is > redundant since the minimum and maximum valid epoch day line up with the > first and last day of the minimum and maximum valid year, respectively. This > patch replace this redundant runtime validation with a test. > > This reduces code complexity (increasing chance for inlining to happen) and > removes a couple of branches from generated code. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Sparse out the range of offsets tested - Changes: - all: https://git.openjdk.java.net/jdk/pull/8014/files - new: https://git.openjdk.java.net/jdk/pull/8014/files/6f82eb7d..44a11aa7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8014=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8014=00-01 Stats: 8 lines in 1 file changed: 1 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/8014.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8014/head:pull/8014 PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v17]
On Tue, 29 Mar 2022 13:49: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:
>
> Add condy static methods
src/java.base/share/classes/java/lang/runtime/Carrier.java line 335:
> 333: CarrierArray(int primitiveCount, int objectCount) {
> 334: this.primitives =
> 335: primitiveCount != 0 ? new long[(primitiveCount + 1)
> / 2] : null;
It might be better to use an unsigned shift here:
Suggestion:
primitiveCount != 0 ? new long[(primitiveCount + 1) >>> 1]
: null;
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay
On Tue, 29 Mar 2022 10:37:20 GMT, Claes Redestad wrote:
> In `LocalDate::ofEpochDays` we validate the epoch day input, then we also
> validate the year derived from that value. This second validation is
> redundant since the minimum and maximum valid epoch day line up with the
> first and last day of the minimum and maximum valid year, respectively. This
> patch replace this redundant runtime validation with a test.
>
> This reduces code complexity (increasing chance for inlining to happen) and
> removes a couple of branches from generated code.
test/jdk/java/time/test/java/time/TestLocalDate.java line 432:
> 430: long minYear = ChronoField.YEAR.range().getMinimum();
> 431: long maxYear = ChronoField.YEAR.range().getMinimum();
> 432: for (int i = 0; i < 500; i++) {
The 500 seems like it would burn a lot of cycles mostly redundant.
How about { 1, 2, 3, 28, 29, 30, 31, 32, 363, 364, 365, 366, 367}.
-
PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v3]
On Tue, 29 Mar 2022 12:43:25 GMT, Volker Simonis wrote: >> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` >> to highlight that it might write more bytes than the returned number of >> inflated bytes into the buffer `b`. >> >> The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, >> int len)` will leave the content beyond the last read byte in the read >> buffer `b` unaffected. However, the overridden `read` method in >> `InflaterInputStream` passes the read buffer `b` to >> `Inflater::inflate(byte[] b, int off, int len)` which doesn't provide this >> guarantee. Depending on implementation details, `Inflater::inflate` might >> write more than the returned number of inflated bytes into the buffer `b`. >> >> ### TL;DR >> >> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater >> functionality. `Inflater::inflate(byte[] output, int off, int len)` >> currently calls zlib's native `inflate(..)` function and passes the address >> of `output[off]` and `len` to it via JNI. >> >> The specification of zlib's `inflate(..)` function (i.e. the [API >> documentation in the original zlib >> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) >> doesn't give any guarantees with regard to usage of the output buffer. It >> only states that upon completion the function will return the number of >> bytes that have been written (i.e. "inflated") into the output buffer. >> >> The original zlib implementation only wrote as many bytes into the output >> buffer as it inflated. However, this is not a hard requirement and newer, >> more performant implementations of the zlib library like >> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) >> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes >> of the output buffer than they actually inflate as a scratch buffer. See >> https://github.com/simonis/zlib-chromium for a more detailed description of >> their approach and its performance benefit. >> >> These new zlib versions can still be used transparently from Java (e.g. by >> putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because >> they still fully comply to specification of `Inflater::inflate(..)`. >> However, we might run into problems when using the `Inflater` functionality >> from the `InflaterInputStream` class. `InflaterInputStream` is derived from >> from `InputStream` and as such, its `read(byte[] b, int off, int len)` >> method is quite constrained. It specifically specifies that if *k* bytes >> have been read, then "these bytes will be stored in elements `b[off]` >> through `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through >> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[] b, int >> off, int len)` (which is constrained by `InputStream::read(..)`'s >> specification) calls `Inflater::inflate(byte[] b, int off, int len)` and >> directly passes its output buffer down to the native zlib `inflate(..)` >> method which is free to change the bytes beyond `b[off+` *k*`]` (where *k* is the number of inflated bytes). >> >> From a practical point of view, I don't see this as a big problem, because >> callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never >> know how many bytes will be written into the output buffer `b` (and in fact >> its content can always be completely overwritten). It therefore makes no >> sense to depend on any data there being untouched after the call. Also, >> having used zlib-cloudflare productively for about two years, we haven't >> seen real-world issues because of this behavior yet. However, from a >> specification point of view it is easy to artificially construct a program >> which violates `InflaterInputStream::read(..)`'s postcondition if using one >> of the alterantive zlib implementations. A recently integrated JTreg test >> (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails >> with zlib-chromium but can fixed easily to run with alternative >> implementations as well (see >> [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Extended API-doc based on reviewer feedback One other way to communicate changes is in the release-note. I added release-note=yes to the bug. - PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v17]
> 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 condy static methods - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/e577e16a..23122263 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=15-16 Stats: 99 lines in 2 files changed: 75 ins; 9 del; 15 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: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Tue, 29 Mar 2022 01:58:05 GMT, Jaikiran Pai wrote:
> 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:
>
> ```
> ZipFile zf = ...
> ZipEntry ze = zf.getEntry("some-file");
> InputStream is = zf.getInputStream(ze);
> ```
>
> As we see above, none of these APIs talk about `InflaterInputStream` (the
> return type of `ZipFile.getInpustream(...)` is an `InputStream`). So end
> users won't be able to view this spec change. Perhaps we should also add some
> note in the `ZipFile.getInpustream()` API to make a mention of this
> potential difference in behaviour of the returned stream?
You are right with your observation and I'll be happy to add a corresponding
comment if @LanceAndersen and @AlanBateman agree. Please let me know what you
think?
-
PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Mon, 28 Mar 2022 18:23:46 GMT, Lance Andersen wrote: >> 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 Thanks for your comments. I've updated the API-doc according to your recommendations. Please feel free to propose new/better wording :) I'll wait with updating the CSR until we reach final agreement in this PR. - PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v3]
> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` to > highlight that it might write more bytes than the returned number of > inflated bytes into the buffer `b`. > > The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, > int len)` will leave the content beyond the last read byte in the read buffer > `b` unaffected. However, the overridden `read` method in > `InflaterInputStream` passes the read buffer `b` to `Inflater::inflate(byte[] > b, int off, int len)` which doesn't provide this guarantee. Depending on > implementation details, `Inflater::inflate` might write more than the > returned number of inflated bytes into the buffer `b`. > > ### TL;DR > > `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater > functionality. `Inflater::inflate(byte[] output, int off, int len)` currently > calls zlib's native `inflate(..)` function and passes the address of > `output[off]` and `len` to it via JNI. > > The specification of zlib's `inflate(..)` function (i.e. the [API > documentation in the original zlib > implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) > doesn't give any guarantees with regard to usage of the output buffer. It > only states that upon completion the function will return the number of bytes > that have been written (i.e. "inflated") into the output buffer. > > The original zlib implementation only wrote as many bytes into the output > buffer as it inflated. However, this is not a hard requirement and newer, > more performant implementations of the zlib library like > [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) > or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes > of the output buffer than they actually inflate as a scratch buffer. See > https://github.com/simonis/zlib-chromium for a more detailed description of > their approach and its performance benefit. > > These new zlib versions can still be used transparently from Java (e.g. by > putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because > they still fully comply to specification of `Inflater::inflate(..)`. However, > we might run into problems when using the `Inflater` functionality from the > `InflaterInputStream` class. `InflaterInputStream` is derived from from > `InputStream` and as such, its `read(byte[] b, int off, int len)` method is > quite constrained. It specifically specifies that if *k* bytes have been > read, then "these bytes will be stored in elements `b[off]` through > `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through `b[off+len-1]` > **unaffected**". But `InflaterInputStream::read(byte[] b, int off, int len)` > (which is constrained by `InputStream::read(..)`'s specification) calls > `Inflater::inflate(byte[] b, int off, int len)` and directly passes its > output buffer down to the native zlib `inflate(..)` method which is free to > change the bytes beyond `b[off+`* k*`]` (where *k* is the number of inflated bytes). > > From a practical point of view, I don't see this as a big problem, because > callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never > know how many bytes will be written into the output buffer `b` (and in fact > its content can always be completely overwritten). It therefore makes no > sense to depend on any data there being untouched after the call. Also, > having used zlib-cloudflare productively for about two years, we haven't seen > real-world issues because of this behavior yet. However, from a specification > point of view it is easy to artificially construct a program which violates > `InflaterInputStream::read(..)`'s postcondition if using one of the > alterantive zlib implementations. A recently integrated JTreg test > (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails > with zlib-chromium but can fixed easily to run with alternative > implementations as well (see > [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). Volker Simonis has updated the pull request incrementally with one additional commit since the last revision: Extended API-doc based on reviewer feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/7986/files - new: https://git.openjdk.java.net/jdk/pull/7986/files/b55fc332..62a46591 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7986=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7986=01-02 Stats: 9 lines in 1 file changed: 5 ins; 0 del; 4 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: JDK-8282798 java.lang.runtime.Carrier [v16]
On Mon, 28 Mar 2022 20:32:02 GMT, ExE Boss wrote:
>> 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.
Thanks for pointing this out. It is the slot count, but I was over-allocating
the long array.
diff --git a/src/java.base/share/classes/java/lang/runtime/Carrier.java
b/src/java.base/share/classes/java/lang/runtime/Carrier.java
index 50a32346417..775df6cb4c3 100644
--- a/src/java.base/share/classes/java/lang/runtime/Carrier.java
+++ b/src/java.base/share/classes/java/lang/runtime/Carrier.java
@@ -331,7 +331,7 @@ public final class Carrier {
* @param objectCount slot count required for objects
*/
CarrierArray(int primitiveCount, int objectCount) {
-this.primitives = new long[primitiveCount];
+this.primitives = new long[(primitiveCount + 1) / 2];
this.objects = new Object[objectCount];
}
@@ -432,10 +432,10 @@ public final class Carrier {
int longCount = carrierShape.longCount();
int intCount = carrierShape.intCount();
int objectCount = carrierShape.objectCount();
-int primitiveSlots = longCount * LONG_SLOTS + intCount;
+int primitiveCount = longCount * LONG_SLOTS + intCount;
MethodHandle constructor =
MethodHandles.insertArguments(CONSTRUCTOR,
-0, primitiveSlots, objectCount);
+0, primitiveCount, objectCount);
// long array index
int index = 0;
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8003417: WeakHashMap$HashIterator removes wrong entry
On Sat, 20 Nov 2021 10:08:41 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > reported in https://bugs.openjdk.java.net/browse/JDK-8003417? > > The issue notes that this is applicable for `WeakHashMap` which have `null` > keys. However, the issue is even applicable for `WeakHashMap` instances which > don't have `null` keys, as reproduced and shown by the newly added jtreg test > case in this PR. > > The root cause of the issue is that once the iterator is used to iterate till > the end and the `remove()` is called, then the > `WeakHashMap$HashIterator#remove()` implementation used to pass `null` as the > key to remove from the map, instead of the key of the last returned entry. > The commit in this PR fixes that part. > > A new jtreg test has been added which reproduces the issue as well as > verifies the fix. > `tier1` testing and this new test have passed after this change. However, I > guess this will require a JCK run to be run too, perhaps? If so, I will need > help from someone who has access to them to have this run against those > please. (work is currently in progress on this one) - PR: https://git.openjdk.java.net/jdk/pull/6488
RFR: 8283782: Redundant verification of year in LocalDate::ofEpochDay
In `LocalDate::ofEpochDays` we validate the epoch day input, then we also validate the year derived from that value. This second validation is redundant since the minimum and maximum valid epoch day line up with the first and last day of the minimum and maximum valid year, respectively. This patch replace this redundant runtime validation with a test. This reduces code complexity (increasing chance for inlining to happen) and removes a couple of branches from generated code. - Commit messages: - Explicitly check that year in successfully created LocalDates is valid - Copyright - Redundant verification of year in LocalDate::ofEpochDay Changes: https://git.openjdk.java.net/jdk/pull/8014/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8014=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283782 Stats: 38 lines in 2 files changed: 33 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8014.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8014/head:pull/8014 PR: https://git.openjdk.java.net/jdk/pull/8014
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Sat, 26 Mar 2022 16:35:14 GMT, liach wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8282662: Revert dubious changes in MethodType > > Just curious, this issue asks to replace set constructions with `Set.of`, but > there is no such code changes in this pull request. Is there any set creation > patterns that aren't detected by your ide cleanups, such as > `Collections.addAll(set, elements)` for creating hash set contents from array? @liach I've found a couple of places: - `AsynchronousFileChannel.open()` - `FileChannel.open()` - `DecimalStyle.getAvailableLocales()` They all either rely on user input, i.e. might throw NPE or IAE for nulls or duplicates respectively, or return the set, which is currently modifiable `HashSet`. I think the possible win isn't worth the effort. - PR: https://git.openjdk.java.net/jdk/pull/7729
Integrated: 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. This pull request has now been integrated. Changeset: cc598e03 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/cc598e03de39dd6e8d7e208a69d85b6a9cd0062f Stats: 14 lines in 3 files changed: 6 ins; 3 del; 5 mod 8283774: TestZoneOffset::test_immutable should ignore ZoneOffset::rules Reviewed-by: rriggs, naoto - PR: https://git.openjdk.java.net/jdk/pull/7989
Re: RFR: 8283774: TestZoneOffset::test_immutable should ignore ZoneOffset::rules [v2]
> - 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. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Copyrights - Changes: - all: https://git.openjdk.java.net/jdk/pull/7989/files - new: https://git.openjdk.java.net/jdk/pull/7989/files/44a8cc6b..4da4429b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7989=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7989=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 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: 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. Thanks for reviews! - PR: https://git.openjdk.java.net/jdk/pull/7989
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. Yes - it looks like jdk/internal/reflect/CallerSensitive/CheckCSMs.java needs to be updated. - PR: https://git.openjdk.java.net/jdk/pull/8009
Integrated: 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. This pull request has now been integrated. Changeset: 0e788e0e Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/0e788e0ecbf44f1feee817fe22123a8523da5ee3 Stats: 16 lines in 1 file changed: 9 ins; 2 del; 5 mod 8283781: Avoid allocating unused lastRulesCaches Reviewed-by: rriggs, naoto - PR: https://git.openjdk.java.net/jdk/pull/7990
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. Thanks for reviews! - PR: https://git.openjdk.java.net/jdk/pull/7990
RFR: 8283846: Remove unused jdk.internal.reflect.SignatureIterator
It was no longer used due to JDK-4479184 long ago. - Commit messages: - [PATCH] Remove unused jdk.internal.reflect.SignatureIterator Changes: https://git.openjdk.java.net/jdk/pull/8013/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8013=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283846 Stats: 81 lines in 1 file changed: 0 ins; 81 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8013.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8013/head:pull/8013 PR: https://git.openjdk.java.net/jdk/pull/8013
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. The test failures caught by GitHub Actions jobs look related to the area of this change: 2022-03-29T02:21:21.2187570Zat CheckCSMs.main(CheckCSMs.java:98) 2022-03-29T02:21:21.2188140Zat java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) 2022-03-29T02:21:21.2188650Zat java.base/java.lang.reflect.Method.invoke(Method.java:577) 2022-03-29T02:21:21.2189060Zat com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) 2022-03-29T02:21:21.2189420Zat java.base/java.lang.Thread.run(Thread.java:828) 2022-03-29T02:21:21.2189590Z 2022-03-29T02:21:21.2190030Z JavaTest Message: Test threw exception: java.lang.RuntimeException: Unexpected non-final instance method: 2022-03-29T02:21:21.2190400Z java/io/ObjectStreamField#getType ()Ljava/lang/Class; - 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. Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8282162: [vector] Optimize integral vector negation API [v3]
On Tue, 29 Mar 2022 05:05:43 GMT, Jie Fu wrote: >> 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 > > Note: I didn't check the aarch64 code change. Thanks for the review @DamonFool ! - PR: https://git.openjdk.java.net/jdk/pull/7782
