Re: RFR: JDK-8301833: Add manual tests for FDLIBM porting
On Mon, 6 Feb 2023 17:53:24 GMT, Joe Darcy wrote: > Therefore, I think these tests should be included in the repository, but not > run all the time, which led me to declare them as a manual jtreg test. Manual tests are run at each release. There are a couple of examples in the repo with tests that don't have @test tags. There are easily forgotten but they have been useful in a few areas for cases like this where the maintainer wants something in the repo doesn't doesn't want it run by regular testing. If you do decide to make this a manual test then I think it will need instructions so that someone knows what to do. - PR: https://git.openjdk.org/jdk/pull/12430
Re: RFR: JDK-8301833: Add manual tests for FDLIBM porting
On Mon, 6 Feb 2023 17:53:24 GMT, Joe Darcy wrote: > Therefore, I think these tests should be included in the repository, but not > run all the time, which led me to declare them as a manual jtreg test. - PR: https://git.openjdk.org/jdk/pull/12430
RFR: 8301767: Convert virtual thread tests to JUnit
The non-hotspot tests integrated with JEP 425/428 were mostly TestNG tests. We'd like to convert these JUnit in the main line in advance of other updates to these tests in 21. The changes are mostly mechanical and trivial: - BeforeClass/AfterClass changed to static BeforeAll/AfterAll methods - Tests using data providers are changed to parameterized tests - The order of the parameters to assertEquals are swapped so that the expected result is the first parameter - Usages of expectThrows are changed to assertThrows - Tests that threw SkipException are changed to the Assumptions API There are a small number of drive-by changes to the tests, nothing significant, e.g. - GetStackTrace and ParkWithFixedThreadPool changed from "@run testng" to "@run main" as they aren't TestNG tests. - A few of the tests in StructuredTaskScopeTest for joinXXX are changed to use a CountDownLatch rather than sleeping, as the original tests weren't very robust. - Commit messages: - Fix typos in comments - GetStackTrace.java test missing @requires vm.continuations - Initial commit Changes: https://git.openjdk.org/jdk/pull/12426/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12426&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8301767 Stats: 1416 lines in 34 files changed: 205 ins; 79 del; 1132 mod Patch: https://git.openjdk.org/jdk/pull/12426.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12426/head:pull/12426 PR: https://git.openjdk.org/jdk/pull/12426
Re: RFR: 8299748: java/util/zip/Deinflate.java failing on s390x
On Thu, 2 Feb 2023 10:06:23 GMT, Alan Bateman wrote:
>>> level:1, strategy: 0, dowrap: false
>>> is finished: false
>>
>> Thanks for checking that. So "is finished: false" is telling us that not all
>> of the input was compressed. So I think the right thing is to do the deflate
>> in a loop until there is more input to compress, the inflate probably needs
>> the same. Your original proposal was to make the output buffer larger and I
>> suspect that is just working around a threshold or block size used by the
>> accelerator.
>
>> Hi @AlanBateman ,
>> with latest changes (doing inflate/deinflate) test passes over s390 and x86
>> as well. Please take a look now.
>
> Good. One thing to try is to just deflate/inflate into out1/out2, no need for
> the intermediate BAOS, try this:
>
>
> --- a/test/jdk/java/util/zip/DeInflate.java
> +++ b/test/jdk/java/util/zip/DeInflate.java
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -127,11 +127,25 @@ public class DeInflate {
>
> def.setInput(in, 0, len);
> def.finish();
> -int m = def.deflate(out1);
> +int m = 0;
> +while (!def.finished()) {
> +int remaining = out1.length - m;
> +if (remaining == 0) {
> +throw new RuntimeException("out1 is small");
> +}
> +m += def.deflate(out1, m, remaining);
> +}
>
> Inflater inf = new Inflater(nowrap);
> inf.setInput(out1, 0, m);
> -int n = inf.inflate(out2);
> +int n = 0;
> +while (!inf.finished()) {
> +int remaining = out2.length - n;
> +if (remaining == 0) {
> +throw new RuntimeException("out2 is small");
> +}
> +n += inf.inflate(out2, n, remaining);
> +}
>
> if (n != len ||
> !Arrays.equals(Arrays.copyOf(in, len), Arrays.copyOf(out2, len))
> ||
> @AlanBateman should we proceed with the current changes then ?
The BAOS in your current proposal shouldn't be needed so I suspect there is
something else "interesting" with this zlib implementation. For the patch
above, can you remove the checking for remaining == 0 and print out the value
from deflate at each iterate of the loop. I'm wondering if it returns 0 before
def.finished() returns true.
-
PR: https://git.openjdk.org/jdk/pull/12283
Re: RFR: 8299748: java/util/zip/Deinflate.java failing on s390x
On Thu, 2 Feb 2023 10:06:23 GMT, Alan Bateman wrote:
>>> level:1, strategy: 0, dowrap: false
>>> is finished: false
>>
>> Thanks for checking that. So "is finished: false" is telling us that not all
>> of the input was compressed. So I think the right thing is to do the deflate
>> in a loop until there is more input to compress, the inflate probably needs
>> the same. Your original proposal was to make the output buffer larger and I
>> suspect that is just working around a threshold or block size used by the
>> accelerator.
>
>> Hi @AlanBateman ,
>> with latest changes (doing inflate/deinflate) test passes over s390 and x86
>> as well. Please take a look now.
>
> Good. One thing to try is to just deflate/inflate into out1/out2, no need for
> the intermediate BAOS, try this:
>
>
> --- a/test/jdk/java/util/zip/DeInflate.java
> +++ b/test/jdk/java/util/zip/DeInflate.java
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -127,11 +127,25 @@ public class DeInflate {
>
> def.setInput(in, 0, len);
> def.finish();
> -int m = def.deflate(out1);
> +int m = 0;
> +while (!def.finished()) {
> +int remaining = out1.length - m;
> +if (remaining == 0) {
> +throw new RuntimeException("out1 is small");
> +}
> +m += def.deflate(out1, m, remaining);
> +}
>
> Inflater inf = new Inflater(nowrap);
> inf.setInput(out1, 0, m);
> -int n = inf.inflate(out2);
> +int n = 0;
> +while (!inf.finished()) {
> +int remaining = out2.length - n;
> +if (remaining == 0) {
> +throw new RuntimeException("out2 is small");
> +}
> +n += inf.inflate(out2, n, remaining);
> +}
>
> if (n != len ||
> !Arrays.equals(Arrays.copyOf(in, len), Arrays.copyOf(out2, len))
> ||
@AlanBateman should we proceed with the current changes then ?
-
PR: https://git.openjdk.org/jdk/pull/12283
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java [v3]
On Mon, 6 Feb 2023 11:37:41 GMT, Raffaello Giulietti wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Correct overflow limit in regression test. > > test/jdk/java/lang/StrictMath/FdlibmTranslit.java line 938: > >> 936: >> 937: /* |x| in [log(maxdouble), overflowthresold] */ >> 938: lx = __LO(x); > > Same concern as above about the transliteration to `__LO(x)` Right; sinh and cosh overflow at the same input. - PR: https://git.openjdk.org/jdk/pull/12429
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v11]
On Tue, 7 Feb 2023 00:12:21 GMT, Scott Gibbons wrote: >> Added code for Base64 acceleration (encode and decode) which will accelerate >> ~4x for AVX2 platforms. >> >> Encode performance: >> **Old:** >> >> Benchmark (maxNumBytes) Mode Cnt Score Error >> Units >> Base64Encode.testBase64Encode 1024 thrpt3 4309.439 ± 2.632 >> ops/ms >> >> >> **New:** >> >> Benchmark (maxNumBytes) Mode Cnt Score >> Error Units >> Base64Encode.testBase64Encode 1024 thrpt3 24211.397 ± >> 102.026 ops/ms >> >> >> Decode performance: >> **Old:** >> >> Benchmark (errorIndex) (lineSize) (maxNumBytes) >> Mode Cnt ScoreError Units >> Base64Decode.testBase64Decode 144 4 1024 >> thrpt3 3961.768 ± 93.409 ops/ms >> >> **New:** >> Benchmark (errorIndex) (lineSize) (maxNumBytes) >> Mode Cnt ScoreError Units >> Base64Decode.testBase64Decode 144 4 1024 >> thrpt3 14738.051 ± 24.383 ops/ms > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Add algorithm comments src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2227: > 2225: > 2226: // lut_roll URL > 2227: __ emit_data64(0xb9b9bfbf04111000, relocInfo::none); The lut_roll URL doesn't seem to be correct: 0x5F (URL base64 ASCII for "/") would need an offset of -20H i.e. 0xEC. However the others with upper nibble as 5 need an offset of -65H i.e. 0xBF. It looks to me that the adjustment for 5F should be -4 instead of -1 at line 2722. - PR: https://git.openjdk.org/jdk/pull/12126
Re: RFR: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp [v2]
On Mon, 6 Feb 2023 15:35:30 GMT, Roger Riggs wrote:
> Please add @bug 8301737
>
> It is not going to be obvious why a larger code cashe is needed (and only for
> aarch64).
>
The error message in the .jtr file is `java.rmi.NoSuchObjectException: no such
object in table`, which corresponding source code is as follows：
// src/java.rmi/share/classes/sun/rmi/transport/Transport.java
176 if (target == null || (impl = target.getImpl()) == null) {
// target is null
177 throw new NoSuchObjectException("no such object in table");
178 }
why target is null with `-Xcomp`? then you see the other source code:
// src/java.rmi//share/classes/sun/rmi/transport/ObjectTable.java
346 private static class Reaper implements Runnable {
347
348 public void run() {
349 try {
350 do {
351 // wait for next cleared weak reference
352 WeakRef weakImpl = (WeakRef) reapQueue.remove(); //
Monitor GC here
353
354 synchronized (tableLock) {
355 Target target = implTable.get(weakImpl);
356 if (target != null) {
357 if (!target.isEmpty()) {
358 throw new Error(
359 "object with known references
collected");
360 } else if (target.isPermanent()) {
361 throw new Error("permanent object
collected");
362 }
363 removeTarget(target); // target
will be removed after GC. which happen before getTarget()
364 }
365 }
366 } while (!Thread.interrupted());
367 } catch (InterruptedException e) {
368 // pass away if interrupted
369 }
370 }
371 }
So I used `-Xlog:gc` to find that the trigger gc is caused by insufficient
codecache.
> Can you split the test runs so the original does not run on aarch64 and a new
> second run runs only on aarch64. For example,
>
> ```
> /*
> * @test
> * @bug 8301737
> * @summary Check that objects are exported with ObjectInputFilters via
> UnicastRemoteObject
> * @requires os.arch != "aarch64"
> * @run testng/othervm FilterUROTest
> */
>
> /*
> * @test
> * @summary Check that objects are exported with ObjectInputFilters via
> UnicastRemoteObject
> * @requires os.arch == "aarch64"
> * @run testng/othervm -XX:ReservedCodeCacheSize=512m FilterUROTest
> */
> ```
This bug only for LoongArch64 architecture, x86_64 and AArch64 is ok. Perhaps
the number of instructions under the LoongArch64 architecture is higher than in
aarch64/x86_64. I'm not sure if the s390/risc-v/ppc architecture has the same
problem, so I`m only use `@requires os.arch == "loongarch64" `?
-
PR: https://git.openjdk.org/jdk/pull/12399
Re: RFR: 8293198: [vectorapi] Improve the implementation of VectorMask.indexInRange() [v2]
On Mon, 6 Feb 2023 17:39:42 GMT, Paul Sandoz wrote:
>> Xiaohong Gong has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add smaller array size for benchmark tests
>
> I think it would be useful to adjust the naming and comments of some methods
> to make it clearer the method parameter constraints.
>
> `indexInRange0Helper` is now called if the index is partially or totally out
> of range at the lower or upper ends and `indexInRange0` is called if
> partially or totally out of range at the upper end.
> e.g. a more literal naming could be:
> `AbstractMask::indexInRange0Helper` ->
> `AbstractMask::indexPartiallyInRangeHelper`
> `VectorSupport::indexInRange` -> VectorSupport::indexPartiallyInUpperRange`
> ?
>
> IIUC the performance numbers show that when the array is not a multiple of
> the vector size there is still quite an impact overall to calling
> `VectorSupport::indexInRange` for the last loop iteration. I am guessing the
> overall loop shape is different which impacts other optimizations?
>
> To do this more optimally likely requires a loop transformation where the
> last loop iteration is peeled off, but that's a harder transformation in one
> of the more complicated areas of C2 (although it already supports pre/post
> loop, so maybe its possible to leverage that?).
Thanks for looking at this PR @PaulSandoz !
> I think it would be useful to adjust the naming and comments of some methods
> to make it clearer the method parameter constraints.
>
> `indexInRange0Helper` is now called if the index is partially or totally out
> of range at the lower or upper ends and `indexInRange0` is called if
> partially or totally out of range at the upper end. e.g. a more literal
> naming could be: `AbstractMask::indexInRange0Helper` ->
> `AbstractMask::indexPartiallyInRangeHelper` `VectorSupport::indexInRange` ->
> VectorSupport::indexPartiallyInUpperRange` ?
The renaming looks good to me. Thanks!
> IIUC the performance numbers show that when the array is not a multiple of
> the vector size there is still quite an impact overall to calling
> `VectorSupport::indexInRange` for the last loop iteration. I am guessing the
> overall loop shape is different which impacts other optimizations?
I think the main influence of the benchmark result comes from the masked `
fromArray()/intoArray()` APIs, especially the masked intoArray() API. For the
tail loop part, there is the vector boxing needed on all architectures, with
the following reasons:
- If the architecture doesn't support predicate feature, it cannot be
intrinsified.
- The `checkMaskFromIndexSize` called inside the `else->if` branch may not be
inlined, and the `indexInRange()` generated mask `m` needs the boxing before it.
public final
void intoArray(double[] a, int offset,
VectorMask m) {
if (m.allTrue()) {
intoArray(a, offset);
} else {
DoubleSpecies vsp = vspecies();
if (!VectorIntrinsics.indexInRange(offset, vsp.length(), a.length))
{
checkMaskFromIndexSize(offset, vsp, m, 1, a.length);
}
intoArray0(a, offset, m);
}
}
If the array size is aligned with the vector size, the generated `m` is all
true. Hence, the non-masked `intoArray()` is called instead, which improves the
performance a lot.
Regarding to the `indexInRange()` API implementation, if the array size is the
multiple num of vector size, the branch for the tail loop part is optimized out
to an uncommon-trap by C2 compiler, which may improves the performance as well.
Regarding to the added benchmark, since it is a testing for `indexInRange`,
maybe we can remove the calling to the masked `fromArray()/intoArray()` APIs
and directly save the mask into a boolean array instead. I guess this may
reduce the overall performance gap.
>
> To do this more optimally likely requires a loop transformation where the
> last loop iteration is peeled off, but that's a harder transformation in one
> of the more complicated areas of C2 (although it already supports pre/post
> loop, so maybe its possible to leverage that?).
Yes, it is!
-
PR: https://git.openjdk.org/jdk/pull/12064
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v4]
On Mon, 6 Feb 2023 09:46:15 GMT, Tagir F. Valeev wrote:
>> clamp() methods added to Math and StrictMath
>>
>> `int clamp(long, int, int)` is somewhat different, as it accepts a `long`
>> value and safely clamps it to an `int` range. Other overloads work with a
>> particular type (long, float and double). Using similar approach in other
>> cases (e.g. `float clamp(double, float, float)`) may cause accidental
>> precision loss even if the value is within range, so I decided to avoid this.
>>
>> In all cases, `max >= min` precondition should met. For double and float we
>> additionally order `-0.0 < 0.0`, similarly to what Math.max or
>> Double.compare do. In double and float overloads I try to keep at most one
>> arg-check comparison on common path, so the order of checks might look
>> unusual.
>>
>> For tests, I noticed that tests in java/lang/Math don't use any testing
>> framework (even newer tests), so I somehow mimic the approach of neighbour
>> tests.
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Added explanatory comments
test/jdk/java/lang/Math/Clamp.java line 47:
> 45: private static int testIntClamp() {
> 46: int failures = 0;
> 47: failures += checkIntClamp(0, 1, 2, 1);
Possible refactoring here: represent the test cases in, say, a 2D int array or
an array/list of records and loop over them.
-
PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v4]
On Mon, 6 Feb 2023 09:46:15 GMT, Tagir F. Valeev wrote:
>> clamp() methods added to Math and StrictMath
>>
>> `int clamp(long, int, int)` is somewhat different, as it accepts a `long`
>> value and safely clamps it to an `int` range. Other overloads work with a
>> particular type (long, float and double). Using similar approach in other
>> cases (e.g. `float clamp(double, float, float)`) may cause accidental
>> precision loss even if the value is within range, so I decided to avoid this.
>>
>> In all cases, `max >= min` precondition should met. For double and float we
>> additionally order `-0.0 < 0.0`, similarly to what Math.max or
>> Double.compare do. In double and float overloads I try to keep at most one
>> arg-check comparison on common path, so the order of checks might look
>> unusual.
>>
>> For tests, I noticed that tests in java/lang/Math don't use any testing
>> framework (even newer tests), so I somehow mimic the approach of neighbour
>> tests.
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Added explanatory comments
src/java.base/share/classes/java/lang/Math.java line 2266:
> 2264: // If min == max, we should additionally check for +0.0/-0.0
> case,
> 2265: // so we're still visiting the if statement.
> 2266: if (!(min < max)) {
A suggestion for an additional comment on this line like:
// min greater than, equal to, or unordered with respect to max; NaN values are
unorded
-
PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v11]
On Tue, 7 Feb 2023 00:12:21 GMT, Scott Gibbons wrote:
>> Added code for Base64 acceleration (encode and decode) which will accelerate
>> ~4x for AVX2 platforms.
>>
>> Encode performance:
>> **Old:**
>>
>> Benchmark (maxNumBytes) Mode Cnt Score Error
>> Units
>> Base64Encode.testBase64Encode 1024 thrpt3 4309.439 ± 2.632
>> ops/ms
>>
>>
>> **New:**
>>
>> Benchmark (maxNumBytes) Mode Cnt Score
>> Error Units
>> Base64Encode.testBase64Encode 1024 thrpt3 24211.397 ±
>> 102.026 ops/ms
>>
>>
>> Decode performance:
>> **Old:**
>>
>> Benchmark (errorIndex) (lineSize) (maxNumBytes)
>> Mode Cnt ScoreError Units
>> Base64Decode.testBase64Decode 144 4 1024
>> thrpt3 3961.768 ± 93.409 ops/ms
>>
>> **New:**
>> Benchmark (errorIndex) (lineSize) (maxNumBytes)
>> Mode Cnt ScoreError Units
>> Base64Decode.testBase64Decode 144 4 1024
>> thrpt3 14738.051 ± 24.383 ops/ms
>
> Scott Gibbons has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add algorithm comments
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2720:
> 2718: __ vpshufb(xmm5, xmm9, xmm1, Assembler::AVX_256bit);
> 2719: // If the and of the two is non-zero, we have an invalid input
> character
> 2720: __ vptest(xmm3, xmm5);
For isURL, it looks to me that the vptest will fail for URL valid input 0x5F
("_"):
upper_nibble = 0x5;
lower_nibble = 0xF;
lut_lo_URL = 0x1B; (corresponding to 0xF)
lut_hi = 0x8; (corresponding to 0x5)
lut_lo_URL & lut_hi = 0x8; (not zero, taken as not allowable and so exit from
loop)
Could you please verify on your end and fix this?
My understanding is that this is happening because 5 and 7 upper nibble get the
same encoding 0x8.
-
PR: https://git.openjdk.org/jdk/pull/12126
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v11]
> Added code for Base64 acceleration (encode and decode) which will accelerate > ~4x for AVX2 platforms. > > Encode performance: > **Old:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 4309.439 ± 2.632 > ops/ms > > > **New:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 24211.397 ± 102.026 > ops/ms > > > Decode performance: > **Old:** > > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 3961.768 ± 93.409 ops/ms > > **New:** > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 14738.051 ± 24.383 ops/ms Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Add algorithm comments - Changes: - all: https://git.openjdk.org/jdk/pull/12126/files - new: https://git.openjdk.org/jdk/pull/12126/files/cb271c00..0a274e5a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=09-10 Stats: 16 lines in 1 file changed: 14 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12126.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12126/head:pull/12126 PR: https://git.openjdk.org/jdk/pull/12126
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:25:17 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 333:
>
>> 331: * @see #catchingAll
>> 332: */
>> 333: CatchBuilder catching(ClassDesc exceptionType,
>> Consumer catchHandler);
>
> I imagine there are name clashes with Java keyword, hence the `ing` in the
> names. That said, this should probably revisited in a later bikeshedding
> round - as in the current form, the code builder API has most method names
> that are nouns (the thing being built) but it also has some verbs in there
> (trying, catching) which seem odd.
Please raise the naming convention discussion at classfile-api-dev at
openjdk.org
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/Opcode.java line 39:
>
>> 37: */
>> 38: public enum Opcode {
>> 39: NOP(Classfile.NOP, 1, Kind.NOP),
>
> This also duplicates the constants in classfile...
On the contrary, it has been deduplicated. Opcode is referencing numeric
constants stored in Classfile.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: [jdk20] RFR: 8301863: ObjectInputFilter example incorrectly calls rejectUndecidedClass [v2]
> The example code in ObjectInputFilter for the FilterInThread filter factory > does not do what is intended and is poorly described. Clarifies that the > JVM-wide filter and the thread local filter are merged and will reject > classes that are otherwise not accepted or rejected. If a stream specific > filter is set and does not accept or reject a class, the combined filter is > applied. > > This is a doc-only change. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: 8301863: ObjectInputFilter example incorrectly calls rejectUndecidedClass Correct typo in prose. - Changes: - all: https://git.openjdk.org/jdk20/pull/121/files - new: https://git.openjdk.org/jdk20/pull/121/files/a71e9cfa..aaf5d38c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk20&pr=121&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk20&pr=121&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk20/pull/121.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/121/head:pull/121 PR: https://git.openjdk.org/jdk20/pull/121
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java [v3]
On Mon, 6 Feb 2023 21:55:26 GMT, Joe Darcy wrote:
>> test/jdk/java/lang/StrictMath/FdlibmTranslit.java line 874:
>>
>>> 872: // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
>>> 873: // lx = (((*(unsigned*)&one)>>29)) + (unsigned*)&x ;
>>> 874: lx = __LO(x);
>>
>> This aspect of the transliteration is unclear.
>> The C code seems to ignore endianness of `double` values, and there's a
>> shift by 29 that seems intentional for some reason.
>> Are we safe with the `__LO(x)` transliteration?
>
> Yes, it gave me pause when doing the transliteration.
>
> Unpacking the code a bit,
>
>
> /* |x| in [log(maxdouble), overflowthresold] */
> // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> lx = __LO(x);
> if (ix<0x408633CE ||
> ((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 )))
> {...}
>
>
> Assuming the comment accurate describe the intention of the code, identifying
> values between log(maxdouble) and the overflow threshold. Those particular
> values are as decimal fp, hex hp, and long bits:
>
> 709.7827128933840x1.62e42fefa39efp940862e42__fefa39ef
> 710.47586007394390x1.633ce8fb9f87dp9408633ce_8fb9f87d
>
> so the conditional is checking if the high word (ix) is for something less
> than the low end of the range OR if the high word matches the high word for
> the high end of the range AND low bits are for less than the high end of the
> range. Therefore, I think taking __LO(x) is the correct semantics here.
>
> FWIW, the ExhaustingTest of running all the float values against
> sinh/cosh/tanh in the transliteration port passes when using JDK 20 baseline
> as a reference.
PS One possible future refactoring to the code in
src/java.base/share/classes/java/lang/FdLibm.java
would be to replace such integer-based range checks with floating-point based
range checks, in this case something like:
if (absX <= 0x1.633ce8fb9f87dp9) { // 710.4758600739439
...}
-
PR: https://git.openjdk.org/jdk/pull/12429
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java [v3]
> Initial pass of porting FDLIBM sinh/cosh/tanh to Java. I do intend to
> refactor the regression tests a bit to reduce duplication, but the actual
> ports should be ready for review.
>
> Diff'ing the ports as before, original vs transliteration port:
>
>
> $ diff -w Hyperbolic.c Hyperbolic.translit.java
> 1c1
> < /* __ieee754_sinh(x)
> ---
>> /**
> 17a18,19
>> static class Sinh {
>> private static final double one = 1.0, shuge = 1.0e307;
> 19,33c21
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, shuge = 1.0e307;
> < #else
> < static double one = 1.0, shuge = 1.0e307;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_sinh(double x)
> < #else
> < double __ieee754_sinh(x)
> < double x;
> < #endif
> < {
> ---
>> private static double compute(double x) {
> 36c24
> < unsigned lx;
> ---
>> /* unsigned */ int lx;
> 51c39
> < t = expm1(fabs(x));
> ---
>> t = FdlibmTranslit.expm1(Math.abs(x));
> 57c45
> < if (ix < 0x40862E42) return h*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return h*StrictMath.exp(Math.abs(x)); //
>> TODO switch to translit
> 60,62c48,52
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> < if (ix<0x408633CE ||
> ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(0.5*fabs(x));
> ---
>> // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
>> // lx = (((*(unsigned*)&one)>>29)) + (unsigned*)&x ;
>> lx = __LO(x);
>> if (ix<0x408633CE ||
>> ((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 ))) {
>> w = StrictMath.exp(0.5*Math.abs(x)); // TODO switch to
>> translit
> 70c60,62
> < /* __ieee754_cosh(x)
> ---
>> }
>>
>> /**
> 90,105c82,84
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, half=0.5, huge = 1.0e300;
> < #else
> < static double one = 1.0, half=0.5, huge = 1.0e300;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_cosh(double x)
> < #else
> < double __ieee754_cosh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Cosh {
>> private static final double one = 1.0, half=0.5, huge = 1.0e300;
>> private static double compute(double x) {
> 108c87
> < unsigned lx;
> ---
>> /*unsigned*/ int lx;
> 119c98
> < t = expm1(fabs(x));
> ---
>> t = expm1(Math.abs(x));
> 127c106
> < t = __ieee754_exp(fabs(x));
> ---
>> t = StrictMath.exp(Math.abs(x)); // TODO switch to translit
> 132c111
> < if (ix < 0x40862E42) return half*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return half*StrictMath.exp(Math.abs(x));
>> // TODO switch to translit
> 135c114
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> ---
>> lx = __LO(x);
> 137,138c116,117
> < ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(half*fabs(x));
> ---
>> ((ix==0x408633ce)&&(Integer.compareUnsigned(lx, 0x8fb9f87d)
>> <= 0))) {
>> w = StrictMath.exp(half*Math.abs(x)); // TODO switch to
>> translit
> 146c125,127
> < /* Tanh(x)
> ---
>> }
>>
>> /**
> 169,184c150,152
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one=1.0, two=2.0, tiny = 1.0e-300;
> < #else
> < static double one=1.0, two=2.0, tiny = 1.0e-300;
> < #endif
> <
> < #ifdef __STDC__
> < double tanh(double x)
> < #else
> < double tanh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Tanh {
>> private static final double one=1.0, two=2.0, tiny = 1.0e-300;
>> static double compute(double x) {
> 203c171
> < t = expm1(two*fabs(x));
> ---
>> t = expm1(two*Math.abs(x));
> 206c174
> < t = expm1(-two*fabs(x));
> ---
>> t = expm1(-two*Math.abs(x));
> 214a183
>> }
>
>
> Note that the original has a in-line version of the "__LO" macro rather than
> using the macro.
>
>
> And transliteration vs more idiomatic:
>
>
> $ diff -w Hyperbolic.translit.java Hyperbolic.fdlibm.java
> 21c21
> < private static double compute(double x) {
> ---
>> static double compute(double x) {
> 26c26
> < /* High word of |x|. */
> ---
>> // High word of |x|
> 28c28
> < ix = jx&0x7fff;
> ---
>> ix = jx & 0x7fff_;
> 30,31c30,33
> < /* x is INF or NaN */
> < if(ix>=0x7ff0) return x+x;
> ---
>> // x is INF or NaN
>> if ( ix >= 0x7ff0_) {
>> return x + x;
>> }
> 34,40c36,48
> < if (jx<0) h = -h;
> < /* |x| in [0,22], return sign(x)*0.5*(E+E/(E+1))) */
> <
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java [v2]
On Mon, 6 Feb 2023 11:34:40 GMT, Raffaello Giulietti
wrote:
>> Joe Darcy has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Two more spacing fixes.
>> - Implement spacing improvements from code review comments.
>
> test/jdk/java/lang/StrictMath/FdlibmTranslit.java line 874:
>
>> 872: // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
>> 873: // lx = (((*(unsigned*)&one)>>29)) + (unsigned*)&x ;
>> 874: lx = __LO(x);
>
> This aspect of the transliteration is unclear.
> The C code seems to ignore endianness of `double` values, and there's a shift
> by 29 that seems intentional for some reason.
> Are we safe with the `__LO(x)` transliteration?
Yes, it gave me pause when doing the transliteration.
Unpacking the code a bit,
/* |x| in [log(maxdouble), overflowthresold] */
// lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
lx = __LO(x);
if (ix<0x408633CE ||
((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 )))
{...}
Assuming the comment accurate describe the intention of the code, identifying
values between log(maxdouble) and the overflow threshold. Those particular
values are as decimal fp, hex hp, and long bits:
709.7827128933840x1.62e42fefa39efp940862e42__fefa39ef
710.47586007394390x1.633ce8fb9f87dp9408633ce_8fb9f87d
so the conditional is checking if the high word (ix) is for something less than
the low end of the range OR if the high word matches the high word for the high
end of the range AND low bits are for less than the high end of the range.
Therefore, I think taking __LO(x) is the correct semantics here.
FWIW, the ExhaustingTest of running all the float values against sinh/cosh/tanh
in the transliteration port passes when using JDK 20 baseline as a reference.
-
PR: https://git.openjdk.org/jdk/pull/12429
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java [v2]
> Initial pass of porting FDLIBM sinh/cosh/tanh to Java. I do intend to
> refactor the regression tests a bit to reduce duplication, but the actual
> ports should be ready for review.
>
> Diff'ing the ports as before, original vs transliteration port:
>
>
> $ diff -w Hyperbolic.c Hyperbolic.translit.java
> 1c1
> < /* __ieee754_sinh(x)
> ---
>> /**
> 17a18,19
>> static class Sinh {
>> private static final double one = 1.0, shuge = 1.0e307;
> 19,33c21
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, shuge = 1.0e307;
> < #else
> < static double one = 1.0, shuge = 1.0e307;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_sinh(double x)
> < #else
> < double __ieee754_sinh(x)
> < double x;
> < #endif
> < {
> ---
>> private static double compute(double x) {
> 36c24
> < unsigned lx;
> ---
>> /* unsigned */ int lx;
> 51c39
> < t = expm1(fabs(x));
> ---
>> t = FdlibmTranslit.expm1(Math.abs(x));
> 57c45
> < if (ix < 0x40862E42) return h*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return h*StrictMath.exp(Math.abs(x)); //
>> TODO switch to translit
> 60,62c48,52
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> < if (ix<0x408633CE ||
> ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(0.5*fabs(x));
> ---
>> // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
>> // lx = (((*(unsigned*)&one)>>29)) + (unsigned*)&x ;
>> lx = __LO(x);
>> if (ix<0x408633CE ||
>> ((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 ))) {
>> w = StrictMath.exp(0.5*Math.abs(x)); // TODO switch to
>> translit
> 70c60,62
> < /* __ieee754_cosh(x)
> ---
>> }
>>
>> /**
> 90,105c82,84
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, half=0.5, huge = 1.0e300;
> < #else
> < static double one = 1.0, half=0.5, huge = 1.0e300;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_cosh(double x)
> < #else
> < double __ieee754_cosh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Cosh {
>> private static final double one = 1.0, half=0.5, huge = 1.0e300;
>> private static double compute(double x) {
> 108c87
> < unsigned lx;
> ---
>> /*unsigned*/ int lx;
> 119c98
> < t = expm1(fabs(x));
> ---
>> t = expm1(Math.abs(x));
> 127c106
> < t = __ieee754_exp(fabs(x));
> ---
>> t = StrictMath.exp(Math.abs(x)); // TODO switch to translit
> 132c111
> < if (ix < 0x40862E42) return half*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return half*StrictMath.exp(Math.abs(x));
>> // TODO switch to translit
> 135c114
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> ---
>> lx = __LO(x);
> 137,138c116,117
> < ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(half*fabs(x));
> ---
>> ((ix==0x408633ce)&&(Integer.compareUnsigned(lx, 0x8fb9f87d)
>> <= 0))) {
>> w = StrictMath.exp(half*Math.abs(x)); // TODO switch to
>> translit
> 146c125,127
> < /* Tanh(x)
> ---
>> }
>>
>> /**
> 169,184c150,152
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one=1.0, two=2.0, tiny = 1.0e-300;
> < #else
> < static double one=1.0, two=2.0, tiny = 1.0e-300;
> < #endif
> <
> < #ifdef __STDC__
> < double tanh(double x)
> < #else
> < double tanh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Tanh {
>> private static final double one=1.0, two=2.0, tiny = 1.0e-300;
>> static double compute(double x) {
> 203c171
> < t = expm1(two*fabs(x));
> ---
>> t = expm1(two*Math.abs(x));
> 206c174
> < t = expm1(-two*fabs(x));
> ---
>> t = expm1(-two*Math.abs(x));
> 214a183
>> }
>
>
> Note that the original has a in-line version of the "__LO" macro rather than
> using the macro.
>
>
> And transliteration vs more idiomatic:
>
>
> $ diff -w Hyperbolic.translit.java Hyperbolic.fdlibm.java
> 21c21
> < private static double compute(double x) {
> ---
>> static double compute(double x) {
> 26c26
> < /* High word of |x|. */
> ---
>> // High word of |x|
> 28c28
> < ix = jx&0x7fff;
> ---
>> ix = jx & 0x7fff_;
> 30,31c30,33
> < /* x is INF or NaN */
> < if(ix>=0x7ff0) return x+x;
> ---
>> // x is INF or NaN
>> if ( ix >= 0x7ff0_) {
>> return x + x;
>> }
> 34,40c36,48
> < if (jx<0) h = -h;
> < /* |x| in [0,22], return sign(x)*0.5*(E+E/(E+1))) */
> <
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java [v2]
On Mon, 6 Feb 2023 08:33:37 GMT, Andrey Turbanov wrote:
>> Joe Darcy has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Two more spacing fixes.
>> - Implement spacing improvements from code review comments.
>
> src/java.base/share/classes/java/lang/FdLibm.java line 1233:
>
>> 1231:
>> 1232: h = 0.5;
>> 1233: if ( jx < 0) {
>
> Suggestion:
>
> if (jx < 0) {
Spacing fixed; thanks @turbanoff .
-
PR: https://git.openjdk.org/jdk/pull/12429
[jdk20] RFR: 8301864: ObjectInputFilter example incorrectly calls rejectUndecideClass
The example code in ObjectInputFilter for the FilterInThread filter factory does not do what is intended and is poorly described. Clarifies that the JVM-wide filter and the thread local filter are merged and will reject classes that are otherwise not accepted or rejected. If a stream specific filter is set and does not accept or reject a class, the combined filter is applied. This is a doc-only change. - Commit messages: - 8301864: ObjectInputFilter example incorrectly calls rejectUndecidedClass Changes: https://git.openjdk.org/jdk20/pull/121/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=121&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8301864 Stats: 19 lines in 1 file changed: 4 ins; 9 del; 6 mod Patch: https://git.openjdk.org/jdk20/pull/121.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/121/head:pull/121 PR: https://git.openjdk.org/jdk20/pull/121
Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v5]
> This is an approach to adding a flag to jlink that will allow --compress to > take the same types of arguments as jmod, thus bringing the two into > alignment. This likely requires a CSR and a discussion on whether we should > deprecate or simply remove the original numeric compression arguments. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Adding plugin type to force compact strings vs zip ordering - Changes: - all: https://git.openjdk.org/jdk/pull/11617/files - new: https://git.openjdk.org/jdk/pull/11617/files/41e52039..06f4b3c3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11617&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11617&range=03-04 Stats: 6 lines in 3 files changed: 3 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/11617.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11617/head:pull/11617 PR: https://git.openjdk.org/jdk/pull/11617
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v10]
> Added code for Base64 acceleration (encode and decode) which will accelerate > ~4x for AVX2 platforms. > > Encode performance: > **Old:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 4309.439 ± 2.632 > ops/ms > > > **New:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 24211.397 ± 102.026 > ops/ms > > > Decode performance: > **Old:** > > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 3961.768 ± 93.409 ops/ms > > **New:** > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 14738.051 ± 24.383 ops/ms Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Remove redundant cmp from inner loop of encode - Changes: - all: https://git.openjdk.org/jdk/pull/12126/files - new: https://git.openjdk.org/jdk/pull/12126/files/a8ecc15a..cb271c00 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=08-09 Stats: 7 lines in 1 file changed: 1 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12126.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12126/head:pull/12126 PR: https://git.openjdk.org/jdk/pull/12126
Re: Update TestTooManyEntries to run non-manual
On Sat, Jan 28, 2023 at 5:29 PM Lance Andersen wrote: > I think it is fine to move the validation immediately following findEND() > though I am not sure there is a big win overall. > > I also would prefer to see the test based off of an actual ZIP(or at least > have the current/modified version of the test). > Hi, I updated the PR to "inflate" the CEN such that its size on disk matches the size stated in the END record. As suggested by Alan, this is done using sparse files, the actual disk usage is kept low. While a ZIP with a zero-padded CEN is still not technically valid, the END record is now at least valid with respect to its offset. This allows us to test the validation of the various END record validation scenarios independently and without updating the ZipFile validation order to facilitate the testing. There are now three test methods in the test, one for each of the possible ZipExceptions thrown during END record validation. Cheers, Eirik.
Re: RFR: JDK-8301833: Add manual tests for FDLIBM porting
On Mon, 6 Feb 2023 09:18:35 GMT, Alan Bateman wrote: > Manual tests are the tests of last resort :-) This test may be useful for the > current transliteration work but it's not clear how this manual test would be > run by someone tasked with running the manual tests. Right now, it looks like > it's more of a long running stress test but I think the expectation is that > someone running this test needs to do a special build or somehow compare > results with an older JDK release. Have you explored alternatives to adding a > manual test? Long running HotSpot stress tests run in higher tiers. For > comparison, I'm wondering if samples could be captured in a golden file for > the test to use. > > If we are adding a manual test here then I think it will need good > instructions. @bwhuang-us has done work in recent times on making sure that > the manual tests are in test groups with instructions. To add some quick background: the FDLIBM porting effort is now doing two ports per method: * A transliteration port that is as close to the original FDLIBM C as possible * A (more) idiomatic port closed to regular Java coding In the future, there may be further changes to the idiomatic port to enhance performance or clarity _while returning the same results_, etc. These exhaustive tests are helpful now to provide another layer of checking to the transliteration port (by validating it against say, a JDK 20 build) and also helpful in the future to validate any implementation optimization to the idiomatic port when run against the transliteration port. The tests are long-running (one of the order of at least a minute or two per method under test) and in my estimation don't provide enough utility for their cost to be run all the time. However, they have much higher utility to help validate this initial port and for future refactorings. Therefore, I think these tests should be included in the repository, but not run all the time, which led me to declare them as a manual jtreg test. - PR: https://git.openjdk.org/jdk/pull/12430
Re: RFR: 8293198: [vectorapi] Improve the implementation of VectorMask.indexInRange() [v2]
On Fri, 3 Feb 2023 07:13:10 GMT, Xiaohong Gong wrote:
>> The Vector API `"indexInRange(int offset, int limit)"` is used
>> to compute a vector mask whose lanes are set to true if the
>> index of the lane is inside the range specified by the `"offset"`
>> and `"limit"` arguments, otherwise the lanes are set to false.
>>
>> There are two special cases for this API:
>> 1) If `"offset >= 0 && offset >= limit"`, all the lanes of the
>> generated mask are false.
>> 2) If` "offset >= 0 && limit - offset >= vlength"`, all the
>> lanes of the generated mask are true. Note that `"vlength"` is
>> the number of vector lanes.
>>
>> For such special cases, we can simply use `"maskAll(false|true)"`
>> to implement the API. Otherwise, the original comparison with
>> `"iota" `vector is needed. And for further optimization, we have
>> optimal instruction supported by SVE (i.e. whilelo [1]), which
>> can implement the API directly if the `"offset >= 0"`.
>>
>> As a summary, to optimize the API, we can use the if-else branches
>> to handle the specific cases in java level and intrinsify the
>> remaining case by C2 compiler:
>>
>>
>> public VectorMask indexInRange(int offset, int limit) {
>> if (offset < 0) {
>> return this.and(indexInRange0Helper(offset, limit));
>> } else if (offset >= limit) {
>> return this.and(vectorSpecies().maskAll(false));
>> } else if (limit - offset >= length()) {
>> return this.and(vectorSpecies().maskAll(true));
>> }
>> return this.and(indexInRange0(offset, limit));
>> }
>>
>>
>> The last part (i.e. `"indexInRange0"`) in the above implementation
>> is expected to be intrinsified by C2 compiler if the necessary IRs
>> are supported. Otherwise, it will fall back to the original API
>> implementation (i.e. `"indexInRange0Helper"`). Regarding to the
>> intrinsifaction, the compiler will generate `"VectorMaskGen"` IR
>> with "limit - offset" as the input if the current platform supports
>> it. Otherwise, it generates `"VectorLoadConst + VectorMaskCmp"` based
>> on `"iota < limit - offset"`.
>>
>> For the following java code which uses `"indexInRange"`:
>>
>>
>> static final VectorSpecies SPECIES =
>>DoubleVector.SPECIES_PREFERRED;
>> static final int LENGTH = 1027;
>>
>> public static double[] da;
>> public static double[] db;
>> public static double[] dc;
>>
>> private static void func() {
>> for (int i = 0; i < LENGTH; i += SPECIES.length()) {
>> var m = SPECIES.indexInRange(i, LENGTH);
>> var av = DoubleVector.fromArray(SPECIES, da, i, m);
>> av.lanewise(VectorOperators.NEG).intoArray(dc, i, m);
>> }
>> }
>>
>>
>> The core code generated with SVE 256-bit vector size is:
>>
>>
>> ptrue p2.d ; maskAll(true)
>> ...
>> LOOP:
>> ...
>> sub w11, w13, w14 ; limit - offset
>> cmp w14, w13
>> b.csLABEL-1 ; if (offset >= limit) => uncommon-trap
>> cmp w11, #0x4
>> b.ltLABEL-2 ; if (limit - offset < vlength)
>> mov p1.b, p2.b
>> LABEL-3:
>> ld1d{z16.d}, p1/z, [x10] ; load vector masked
>> ...
>> cmp w14, w29
>> b.ccLOOP
>> ...
>> LABEL-2:
>> whilelo p1.d, x16, x10; VectorMaskGen
>> ...
>> b LABEL-3
>> ...
>> LABEL-1:
>> uncommon-trap
>>
>>
>> Please note that if the array size `LENGTH` is aligned with
>> the vector size 256 (i.e. `LENGTH = 1024`), the branch "LABEL-2"
>> will be optimized out by compiler and it becomes another
>> uncommon-trap.
>>
>> For NEON, the main CFG is the same with above. But the compiler
>> intrinsification is different. Here is the code:
>>
>>
>> sub x10, x10, x12 ; limit - offset
>> scvtf d16, x10
>> dup v16.2d, v16.d[0] ; replicateD
>>
>> mov x8, #0xd8d0
>> movkx8, #0x84cb, lsl #16
>> movkx8, #0x, lsl #32
>> ldr q17, [x8], #0 ; load the "iota" const vector
>> fcmgt v18.2d, v16.2d, v17.2d ; mask = iota < limit - offset
>>
>>
>> Here is the performance data of the new added benchmark on an ARM
>> SVE 256-bit platform:
>>
>>
>> Benchmark (size) BeforeAfter Units
>> IndexInRangeBenchmark.byteIndexInRange 1024 11203.697 41404.431 ops/ms
>> IndexInRangeBenchmark.byteIndexInRange 1027 2365.920 8747.004 ops/ms
>> IndexInRangeBenchmark.doubleIndexInRange 1024 1227.505 6092.194 ops/ms
>> IndexInRangeBenchmark.doubleIndexInRange 1027 351.215 1156.683 ops/ms
>> IndexInRangeBenchmark.floatIndexInRange 1024 1468.876 11032.580 ops/ms
>> IndexInRangeBenchmark.floatIndexInRange 1027 699.645 2439.671 ops/ms
>> IndexInRangeBenchmark.intIndexInRange1024 2842.187 11903.544 ops/ms
>> IndexInRangeBenchmark.intIndexInRange1027 689.866 2547.424 ops/ms
>> IndexInRangeBenchmark.longIndexInRange 1024 1394.135 5902.973 ops/ms
>> IndexInRangeBenchmark.longIndexInRange 102
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v9]
> Added code for Base64 acceleration (encode and decode) which will accelerate > ~4x for AVX2 platforms. > > Encode performance: > **Old:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 4309.439 ± 2.632 > ops/ms > > > **New:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 24211.397 ± 102.026 > ops/ms > > > Decode performance: > **Old:** > > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 3961.768 ± 93.409 ops/ms > > **New:** > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 14738.051 ± 24.383 ops/ms Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision: Removal of a redundant cmp in inner loop. - Changes: - all: https://git.openjdk.org/jdk/pull/12126/files - new: https://git.openjdk.org/jdk/pull/12126/files/80fbf4ef..a8ecc15a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=07-08 Stats: 7 lines in 1 file changed: 2 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12126.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12126/head:pull/12126 PR: https://git.openjdk.org/jdk/pull/12126
Integrated: 8300259: Add test coverage for processing of pending block files in signed JARs
On Mon, 16 Jan 2023 11:44:36 GMT, Eirik Bjorsnos wrote: > This PR adds test coverage for pending block files in signed JAR files > > A signed JAR has pending block files if the block file [RSA, DSA, EC] comes > before the corresponding signature file [SF] in the JAR. > > JarVerifier.processEntry supports processing of such pending block files, but > this code path does not seem to be exercised by current test. > > The new test PendingBlocksJar checks that signed JARs with pending blocks > are processed correctly, both for the valid and invalid cases. This pull request has now been integrated. Changeset: c129ce46 Author:Eirik Bjorsnos Committer: Weijun Wang URL: https://git.openjdk.org/jdk/commit/c129ce4660e6c9b5365c8bf89fb916e2a7c28e98 Stats: 173 lines in 1 file changed: 173 ins; 0 del; 0 mod 8300259: Add test coverage for processing of pending block files in signed JARs Reviewed-by: weijun - PR: https://git.openjdk.org/jdk/pull/12009
Re: java.util.Date.parse(String) doesn't declare thrown IllegalArgumentException
Hi Andrey, Instead, perhaps replace (carefully) the remaining uses in the JDK. Developers should be using the correct APIs and not reading deprecated javadoc. Regards, Roger On 2/6/23 11:41 AM, Andrey Turbanov wrote: Hello. I've noticed that method 'long java.util.Date.parse(String)' doesn't have a reference to IllegalArgumentException in its javadoc. https://docs.oracle.com/javase/7/docs/api/java/util/Date.html#parse(java.lang.String) But this exception is thrown in implementation. I know that this method is deprecated, but it still is used within JDK code itself. Is it intentional that exception is not documented in this method? Andrey Turbanov
Re: RFR: JDK-8301396: Port fdlibm expm1 to Java [v2]
On Mon, 6 Feb 2023 08:19:36 GMT, Alan Bateman wrote: > @jddarcy Are you planning a GC of unused functions in StrictMath.c too? (for > this PR I'm wondering about Java_java_lang_StrictMath_expm1). Yes, once the port is done, I'll remove all the remaining FDLIBM C files. There are dependencies between the different C files, sinh calls expm1, etc., so to avoid needed to untangle all of those, I was going to do the removal in one step at the end. - PR: https://git.openjdk.org/jdk/pull/12394
java.util.Date.parse(String) doesn't declare thrown IllegalArgumentException
Hello. I've noticed that method 'long java.util.Date.parse(String)' doesn't have a reference to IllegalArgumentException in its javadoc. https://docs.oracle.com/javase/7/docs/api/java/util/Date.html#parse(java.lang.String) But this exception is thrown in implementation. I know that this method is deprecated, but it still is used within JDK code itself. Is it intentional that exception is not documented in this method? Andrey Turbanov
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v2]
On Mon, 6 Feb 2023 15:56:09 GMT, Claes Redestad wrote: >> Eirik Bjorsnos has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Spelling fix in test data for non-ascii latin1 string > > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 153: > >> 151: // Need to skip past the length of the name and extra fields >> 152: int nlen = buffer.getShort(off + NLEN); >> 153: int elen = buffer.getShort(off + NLEN); > > Is this supposed to say `ELEN`? Indeed, good catch! Must have lucked out on the comment be longer than the name :-) - PR: https://git.openjdk.org/jdk/pull/12290
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v3]
> After finding a hash match, getEntryPos needs to compare the lookup name up > to the encoded entry name in the CEN. This comparison is done by decoding the > entry name into a String. The names can then be compared using the String > API. This decoding step adds a significat cost to this method. > > This PR suggest to update the string comparison such that in the common case > where both the lookup name and the entry name are encoded in > ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can > instead be compared direcly. > > ZipCoder is updated with a new method to compare a string with an encoded > byte array range. The default implementation decodes to string (like the > current code), while the UTF-8 implementation uses > JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses > Arrays.mismatch for comparison with or without matching trailing slashes. > > Additionally, this PR suggest to make the following updates to getEntryPos: > > - The try/catch for IAE is redundant and can be safely removed. (initCEN > already checks this and will throws IAE for invalid UTF-8). This seems to > give a 3-4% speedup on micros) > - A new test InvalidBytesInEntryNameOrComment is a added to verify that > initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I > found no existing test coverage for this) > - The recursion when looking for "name/" matches is replaced with iteration. > We keep track of any "name/" match and return it at the end of the search. (I > feel this is easier to follow and it also gives a ~30% speedup for addSlash > lookups with no regression on regular lookups) > > (My though is that including these additional updates in this PR might reduce > reviewer overhead given that it touches the exact same code. I might be wrong > on this, please advise :) > > I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified > to use xalan.jar): > > Baseline: > > Benchmark (size) Mode CntScore Error > Units > ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 > ns/op > ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 > ns/op > ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 > ns/op > ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 > ns/op > ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 > ns/op > ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 > ns/op > ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 > ns/op > ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 > ns/op > ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 > ns/op > ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 > ns/op > > > > PR: > > > Benchmark (size) Mode CntScore Error > Units > ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 > ns/op > ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 > ns/op > ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 > ns/op > ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 > ns/op > ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 > ns/op > ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 > ns/op > ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 > ns/op > ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 > ns/op > ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 > ns/op > ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 > ns/op > > > To assess the impact on startup/warmup, I made a main method class which > measures the total time of calling ZipFile.getEntry for all entries in the > 109 JAR file dependenies of spring-petclinic. The shows a nice improvement > (time in micros): > > > Percentile Baseline Patch > 50 % 23155 21149 > 75 % 23598 21454 > 90 % 23989 21691 > 95 % 24238 21973 > 99 % 25270 22446 > STDEV 792 549 > Count 500 500 Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Fix incorrect offset for the CEN "length of extra field". Fixed spelling of "invalid". - Changes: - all: https://git.openjdk.org/jdk/pull/12290/files - new: https://git.openjdk.org/jdk/pull/12290/files/d0c12325..2c5e7c2c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12290.diff Fetch: git fetch
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v2]
On Mon, 6 Feb 2023 15:21:10 GMT, Eirik Bjorsnos wrote: >> After finding a hash match, getEntryPos needs to compare the lookup name up >> to the encoded entry name in the CEN. This comparison is done by decoding >> the entry name into a String. The names can then be compared using the >> String API. This decoding step adds a significat cost to this method. >> >> This PR suggest to update the string comparison such that in the common case >> where both the lookup name and the entry name are encoded in >> ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can >> instead be compared direcly. >> >> ZipCoder is updated with a new method to compare a string with an encoded >> byte array range. The default implementation decodes to string (like the >> current code), while the UTF-8 implementation uses >> JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses >> Arrays.mismatch for comparison with or without matching trailing slashes. >> >> Additionally, this PR suggest to make the following updates to getEntryPos: >> >> - The try/catch for IAE is redundant and can be safely removed. (initCEN >> already checks this and will throws IAE for invalid UTF-8). This seems to >> give a 3-4% speedup on micros) >> - A new test InvalidBytesInEntryNameOrComment is a added to verify that >> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I >> found no existing test coverage for this) >> - The recursion when looking for "name/" matches is replaced with iteration. >> We keep track of any "name/" match and return it at the end of the search. >> (I feel this is easier to follow and it also gives a ~30% speedup for >> addSlash lookups with no regression on regular lookups) >> >> (My though is that including these additional updates in this PR might >> reduce reviewer overhead given that it touches the exact same code. I might >> be wrong on this, please advise :) >> >> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified >> to use xalan.jar): >> >> Baseline: >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 >> ns/op >> >> >> >> PR: >> >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 >> ns/op >> >> >> To assess the impact on startup/warmup, I made a main method class which >> measures the total time of calling ZipFile.getEntry for all entries in the >> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement >> (time in micros): >> >> >> Percentile Baseline Patch >> 50 % 23155 21149 >> 75 % 23598 21454 >> 90 % 23989 21691 >> 95 % 24238 21973 >> 99 % 25270 22446 >> STDEV 792 549 >> Count 500 500 > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Spelling fix in test data for non-ascii latin1 string I think this looks good. Glad we can get this done without adding even more ways to peek into `String` internals. test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 158: > 156: int coff = off + CEN_HDR + nlen + elen; > 157: > 158: // Write inva
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v2]
On Mon, 6 Feb 2023 15:21:10 GMT, Eirik Bjorsnos wrote: >> After finding a hash match, getEntryPos needs to compare the lookup name up >> to the encoded entry name in the CEN. This comparison is done by decoding >> the entry name into a String. The names can then be compared using the >> String API. This decoding step adds a significat cost to this method. >> >> This PR suggest to update the string comparison such that in the common case >> where both the lookup name and the entry name are encoded in >> ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can >> instead be compared direcly. >> >> ZipCoder is updated with a new method to compare a string with an encoded >> byte array range. The default implementation decodes to string (like the >> current code), while the UTF-8 implementation uses >> JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses >> Arrays.mismatch for comparison with or without matching trailing slashes. >> >> Additionally, this PR suggest to make the following updates to getEntryPos: >> >> - The try/catch for IAE is redundant and can be safely removed. (initCEN >> already checks this and will throws IAE for invalid UTF-8). This seems to >> give a 3-4% speedup on micros) >> - A new test InvalidBytesInEntryNameOrComment is a added to verify that >> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I >> found no existing test coverage for this) >> - The recursion when looking for "name/" matches is replaced with iteration. >> We keep track of any "name/" match and return it at the end of the search. >> (I feel this is easier to follow and it also gives a ~30% speedup for >> addSlash lookups with no regression on regular lookups) >> >> (My though is that including these additional updates in this PR might >> reduce reviewer overhead given that it touches the exact same code. I might >> be wrong on this, please advise :) >> >> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified >> to use xalan.jar): >> >> Baseline: >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 >> ns/op >> >> >> >> PR: >> >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 >> ns/op >> >> >> To assess the impact on startup/warmup, I made a main method class which >> measures the total time of calling ZipFile.getEntry for all entries in the >> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement >> (time in micros): >> >> >> Percentile Baseline Patch >> 50 % 23155 21149 >> 75 % 23598 21454 >> 90 % 23989 21691 >> 95 % 24238 21973 >> 99 % 25270 22446 >> STDEV 792 549 >> Count 500 500 > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Spelling fix in test data for non-ascii latin1 string test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 153: > 151: // Need to skip past the length of the name and extra fields > 152: int nlen = buffer.getShort(off + NLEN); > 153: int elen = buffer.getShort(off + NLEN); Is this supposed to
Re: RFR: 8301834: Templated Buffer classes leave a lot of empty lines in the generated source
On Mon, 6 Feb 2023 06:57:43 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8301834? > > Some classes in `java.nio` package are generated from template files, during > the build. The template files are processed by a build tool implemented by a > Java class `make/jdk/src/classes/build/tools/spp/Spp.java`. This template > processing tool allows for an (optional) parameter called `-nel` which as per > its documentation: > >> If -nel is declared then empty lines will not be substituted for lines of >> text in the template that do not appear in the output. > > Various places in the JDK build where this tool is used to generate source > from template files, already use the `-nel` option to not generate the empty > lines in the source files. However, the `GensrcBuffer.gmk` which generates > the source for `java.nio` classes doesn't use this option. The commit in this > PR adds this option when generating the `java.nio` classes. > > Existing tests in `test/jdk/java/nio` continue to pass after this change. > I've checked the generated content and compared it with the older versions to > verify that these empty lines no longer appear in these generated classes. > > Additional `tier` testing has been triggered to make sure no regression is > introduced. Would it be feasible (and useful) to maybe just in release builds have these removed? Or maybe create some mappings to the templates for the debugging purposes? Some classes get very unreadable with these. - PR: https://git.openjdk.org/jdk/pull/12431
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v2]
On Mon, 6 Feb 2023 15:14:37 GMT, Eirik Bjorsnos wrote: >> Eirik Bjorsnos has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Spelling fix in test data for non-ascii latin1 string > > test/jdk/java/util/zip/ZipFile/TestZipFileEncodings.java line 126: > >> 124: // latin1, but not ASCII >> 125: String entryName = "smörgårdsbord"; >> 126: > > @cl4es Are we ok with non-ASCII in source files? I'd hate to escape this ;-) As long as the file is UTF-8 encoded then I think it should be fine. Thanks for fixing the spelling before I could remark on it! :D - PR: https://git.openjdk.org/jdk/pull/12290
Re: RFR: 8300259: Add test coverage for processing of pending block files in signed JARs [v2]
On Tue, 17 Jan 2023 18:54:13 GMT, Eirik Bjorsnos wrote: >> This PR adds test coverage for pending block files in signed JAR files >> >> A signed JAR has pending block files if the block file [RSA, DSA, EC] comes >> before the corresponding signature file [SF] in the JAR. >> >> JarVerifier.processEntry supports processing of such pending block files, >> but this code path does not seem to be exercised by current test. >> >> The new test PendingBlocksJar checks that signed JARs with pending blocks >> are processed correctly, both for the valid and invalid cases. > > Eirik Bjorsnos has updated the pull request incrementally with two additional > commits since the last revision: > > - Make it more clear in the @summary tag that it is the block file that is > pending, not the signature file > - Renamed test from PendingBlocksJar to more descriptive > SignedJarPendingBlock Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12009
Re: RFR: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp [v2]
On Mon, 6 Feb 2023 11:44:07 GMT, SUN Guoyun wrote: >> Hi all, >> When -Xcomp be used, this testcase will use more codecaches, causing the GC >> to be triggered early, then causing this test failed on LoongArch64 >> architecture. >> >> This PR fix the issue, Please help review it. >> >> Thanks. > > SUN Guoyun has updated the pull request incrementally with one additional > commit since the last revision: > > 8301737: > java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with > -Xcomp Please add @bug 8301737 It is not going to be obvious why a larger code cashe is needed (and only for aarch64). Can you split the test runs so the original does not run on aarch64 and a new second run runs only on aarch64. For example, /* * @test * @bug 8301737 * @summary Check that objects are exported with ObjectInputFilters via UnicastRemoteObject * @requires os.arch != "aarch64" * @run testng/othervm FilterUROTest */ /* * @test * @summary Check that objects are exported with ObjectInputFilters via UnicastRemoteObject * @requires os.arch == "aarch64" * @run testng/othervm -XX:ReservedCodeCacheSize=512m FilterUROTest */ - PR: https://git.openjdk.org/jdk/pull/12399
Integrated: JDK-8300098 : java/util/concurrent/ConcurrentHashMap/ConcurrentAssociateTest.java fails with internal timeout when executed with TieredCompilation1/3
On Tue, 31 Jan 2023 10:45:07 GMT, Viktor Klang wrote: > The proposed fix by @DougLea ensures that the state transition into waiting > is retried in the cases where a previous waiter isn't making progress and a > new waiter goes into waiting. This pull request has now been integrated. Changeset: ecf8842c Author:Viktor Klang Committer: Alan Bateman URL: https://git.openjdk.org/jdk/commit/ecf8842cd2309210f3d5eee7f9f28a198a860686 Stats: 12 lines in 1 file changed: 2 ins; 2 del; 8 mod 8300098: java/util/concurrent/ConcurrentHashMap/ConcurrentAssociateTest.java fails with internal timeout when executed with TieredCompilation1/3 Co-authored-by: Doug Lea Reviewed-by: jpai, alanb - PR: https://git.openjdk.org/jdk/pull/12319
Re: RFR: JDK-8301621: libzip should use pread instead of lseek+read
On Sun, 5 Feb 2023 19:04:24 GMT, Alan Bateman wrote: > Are you planning to include benchmark results to go with this change? > > It should be okay to change readFullyAt to use pread, and ReadFile with an OV > with the position. The latter has a side effect that it changes the file > pointer but it should be okay in the zip code. One thing that the changes > highlight is that this native file is begging to be refactoring into platform > specific code (this isn't the PR to do that). > > In passing, the JNI code use 4 space indent, not 2. > > The O_CLOEXEC part is probably okay but if you look at the > Runtime.exec/Process implementation you'll see that it isn't really because > that code closes all file descriptors on exec. I can add benchmarks later this week. I'll also fix the indentation. On `O_CLOEXEC`, closing after exec isn't really foolproof. Additionally that requires always using the Java method of subprocess spawning, which isn't always the case. `O_CLOEXEC` covers all use cases and is generally the correct thing to do unless a file descriptor is explicitly intended to be shared. - PR: https://git.openjdk.org/jdk/pull/12417
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v2]
On Mon, 6 Feb 2023 15:17:14 GMT, Eirik Bjorsnos wrote: >> After finding a hash match, getEntryPos needs to compare the lookup name up >> to the encoded entry name in the CEN. This comparison is done by decoding >> the entry name into a String. The names can then be compared using the >> String API. This decoding step adds a significat cost to this method. >> >> This PR suggest to update the string comparison such that in the common case >> where both the lookup name and the entry name are encoded in >> ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can >> instead be compared direcly. >> >> ZipCoder is updated with a new method to compare a string with an encoded >> byte array range. The default implementation decodes to string (like the >> current code), while the UTF-8 implementation uses >> JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses >> Arrays.mismatch for comparison with or without matching trailing slashes. >> >> Additionally, this PR suggest to make the following updates to getEntryPos: >> >> - The try/catch for IAE is redundant and can be safely removed. (initCEN >> already checks this and will throws IAE for invalid UTF-8). This seems to >> give a 3-4% speedup on micros) >> - A new test InvalidBytesInEntryNameOrComment is a added to verify that >> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I >> found no existing test coverage for this) >> - The recursion when looking for "name/" matches is replaced with iteration. >> We keep track of any "name/" match and return it at the end of the search. >> (I feel this is easier to follow and it also gives a ~30% speedup for >> addSlash lookups with no regression on regular lookups) >> >> (My though is that including these additional updates in this PR might >> reduce reviewer overhead given that it touches the exact same code. I might >> be wrong on this, please advise :) >> >> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified >> to use xalan.jar): >> >> Baseline: >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 >> ns/op >> >> >> >> PR: >> >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 >> ns/op >> >> >> To assess the impact on startup/warmup, I made a main method class which >> measures the total time of calling ZipFile.getEntry for all entries in the >> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement >> (time in micros): >> >> >> Percentile Baseline Patch >> 50 % 23155 21149 >> 75 % 23598 21454 >> 90 % 23989 21691 >> 95 % 24238 21973 >> 99 % 25270 22446 >> STDEV 792 549 >> Count 500 500 > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Spelling fix in test data for non-ascii latin1 string test/jdk/java/util/zip/ZipFile/TestZipFileEncodings.java line 126: > 124: // latin1, but not ASCII > 125: String entryName = "smörgårdsbord"; > 126: @cl4es Are we ok with non-ASCII in source files? I'd hate to escape this ;-) - PR: https://git.openjdk.or
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v2]
> After finding a hash match, getEntryPos needs to compare the lookup name up > to the encoded entry name in the CEN. This comparison is done by decoding the > entry name into a String. The names can then be compared using the String > API. This decoding step adds a significat cost to this method. > > This PR suggest to update the string comparison such that in the common case > where both the lookup name and the entry name are encoded in > ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can > instead be compared direcly. > > ZipCoder is updated with a new method to compare a string with an encoded > byte array range. The default implementation decodes to string (like the > current code), while the UTF-8 implementation uses > JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses > Arrays.mismatch for comparison with or without matching trailing slashes. > > Additionally, this PR suggest to make the following updates to getEntryPos: > > - The try/catch for IAE is redundant and can be safely removed. (initCEN > already checks this and will throws IAE for invalid UTF-8). This seems to > give a 3-4% speedup on micros) > - A new test InvalidBytesInEntryNameOrComment is a added to verify that > initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I > found no existing test coverage for this) > - The recursion when looking for "name/" matches is replaced with iteration. > We keep track of any "name/" match and return it at the end of the search. (I > feel this is easier to follow and it also gives a ~30% speedup for addSlash > lookups with no regression on regular lookups) > > (My though is that including these additional updates in this PR might reduce > reviewer overhead given that it touches the exact same code. I might be wrong > on this, please advise :) > > I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified > to use xalan.jar): > > Baseline: > > Benchmark (size) Mode CntScore Error > Units > ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 > ns/op > ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 > ns/op > ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 > ns/op > ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 > ns/op > ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 > ns/op > ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 > ns/op > ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 > ns/op > ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 > ns/op > ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 > ns/op > ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 > ns/op > > > > PR: > > > Benchmark (size) Mode CntScore Error > Units > ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 > ns/op > ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 > ns/op > ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 > ns/op > ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 > ns/op > ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 > ns/op > ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 > ns/op > ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 > ns/op > ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 > ns/op > ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 > ns/op > ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 > ns/op > > > To assess the impact on startup/warmup, I made a main method class which > measures the total time of calling ZipFile.getEntry for all entries in the > 109 JAR file dependenies of spring-petclinic. The shows a nice improvement > (time in micros): > > > Percentile Baseline Patch > 50 % 23155 21149 > 75 % 23598 21454 > 90 % 23989 21691 > 95 % 24238 21973 > 99 % 25270 22446 > STDEV 792 549 > Count 500 500 Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Spelling fix in test data for non-ascii latin1 string - Changes: - all: https://git.openjdk.org/jdk/pull/12290/files - new: https://git.openjdk.org/jdk/pull/12290/files/3ca98e21..d0c12325 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12290.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Mon, 6 Feb 2023 12:01:19 GMT, Eirik Bjorsnos wrote: >> Nice, I have updated the PR such that the new shared secret is replaced with >> using getBytesNoRepl instead. If there is a performance difference, it seems >> to hide in the noise. >> >> I had expected such a regression to be caught by existing tests, which seems >> not to be the case. I added TestZipFileEncodings.latin1NotAscii to adress >> this. > > getBytesNoRepl throws CharacterCodingException "for malformed input or > unmappable characters". > > This should never happen since initCEN should already reject it. If it should > happen anyway, I return NO_MATCH which will ignore the match just like the > catch in getEntryPos currently does. Yes, this should be fine. - PR: https://git.openjdk.org/jdk/pull/12290
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Mon, 30 Jan 2023 10:32:38 GMT, Eirik Bjorsnos wrote:
> After finding a hash match, getEntryPos needs to compare the lookup name up
> to the encoded entry name in the CEN. This comparison is done by decoding the
> entry name into a String. The names can then be compared using the String
> API. This decoding step adds a significat cost to this method.
>
> This PR suggest to update the string comparison such that in the common case
> where both the lookup name and the entry name are encoded in
> ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can
> instead be compared direcly.
>
> ZipCoder is updated with a new method to compare a string with an encoded
> byte array range. The default implementation decodes to string (like the
> current code), while the UTF-8 implementation uses
> JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses
> Arrays.mismatch for comparison with or without matching trailing slashes.
>
> Additionally, this PR suggest to make the following updates to getEntryPos:
>
> - The try/catch for IAE is redundant and can be safely removed. (initCEN
> already checks this and will throws IAE for invalid UTF-8). This seems to
> give a 3-4% speedup on micros)
> - A new test InvalidBytesInEntryNameOrComment is a added to verify that
> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I
> found no existing test coverage for this)
> - The recursion when looking for "name/" matches is replaced with iteration.
> We keep track of any "name/" match and return it at the end of the search. (I
> feel this is easier to follow and it also gives a ~30% speedup for addSlash
> lookups with no regression on regular lookups)
>
> (My though is that including these additional updates in this PR might reduce
> reviewer overhead given that it touches the exact same code. I might be wrong
> on this, please advise :)
>
> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified
> to use xalan.jar):
>
> Baseline:
>
> Benchmark (size) Mode CntScore Error
> Units
> ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004
> ns/op
> ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320
> ns/op
> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075
> ns/op
> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838
> ns/op
> ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455
> ns/op
> ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341
> ns/op
> ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141
> ns/op
>
>
>
> PR:
>
>
> Benchmark (size) Mode CntScore Error
> Units
> ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329
> ns/op
> ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428
> ns/op
> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502
> ns/op
> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191
> ns/op
> ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963
> ns/op
> ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717
> ns/op
> ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051
> ns/op
>
>
> To assess the impact on startup/warmup, I made a main method class which
> measures the total time of calling ZipFile.getEntry for all entries in the
> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement
> (time in micros):
>
>
> Percentile Baseline Patch
> 50 % 23155 21149
> 75 % 23598 21454
> 90 % 23989 21691
> 95 % 24238 21973
> 99 % 25270 22446
> STDEV 792 549
> Count 500 500
Filed JDK-8301873 for this, update PR title when you're ready.
src/java.base/share/classes/java/lang/System.java line 2668:
> 2666: @Override
> 2667: public int mismatchUTF8(String str, byte[] b, int
> fromIndex, int toIndex) {
> 2668: byte[] encoded = str.isLatin1() ? str.value() :
> str.getBytes(UTF_8.INSTANCE);
I think this is incorrect: latin-1 characters above codepoint 127 (non-ascii)
would be represented by 2 bytes in UTF-8. What you want here is probably
`str.isAscii() ? ...`. The ASCII check will have to lo
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Mon, 30 Jan 2023 14:20:58 GMT, Eirik Bjorsnos wrote: >> After finding a hash match, getEntryPos needs to compare the lookup name up >> to the encoded entry name in the CEN. This comparison is done by decoding >> the entry name into a String. The names can then be compared using the >> String API. This decoding step adds a significat cost to this method. >> >> This PR suggest to update the string comparison such that in the common case >> where both the lookup name and the entry name are encoded in >> ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can >> instead be compared direcly. >> >> ZipCoder is updated with a new method to compare a string with an encoded >> byte array range. The default implementation decodes to string (like the >> current code), while the UTF-8 implementation uses >> JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses >> Arrays.mismatch for comparison with or without matching trailing slashes. >> >> Additionally, this PR suggest to make the following updates to getEntryPos: >> >> - The try/catch for IAE is redundant and can be safely removed. (initCEN >> already checks this and will throws IAE for invalid UTF-8). This seems to >> give a 3-4% speedup on micros) >> - A new test InvalidBytesInEntryNameOrComment is a added to verify that >> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I >> found no existing test coverage for this) >> - The recursion when looking for "name/" matches is replaced with iteration. >> We keep track of any "name/" match and return it at the end of the search. >> (I feel this is easier to follow and it also gives a ~30% speedup for >> addSlash lookups with no regression on regular lookups) >> >> (My though is that including these additional updates in this PR might >> reduce reviewer overhead given that it touches the exact same code. I might >> be wrong on this, please advise :) >> >> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified >> to use xalan.jar): >> >> Baseline: >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 >> ns/op >> >> >> >> PR: >> >> >> Benchmark (size) Mode CntScore Error >> Units >> ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 >> ns/op >> ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 >> ns/op >> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 >> ns/op >> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 >> ns/op >> ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 >> ns/op >> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 >> ns/op >> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 >> ns/op >> ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 >> ns/op >> ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 >> ns/op >> >> >> To assess the impact on startup/warmup, I made a main method class which >> measures the total time of calling ZipFile.getEntry for all entries in the >> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement >> (time in micros): >> >> >> Percentile Baseline Patch >> 50 % 23155 21149 >> 75 % 23598 21454 >> 90 % 23989 21691 >> 95 % 24238 21973 >> 99 % 25270 22446 >> STDEV 792 549 >> Count 500 500 > > src/java.base/share/classes/java/lang/System.java line 2678: > >> 2676: } >> 2677: return Arrays.mismatch(encoded, 0, encoded.length, b, >> fromIndex, toIndex); >> 2678: } > > Leaving the ArraySupport.mismatch code here for now if anyone wants to > investigate the ~3% regression introduced by the range checks in > Arrays.mismatch The performance hit of using Arrays.mismatch instead of ArraysSupport might be
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Mon, 6 Feb 2023 14:27:36 GMT, Claes Redestad wrote: >> Interesting. Would be nice to solve this in the JIT! >> >> This disabled code got deleted in my last commit, but it seems like you have >> a good analysis so we can let it go now. > > Right. I might have fumbled this experiment a bit, and perhaps your setup > would inline and then eliminate some of the known-in-range checks already. > > Though we have intrinsified some of the `Preconditions.check*` methods in the > past to help improve range checks, but the `checkFromToIndex` method that > would be applicable here has not been intrinsified. It might be a reasonable > path forward to replace `Arrays.rangeCheck` with > `Preconditions.checkFromToIndex` and then look at intrinsifying that method > to help eliminating or optimizing some of the checks. Nevermind, I had a flaw in my experiment and seems the first range check in a call like `Arrays.mismatch(encoded, 0, encoded.length, b, off, off+len);` should be eliminated. So perhaps you're seeing the cost of the second range check, which might be unavoidable to be safe (zip meta data could otherwise be doctored to try and perform out of bounds reads via intrinsified code) - PR: https://git.openjdk.org/jdk/pull/12290
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Mon, 6 Feb 2023 11:56:22 GMT, Eirik Bjorsnos wrote:
>> src/java.base/share/classes/java/lang/System.java line 2671:
>>
>>> 2669: if (false) {
>>> 2670: // Arrays.mismatch without the range checks (~5%
>>> faster micro getEntryHit)
>>> 2671: int aLength = encoded.length;
>>
>> Part of the difference you're seeing is due to knowing that you'll be
>> matching the entire length of the first array (`encoded, 0, encoded.length`).
>>
>> As an experiment I added `Arrays.mismatch(byte[], byte[], int, int)` to
>> mismatch the entire range of the first array argument vs the range of the
>> second and can spot an improvement in affected micros:
>>
>> Benchmark (size) Mode Cnt Score
>> Error Units
>> ArraysMismatch.Char.differentSubrangeMatches 90 avgt 10 12.165 ±
>> 0.074 ns/op # mismatch(a, aFrom, aTo, b, bFrom, bTo)
>> ArraysMismatch.Char.subrangeMatches 90 avgt 10 10.748 ±
>> 0.006 ns/op # mismatch(a, b, bFrom, bTo)
>>
>> This might be something we can solve in the JITs without having to add new
>> methods to `java.util.Arrays` to deal as efficiently as possible with the
>> case when we're matching against the entirety of one of the arrays.
>
> Interesting. Would be nice to solve this in the JIT!
>
> This disabled code got deleted in my last commit, but it seems like you have
> a good analysis so we can let it go now.
Right. I might have fumbled this experiment a bit, and perhaps your setup would
inline and then eliminate some of the known-in-range checks already.
Though we have intrinsified some of the `Preconditions.check*` methods in the
past to help improve range checks, but the `checkFromToIndex` method that would
be applicable here has not been intrinsified. It might be a reasonable path
forward to replace `Arrays.rangeCheck` with `Preconditions.checkFromToIndex`
and then look at intrinsifying that method to help eliminating or optimizing
some of the checks.
-
PR: https://git.openjdk.org/jdk/pull/12290
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Sun, 5 Feb 2023 22:13:50 GMT, Claes Redestad wrote:
>> After finding a hash match, getEntryPos needs to compare the lookup name up
>> to the encoded entry name in the CEN. This comparison is done by decoding
>> the entry name into a String. The names can then be compared using the
>> String API. This decoding step adds a significat cost to this method.
>>
>> This PR suggest to update the string comparison such that in the common case
>> where both the lookup name and the entry name are encoded in
>> ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can
>> instead be compared direcly.
>>
>> ZipCoder is updated with a new method to compare a string with an encoded
>> byte array range. The default implementation decodes to string (like the
>> current code), while the UTF-8 implementation uses
>> JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses
>> Arrays.mismatch for comparison with or without matching trailing slashes.
>>
>> Additionally, this PR suggest to make the following updates to getEntryPos:
>>
>> - The try/catch for IAE is redundant and can be safely removed. (initCEN
>> already checks this and will throws IAE for invalid UTF-8). This seems to
>> give a 3-4% speedup on micros)
>> - A new test InvalidBytesInEntryNameOrComment is a added to verify that
>> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I
>> found no existing test coverage for this)
>> - The recursion when looking for "name/" matches is replaced with iteration.
>> We keep track of any "name/" match and return it at the end of the search.
>> (I feel this is easier to follow and it also gives a ~30% speedup for
>> addSlash lookups with no regression on regular lookups)
>>
>> (My though is that including these additional updates in this PR might
>> reduce reviewer overhead given that it touches the exact same code. I might
>> be wrong on this, please advise :)
>>
>> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified
>> to use xalan.jar):
>>
>> Baseline:
>>
>> Benchmark (size) Mode CntScore Error
>> Units
>> ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004
>> ns/op
>> ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320
>> ns/op
>> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386
>> ns/op
>> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075
>> ns/op
>> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838
>> ns/op
>> ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998
>> ns/op
>> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545
>> ns/op
>> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455
>> ns/op
>> ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341
>> ns/op
>> ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141
>> ns/op
>>
>>
>>
>> PR:
>>
>>
>> Benchmark (size) Mode CntScore Error
>> Units
>> ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329
>> ns/op
>> ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428
>> ns/op
>> ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154
>> ns/op
>> ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502
>> ns/op
>> ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191
>> ns/op
>> ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114
>> ns/op
>> ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505
>> ns/op
>> ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963
>> ns/op
>> ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717
>> ns/op
>> ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051
>> ns/op
>>
>>
>> To assess the impact on startup/warmup, I made a main method class which
>> measures the total time of calling ZipFile.getEntry for all entries in the
>> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement
>> (time in micros):
>>
>>
>> Percentile Baseline Patch
>> 50 % 23155 21149
>> 75 % 23598 21454
>> 90 % 23989 21691
>> 95 % 24238 21973
>> 99 % 25270 22446
>> STDEV 792 549
>> Count 500 500
>
> src/java.base/share/classes/java/lang/System.java line 2668:
>
>> 2666: @Override
>> 2667: public int mismatchUTF8(String str, byte[] b, int
>> fromIndex, int toIndex) {
>> 2668: byte[] encoded = str.isLatin1() ? str.value() :
>> str.getBytes(UTF_8.INSTANCE);
>
> I think this is incorrect: latin-1 characters above codepoint 127 (non-ascii)
> would be represented by 2 bytes in UTF-8. What you want here is probably
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Mon, 6 Feb 2023 11:47:42 GMT, Eirik Bjorsnos wrote:
>> src/java.base/share/classes/java/lang/System.java line 2668:
>>
>>> 2666: @Override
>>> 2667: public int mismatchUTF8(String str, byte[] b, int
>>> fromIndex, int toIndex) {
>>> 2668: byte[] encoded = str.isLatin1() ? str.value() :
>>> str.getBytes(UTF_8.INSTANCE);
>>
>> I think this is incorrect: latin-1 characters above codepoint 127
>> (non-ascii) would be represented by 2 bytes in UTF-8. What you want here is
>> probably `str.isAscii() ? ...`. The ASCII check will have to look at the
>> bytes, so will incur a minor penalty.
>>
>> Good news is that you should already be able to do this with what's already
>> exposed via `JLA.getBytesNoRepl(str, StandardCharsets.UTF_8)`, so no need
>> for more shared secrets.
>
> Nice, I have updated the PR such that the new shared secret is replaced with
> using getBytesNoRepl instead. If there is a performance difference, it seems
> to hide in the noise.
>
> I had expected such a regression to be caught by existing tests, which seems
> not to be the case. I added TestZipFileEncodings.latin1NotAscii to adress
> this.
getBytesNoRepl throws CharacterCodingException "for malformed input or
unmappable characters".
This should never happen since initCEN should already reject it. If it should
happen anyway, I return NO_MATCH which will ignore the match just like the
catch in getEntryPos currently does.
-
PR: https://git.openjdk.org/jdk/pull/12290
RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
After finding a hash match, getEntryPos needs to compare the lookup name up to the encoded entry name in the CEN. This comparison is done by decoding the entry name into a String. The names can then be compared using the String API. This decoding step adds a significat cost to this method. This PR suggest to update the string comparison such that in the common case where both the lookup name and the entry name are encoded in ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can instead be compared direcly. ZipCoder is updated with a new method to compare a string with an encoded byte array range. The default implementation decodes to string (like the current code), while the UTF-8 implementation uses JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses Arrays.mismatch for comparison with or without matching trailing slashes. Additionally, this PR suggest to make the following updates to getEntryPos: - The try/catch for IAE is redundant and can be safely removed. (initCEN already checks this and will throws IAE for invalid UTF-8). This seems to give a 3-4% speedup on micros) - A new test InvalidBytesInEntryNameOrComment is a added to verify that initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I found no existing test coverage for this) - The recursion when looking for "name/" matches is replaced with iteration. We keep track of any "name/" match and return it at the end of the search. (I feel this is easier to follow and it also gives a ~30% speedup for addSlash lookups with no regression on regular lookups) (My though is that including these additional updates in this PR might reduce reviewer overhead given that it touches the exact same code. I might be wrong on this, please advise :) I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified to use xalan.jar): Baseline: Benchmark (size) Mode CntScore Error Units ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 ns/op ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 ns/op ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 ns/op ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 ns/op ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 ns/op ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 ns/op ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 ns/op ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 ns/op ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 ns/op ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 ns/op PR: Benchmark (size) Mode CntScore Error Units ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 ns/op ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 ns/op ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 ns/op ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 ns/op ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 ns/op ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 ns/op ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 ns/op ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 ns/op ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 ns/op ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 ns/op To assess the impact on startup/warmup, I made a main method class which measures the total time of calling ZipFile.getEntry for all entries in the 109 JAR file dependenies of spring-petclinic. The shows a nice improvement (time in micros): Percentile Baseline Patch 50 % 23155 21149 75 % 23598 21454 90 % 23989 21691 95 % 24238 21973 99 % 25270 22446 STDEV 792 549 Count 500 500 - Commit messages: - Replace new shared secret with using getBytesNoRepl in UTF8ZipCoder.compare. Add a test case for UTF-8 encoded entry name which is latin1, but not ASCII - Rename test to InvalidBytesInEntryNameOrComment - Adjust whitespace - Some minor improvements to code comments - Micros seem to indicate that the range checks in Arrays.mismatch might have as much as 5% regression - Move String/byte[] comparison into ZipCoder. Change the default implementation to decode to string for comparison instead of encoding to bytes, this seems safer. Revert some changes from previous commits to parameters in the hasTrailingSlash method. - ByteBuffers for reading ZIP files must be little-endian - Produce template ZIP as a byte[] instead of writing it to disk - Improve summary for the test - Simplify comment for "name/" m
Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos
On Mon, 30 Jan 2023 10:32:38 GMT, Eirik Bjorsnos wrote: > After finding a hash match, getEntryPos needs to compare the lookup name up > to the encoded entry name in the CEN. This comparison is done by decoding the > entry name into a String. The names can then be compared using the String > API. This decoding step adds a significat cost to this method. > > This PR suggest to update the string comparison such that in the common case > where both the lookup name and the entry name are encoded in > ASCII-compatible UTF-8, decoding can be avoided and the byte arrays can > instead be compared direcly. > > ZipCoder is updated with a new method to compare a string with an encoded > byte array range. The default implementation decodes to string (like the > current code), while the UTF-8 implementation uses > JavaLangAccess.getBytesNoRepl to get the bytes. Both methods thes uses > Arrays.mismatch for comparison with or without matching trailing slashes. > > Additionally, this PR suggest to make the following updates to getEntryPos: > > - The try/catch for IAE is redundant and can be safely removed. (initCEN > already checks this and will throws IAE for invalid UTF-8). This seems to > give a 3-4% speedup on micros) > - A new test InvalidBytesInEntryNameOrComment is a added to verify that > initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I > found no existing test coverage for this) > - The recursion when looking for "name/" matches is replaced with iteration. > We keep track of any "name/" match and return it at the end of the search. (I > feel this is easier to follow and it also gives a ~30% speedup for addSlash > lookups with no regression on regular lookups) > > (My though is that including these additional updates in this PR might reduce > reviewer overhead given that it touches the exact same code. I might be wrong > on this, please advise :) > > I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified > to use xalan.jar): > > Baseline: > > Benchmark (size) Mode CntScore Error > Units > ZipFileGetEntry.getEntryHit 512 avgt 15 74.941 ± 1.004 > ns/op > ZipFileGetEntry.getEntryHit 1024 avgt 15 84.943 ± 1.320 > ns/op > ZipFileGetEntry.getEntryHitUncached 512 avgt 15 120.371 ± 2.386 > ns/op > ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 126.128 ± 1.075 > ns/op > ZipFileGetEntry.getEntryMiss 512 avgt 15 23.818 ± 0.838 > ns/op > ZipFileGetEntry.getEntryMiss1024 avgt 15 29.762 ± 5.998 > ns/op > ZipFileGetEntry.getEntryMissUncached 512 avgt 15 59.405 ± 0.545 > ns/op > ZipFileGetEntry.getEntryMissUncached1024 avgt 15 71.840 ± 2.455 > ns/op > ZipFileGetEntry.getEntrySlash512 avgt 15 135.621 ± 4.341 > ns/op > ZipFileGetEntry.getEntrySlash 1024 avgt 15 134.190 ± 2.141 > ns/op > > > > PR: > > > Benchmark (size) Mode CntScore Error > Units > ZipFileGetEntry.getEntryHit 512 avgt 15 62.267 ± 1.329 > ns/op > ZipFileGetEntry.getEntryHit 1024 avgt 15 72.916 ± 2.428 > ns/op > ZipFileGetEntry.getEntryHitUncached 512 avgt 15 101.630 ± 1.154 > ns/op > ZipFileGetEntry.getEntryHitUncached 1024 avgt 15 113.161 ± 0.502 > ns/op > ZipFileGetEntry.getEntryMiss 512 avgt 15 23.003 ± 1.191 > ns/op > ZipFileGetEntry.getEntryMiss1024 avgt 15 23.236 ± 1.114 > ns/op > ZipFileGetEntry.getEntryMissUncached 512 avgt 15 56.781 ± 1.505 > ns/op > ZipFileGetEntry.getEntryMissUncached1024 avgt 15 67.767 ± 1.963 > ns/op > ZipFileGetEntry.getEntrySlash512 avgt 15 73.745 ± 2.717 > ns/op > ZipFileGetEntry.getEntrySlash 1024 avgt 15 75.784 ± 1.051 > ns/op > > > To assess the impact on startup/warmup, I made a main method class which > measures the total time of calling ZipFile.getEntry for all entries in the > 109 JAR file dependenies of spring-petclinic. The shows a nice improvement > (time in micros): > > > Percentile Baseline Patch > 50 % 23155 21149 > 75 % 23598 21454 > 90 % 23989 21691 > 95 % 24238 21973 > 99 % 25270 22446 > STDEV 792 549 > Count 500 500 As suggested by @cl4es, I've replaced the use of ArraySupport.mismatch with Arrays.mismatch. The added range checks seems to give a regression of ~3% on the getEntryHit micro. I realized that encoding to bytes and then comparing to CEN bytes might not be safe for encodings were multiple representations is possible for the same code points. So I moved string/byte array comparison into ZipCoder, which can now decode from CEN and compare as in the current code. Micros indicate this has no performance impact. src/java.base/share/classes/java/lang/System.java
Re: JEP415: FilterInThread Example
Thanks, I'd planned to file a bug too.
If you think of any improvements, let me know.
On 2/6/23 8:26 AM, Dr Heinz M. Kabutz wrote:
FWIW, I've also submitted this as a bug report:
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8301863
Regards
Heinz
--
Dr Heinz M. Kabutz (PhD CompSci)
Author of "The Java™ Specialists' Newsletter" -www.javaspecialists.eu
Java Champion -www.javachampions.org
JavaOne Rock Star Speaker
Tel: +30 69 75 595 262
Skype: kabutz
On 2023/02/06 06:55, Roger Riggs wrote:
Hi Heinz,
Indeed, this example is not intuitive and does not do what was
intended and could use a better explanation.
The interaction of three filters gets complicated and their
combination depends on the ordering and intention of each filter and
the particular filter factory goal. The FilterInThread example
provides only one of many possible functions.
The FilterInThread example uses the JVM-wide filter, thread filter,
and stream filter for different purposes.
The JVM-wide filter has a particular role in that it can be set on
the command line via a system property.
It is typically used as a backstop after the application is deployed
to patch in additional rejection of classes.
The property value syntax allows for either allowing or rejecting
classes, and an otherwise unmentioned class is left UNDECIDED,
possibly with some risk exposure.
The thread filter is used to more focus de-serialization on a group
of classes, either to narrow it or expand it.
The FilterInThread example takes the position that any UNDECIDED in
the thread's filter and the JVM-wide filter should be rejected even
if the pattern does not explicitly do so. This keeps an oversight in
filter construction from becoming a risk.
The stream filter is included mostly for backward compatibility,
introduced in JDK 9 via JEP 290. The stream filter is set by the code
creating the ObjectInputStream and part of its design and purpose. In
the FilterInThread example, if it returns UNDECIDED, the result is
determined by a merge of the other two filters and further rejecting
UNDECIDED.
The bug in the example, that individually rejects UNDECIDED in the
JVM-wide and thread filters respectively, should instead reject only
if both return UNDECIDED.
The revised example is:
*// Returns a composite filter of the static JVM-wide filter, a
thread-specific filter, *// and the stream-specific filter. *public
ObjectInputFilter apply(ObjectInputFilter curr, ObjectInputFilter
next) { *if (curr == null) { *// Called from the OIS constructor or
perhaps OIS.setObjectInputFilter with no current filter *var filter =
filterThreadLocal.get(); *if (filter != null) { *// Merge to invoke
the thread local filter and then the JVM-wide filter (if any) *filter
= ObjectInputFilter.merge(filter, next); *return
ObjectInputFilter.rejectUndecidedClass(filter); *} *return (next ==
null) ? null : ObjectInputFilter.rejectUndecidedClass(next); *} else
{ *// Called from OIS.setObjectInputFilter with a current filter and
a stream-specific filter. *// The curr filter already incorporates
the thread filter and static JVM-wide filter *// and rejection of
undecided classes *// If there is a stream-specific filter merge to
invoke it and then the current filter. *if (next != null) { *return
ObjectInputFilter.merge(next, curr); *} *return curr; *} *}
The filters are evaluated as:
merge(restrictLargeArrays,rejectUndecidedClass(merge(allowInteger,allowArrayList)))
The first call to the factory returns a filter: `var f1 =
rejectUndecidedClass(merge(allowInteger,allowArrayList))`
The second call to the factory returns filter: `var f2 =
merge(restrictLargeArrays, f1)`
The filters are evaluated in order, until an accept or reject is
returned:
1) restrictLargeArrays (stream)
2) allowInteger (thread filter)
3) allowArrayList (JVM-wide filter)
This has the same value as your ideal but without an extra
RejectUndecidedClass.
Note that in this composition, the choice by a filter to accept or
reject can not be overridden by a subsequent filter.
Thank you for the comments and suggestions, Roger
On 2/3/23 1:20 PM, Dr Heinz M. Kabutz wrote:
I was trying to get my head around the FilterInThread example in JEP
415 (https://openjdk.org/jeps/415) and the JavaDoc for the
ObjectInputFilter
(https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/ObjectInputFilter.html)
For example, let's assume we have three filters. The first allow
ArrayList, the second allows Integer, the third restricts arrays to
not be larger than 1000.
ObjectInputFilter allowArrayList = ObjectInputFilter.allowFilter(
Set.of(ArrayList.class, Object.class)::contains, UNDECIDED
);
ObjectInputFilter allowInteger = ObjectInputFilter.allowFilter(
Set.of(Number.class, Integer.class)::contains, UNDECIDED
);
ObjectInputFilter restrictLargeArrays =
ObjectInputFilter.Config.createFilter("maxarray
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:37:43 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/ClassSignature.java line > 34: > >> 32: * Models the generic signature of a class file, as defined by JVMS >> 4.7.9. >> 33: */ >> 34: public sealed interface ClassSignature > > It is a bit odd to have Signature and XYZSignature in the API with the latter > not extending the former. I think I get what the API is aiming for though - > e.g. Signature is for modelling low-level "portions" of the signature > attributes, whereas ClassSignature, MethodSignature and friends are there to > model the signature attribute attached to a class, method, etc. The confusion come from simplified name. Signature probably should be called JavaTypeSignature according to the spec. ClassSignature and MethodSignature could not extend it, as it would not respect the reality. Each of them are completely different species. Signature (or JavaTypeSignature) on the other side has many sub-types. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:11:41 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/ClassModel.java line 104:
>
>> 102: * found
>> 103: */
>> 104: default List verify(Consumer debugOutput) {
>
> not super sure whether `verify` belongs here - could be another component in
> the `components` package?
Classfile API by default does not verify produced or transformed bytecode, so
this is more a question if we want to have verification an integral part of the
API or a standalone tool.
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 613:
>
>> 611: }
>> 612:
>> 613: default CodeBuilder labelBinding(Label label) {
>
> Maybe just `bind` or `bindLabel` ?
It has been discussed (and changed) many times. Please raise this discussion at
classfile-api-dev at openjdk.org
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 1371:
>
>> 1369: }
>> 1370:
>> 1371: default CodeBuilder tableswitch(Label defaultTarget,
>> List cases) {
>
> `switch` seems the one instruction for which an high-level variant (like
> `trying`) could be useful, as generating code for that can be quite painful.
Nice RFE, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:59:53 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/BufWriter.java line 40: > >> 38: * to the end of the buffer, as well as to create constant pool entries. >> 39: */ >> 40: public sealed interface BufWriter > > What is the relationship between this and ClassReader? Is one the dual of the > other - e.g. is the intended use for BufWriter to write custom attributes, > whereas ClassReader reads custom attributes? Yes, it is an exposure of low-level API to support custom attributes. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:58:04 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/BootstrapMethodEntry.java > line 41: > >> 39: * part of the constant pool. >> 40: */ >> 41: public sealed interface BootstrapMethodEntry > > Usages of this seem all to fall into the `constantpool` package - does this > interface belong there? `BootstrapMethodEntry` is not a constant pool entry, but `BootstrapMethodsAttribute` entry. It might be rather moved under `attribute` package and renamed to `BootstrapMethodInfo` to follow the same pattern as other attributes/infos. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:56:45 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/Attributes.java line 774:
>
>> 772: */
>> 773: public static AttributeMapper standardAttribute(Utf8Entry name) {
>> 774: int hash = name.hashCode();
>
> If we have a map, why do we need this "inlined" map here? Performance reasons?
Yes, performance is the main reason.
I'll note to do a fresh differential performance benchmarks with a HashMap.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:52:49 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/AttributedElement.java
> line 94:
>
>> 92: * are permitted.
>> 93: */
>> 94: enum Kind {
>
> Not sure how to interpret this. This seems to refer to an attribute - e.g.
> where is an attribute allowed - rather than to the element (e.g. the
> declaration to which the attribute is attached). All the constants are
> unused, so hard to make sense of how this is used.
There are at least 72 usages of AttributedElement.Kind across the Classfile API.
Do you suggest to rename it to something else (for example Location)?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 18:07:27 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 346: >> >>> 344: public static final int MAGIC_NUMBER = 0xCAFEBABE; >>> 345: >>> 346: public static final int NOP = 0; >> >> Not sure how I feel about the constants being here. It seems to me that they >> can be moved to more appropriate places - e.g. Instructor, TypeAnnotation >> (for the TAT ones), ConstantPool (for the TAG ones). >> >> The classfile versions, OTOH, do seem to belong here. > > Actually, we also have a ClassfileVersion class, so that could be a better > place for version numbers? There were several iterations of "where to store numeric constants". It is hard to find them when spread across many classes and duplicities appear instantly. Dedicated "Constants" would be another bikeshed with conflicting name. Co-location in Classfile was the latest decission as it is not just a central bikeshed, but it also reflect connection with classfile specification. Please raise that discussion at classfile-api-dev at openjdk.org if necessary. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:43:22 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 58:
>
>> 56: * Main entry points for parsing, transforming, and generating
>> classfiles.
>> 57: */
>> 58: public class Classfile {
>
> class or interface? (bikeshed, I realize)
yes, a bikeshed
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 197:
>
>> 195: * @return the classfile bytes
>> 196: */
>> 197: public static byte[] build(ClassDesc thisClass,
>
> The "build" methods here seem regular - e.g. build { class, module } x { byte
> array, path }. As a future improvement, perhaps capturing the "sink" of a
> build process might be helpful - so that you can avoid overloads between
> array and path variants. (e.g. `build( toByteArray(arr))`, or
> `build(...).toByteArray(...)`.
Classfile::build always return byte array and there is no "sink" model inside.
Classfile::buildTo is frequently used extension; a wrapper calling build and
writing the byte array using Files::write.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:37:55 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/package-info.java line 39:
>
>> 37: *
>> 38: * {@snippet lang=java :
>> 39: * ClassModel cm = ClassModel.of(bytes);
>
> There are several references to `ClassModel::of` (here and elsewhere), but
> this seems to have been moved to `ClassFile::parse`
will fix, thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v13]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - javadoc fixes - obsolete identifiers and unused imports cleanup - TypeAnnotation.TypePathComponent cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/8df1dc21..1aabe0e3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=11-12 Stats: 32 lines in 9 files changed: 8 ins; 5 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: JEP415: FilterInThread Example
FWIW, I've also submitted this as a bug report:
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8301863
Regards
Heinz
--
Dr Heinz M. Kabutz (PhD CompSci)
Author of "The Java™ Specialists' Newsletter" -www.javaspecialists.eu
Java Champion -www.javachampions.org
JavaOne Rock Star Speaker
Tel: +30 69 75 595 262
Skype: kabutz
On 2023/02/06 06:55, Roger Riggs wrote:
Hi Heinz,
Indeed, this example is not intuitive and does not do what was
intended and could use a better explanation.
The interaction of three filters gets complicated and their
combination depends on the ordering and intention of each filter and
the particular filter factory goal. The FilterInThread example
provides only one of many possible functions.
The FilterInThread example uses the JVM-wide filter, thread filter,
and stream filter for different purposes.
The JVM-wide filter has a particular role in that it can be set on the
command line via a system property.
It is typically used as a backstop after the application is deployed
to patch in additional rejection of classes.
The property value syntax allows for either allowing or rejecting
classes, and an otherwise unmentioned class is left UNDECIDED,
possibly with some risk exposure.
The thread filter is used to more focus de-serialization on a group of
classes, either to narrow it or expand it.
The FilterInThread example takes the position that any UNDECIDED in
the thread's filter and the JVM-wide filter should be rejected even if
the pattern does not explicitly do so. This keeps an oversight in
filter construction from becoming a risk.
The stream filter is included mostly for backward compatibility,
introduced in JDK 9 via JEP 290. The stream filter is set by the code
creating the ObjectInputStream and part of its design and purpose. In
the FilterInThread example, if it returns UNDECIDED, the result is
determined by a merge of the other two filters and further rejecting
UNDECIDED.
The bug in the example, that individually rejects UNDECIDED in the
JVM-wide and thread filters respectively, should instead reject only
if both return UNDECIDED.
The revised example is:
*// Returns a composite filter of the static JVM-wide filter, a
thread-specific filter, *// and the stream-specific filter. *public
ObjectInputFilter apply(ObjectInputFilter curr, ObjectInputFilter
next) { *if (curr == null) { *// Called from the OIS constructor or
perhaps OIS.setObjectInputFilter with no current filter *var filter =
filterThreadLocal.get(); *if (filter != null) { *// Merge to invoke
the thread local filter and then the JVM-wide filter (if any) *filter
= ObjectInputFilter.merge(filter, next); *return
ObjectInputFilter.rejectUndecidedClass(filter); *} *return (next ==
null) ? null : ObjectInputFilter.rejectUndecidedClass(next); *} else {
*// Called from OIS.setObjectInputFilter with a current filter and a
stream-specific filter. *// The curr filter already incorporates the
thread filter and static JVM-wide filter *// and rejection of
undecided classes *// If there is a stream-specific filter merge to
invoke it and then the current filter. *if (next != null) { *return
ObjectInputFilter.merge(next, curr); *} *return curr; *} *}
The filters are evaluated as:
merge(restrictLargeArrays,rejectUndecidedClass(merge(allowInteger,allowArrayList)))
The first call to the factory returns a filter: `var f1 =
rejectUndecidedClass(merge(allowInteger,allowArrayList))`
The second call to the factory returns filter: `var f2 =
merge(restrictLargeArrays, f1)`
The filters are evaluated in order, until an accept or reject is returned:
1) restrictLargeArrays (stream)
2) allowInteger (thread filter)
3) allowArrayList (JVM-wide filter)
This has the same value as your ideal but without an extra
RejectUndecidedClass.
Note that in this composition, the choice by a filter to accept or
reject can not be overridden by a subsequent filter.
Thank you for the comments and suggestions, Roger
On 2/3/23 1:20 PM, Dr Heinz M. Kabutz wrote:
I was trying to get my head around the FilterInThread example in JEP
415 (https://openjdk.org/jeps/415) and the JavaDoc for the
ObjectInputFilter
(https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/ObjectInputFilter.html)
For example, let's assume we have three filters. The first allow
ArrayList, the second allows Integer, the third restricts arrays to
not be larger than 1000.
ObjectInputFilter allowArrayList = ObjectInputFilter.allowFilter(
Set.of(ArrayList.class, Object.class)::contains, UNDECIDED
);
ObjectInputFilter allowInteger = ObjectInputFilter.allowFilter(
Set.of(Number.class, Integer.class)::contains, UNDECIDED
);
ObjectInputFilter restrictLargeArrays =
ObjectInputFilter.Config.createFilter("maxarray=1000");
Let's say that we create a FilterInThread instance and install that
as our factory. Furthermore, we set the allowArrayList as
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:32:37 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/attribute/SignatureAttribute.java > line 66: > >> 64: >> 65: /** >> 66: * Parse the siganture as a method signature. > > typo here `siganture` - check the entire class will fix, thanks - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:20:19 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line > 617: > >> 615: } >> 616: >> 617: public static final TypePathComponent ARRAY = new >> UnboundAttribute.TypePathComponentImpl(Kind.ARRAY, 0); > > `public static final` is redundant here (it's an interface) - please check > these (I've seen others). E.g. all `public` modifier for types nested in > interfaces are redundant as well. yes, will fix and search for other such cases, thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:23:51 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
> 641:
>
>> 639: int typeArgumentIndex();
>> 640:
>> 641: static TypePathComponent of(int typePathKind, int
>> typeArgumentIndex) {
>
> Should this take a `Kind` instead of an `int`?
Yes, will fix, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8301621: libzip should use pread instead of lseek+read
On Fri, 3 Feb 2023 19:49:44 GMT, Justin King wrote: > Avoid using `lseek` + `read` in favor of `pread`. For Windows, we can do the > same thing by using `OVERLAPPED`, as we are in synchronous mode we can use > `Offset` and `OffsetHigh` to achieve the same thing. > > Additionally I updated open to use `O_CLOEXEC` when available, as that really > should be used. src/java.base/share/native/libzip/zip_util.c line 227: > 225: number_of_bytes_to_read = (DWORD) (nbytes - total); > 226: } > 227: number_of_bytes_read = 0; do we really need to set number_of_bytes_read = 0 in every iteration ? . As per MSDN it looks like ReadFile will do it implicitly. [out, optional] lpNumberOfBytesRead A pointer to the variable that receives the number of bytes read when using a synchronous hFile parameter. ReadFile sets this value to zero before doing any work or error checking. Use NULL for this parameter if this is an asynchronous operation to avoid potentially erroneous results. - PR: https://git.openjdk.org/jdk/pull/12417
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:22:32 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Classfile API moved under jdk.internal.classfile package
>
> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
> 75:
>
>> 73: * The kind of target on which the annotation appears.
>> 74: */
>> 75: public enum TargetType {
>
> My IDE says this enum is not being used. I tend to believe it, since the
> TargetInfo sealed interface also seems to model the same thing?
There is only one TargetInfo for all TargetTypes, so instead of 22
sub-interfaces of TargetInfo, the instance of TargetType enum is hold in
TargetInfo.
> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
> 577:
>
>> 575:
>> 576: /**
>> 577: * type_path.path.
>
> The javadoc in this class seems off
will fix, thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp
On Fri, 3 Feb 2023 15:07:30 GMT, Roger Riggs wrote: > What is the connection between -Xcomp and the fix: > `-Dsun.net.httpserver.idleInterval=5`? The test does not use httpserver. > Perhaps jtreg /agent does, but an explanation of the interaction would be > appreciated. Please review again, thank you. - PR: https://git.openjdk.org/jdk/pull/12399
Re: RFR: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp [v2]
On Mon, 6 Feb 2023 11:44:07 GMT, SUN Guoyun wrote: >> Hi all, >> When -Xcomp be used, this testcase will use more codecaches, causing the GC >> to be triggered early, then causing this test failed on LoongArch64 >> architecture. >> >> This PR fix the issue, Please help review it. >> >> Thanks. > > SUN Guoyun has updated the pull request incrementally with one additional > commit since the last revision: > > 8301737: > java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with > -Xcomp I found the non-permanent target will be collection by GC early with `-Xcomp`. using `-Xlog:gc` can found the reason is: `GC(0) Pause Young (Concurrent Start) (CodeCache GC Threshold) 12M->2M(258M) 16.437ms` so I add `-XX:ReservedCodeCacheSize=512m` to make sure the test PASSED. Please review again, thank you. - PR: https://git.openjdk.org/jdk/pull/12399
Re: RFR: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp [v2]
On Mon, 6 Feb 2023 02:46:46 GMT, SUN Guoyun wrote: >> test/jdk/java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java >> line 41: >> >>> 39: /* >>> 40: * @test >>> 41: * @run testng/othervm -Dsun.net.httpserver.idleInterval=5 >>> FilterUROTest >> >> This test doesn't seem to be using an HttpServer so setting this system >> property seems useless as it should have no effect. > > Sorry, I was mistaken. I'll fix it later Done. please review again, thank you. - PR: https://git.openjdk.org/jdk/pull/12399
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java
On Sun, 5 Feb 2023 03:05:55 GMT, Joe Darcy wrote:
> Initial pass of porting FDLIBM sinh/cosh/tanh to Java. I do intend to
> refactor the regression tests a bit to reduce duplication, but the actual
> ports should be ready for review.
>
> Diff'ing the ports as before, original vs transliteration port:
>
>
> $ diff -w Hyperbolic.c Hyperbolic.translit.java
> 1c1
> < /* __ieee754_sinh(x)
> ---
>> /**
> 17a18,19
>> static class Sinh {
>> private static final double one = 1.0, shuge = 1.0e307;
> 19,33c21
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, shuge = 1.0e307;
> < #else
> < static double one = 1.0, shuge = 1.0e307;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_sinh(double x)
> < #else
> < double __ieee754_sinh(x)
> < double x;
> < #endif
> < {
> ---
>> private static double compute(double x) {
> 36c24
> < unsigned lx;
> ---
>> /* unsigned */ int lx;
> 51c39
> < t = expm1(fabs(x));
> ---
>> t = FdlibmTranslit.expm1(Math.abs(x));
> 57c45
> < if (ix < 0x40862E42) return h*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return h*StrictMath.exp(Math.abs(x)); //
>> TODO switch to translit
> 60,62c48,52
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> < if (ix<0x408633CE ||
> ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(0.5*fabs(x));
> ---
>> // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
>> // lx = (((*(unsigned*)&one)>>29)) + (unsigned*)&x ;
>> lx = __LO(x);
>> if (ix<0x408633CE ||
>> ((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 ))) {
>> w = StrictMath.exp(0.5*Math.abs(x)); // TODO switch to
>> translit
> 70c60,62
> < /* __ieee754_cosh(x)
> ---
>> }
>>
>> /**
> 90,105c82,84
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, half=0.5, huge = 1.0e300;
> < #else
> < static double one = 1.0, half=0.5, huge = 1.0e300;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_cosh(double x)
> < #else
> < double __ieee754_cosh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Cosh {
>> private static final double one = 1.0, half=0.5, huge = 1.0e300;
>> private static double compute(double x) {
> 108c87
> < unsigned lx;
> ---
>> /*unsigned*/ int lx;
> 119c98
> < t = expm1(fabs(x));
> ---
>> t = expm1(Math.abs(x));
> 127c106
> < t = __ieee754_exp(fabs(x));
> ---
>> t = StrictMath.exp(Math.abs(x)); // TODO switch to translit
> 132c111
> < if (ix < 0x40862E42) return half*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return half*StrictMath.exp(Math.abs(x));
>> // TODO switch to translit
> 135c114
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> ---
>> lx = __LO(x);
> 137,138c116,117
> < ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(half*fabs(x));
> ---
>> ((ix==0x408633ce)&&(Integer.compareUnsigned(lx, 0x8fb9f87d)
>> <= 0))) {
>> w = StrictMath.exp(half*Math.abs(x)); // TODO switch to
>> translit
> 146c125,127
> < /* Tanh(x)
> ---
>> }
>>
>> /**
> 169,184c150,152
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one=1.0, two=2.0, tiny = 1.0e-300;
> < #else
> < static double one=1.0, two=2.0, tiny = 1.0e-300;
> < #endif
> <
> < #ifdef __STDC__
> < double tanh(double x)
> < #else
> < double tanh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Tanh {
>> private static final double one=1.0, two=2.0, tiny = 1.0e-300;
>> static double compute(double x) {
> 203c171
> < t = expm1(two*fabs(x));
> ---
>> t = expm1(two*Math.abs(x));
> 206c174
> < t = expm1(-two*fabs(x));
> ---
>> t = expm1(-two*Math.abs(x));
> 214a183
>> }
>
>
> Note that the original has a in-line version of the "__LO" macro rather than
> using the macro.
>
>
> And transliteration vs more idiomatic:
>
>
> $ diff -w Hyperbolic.translit.java Hyperbolic.fdlibm.java
> 21c21
> < private static double compute(double x) {
> ---
>> static double compute(double x) {
> 26c26
> < /* High word of |x|. */
> ---
>> // High word of |x|
> 28c28
> < ix = jx&0x7fff;
> ---
>> ix = jx & 0x7fff_;
> 30,31c30,33
> < /* x is INF or NaN */
> < if(ix>=0x7ff0) return x+x;
> ---
>> // x is INF or NaN
>> if ( ix >= 0x7ff0_) {
>> return x + x;
>> }
> 34,40c36,48
> < if (jx<0) h = -h;
> < /* |x| i
Re: RFR: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp [v2]
> Hi all, > When -Xcomp be used, java thread to block for longer, then causing this test > failed frequently on the AArch64 and LoongArch64 architecture. > > This PR fix the issue, Please help review it. > > Thanks. SUN Guoyun has updated the pull request incrementally with one additional commit since the last revision: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp - Changes: - all: https://git.openjdk.org/jdk/pull/12399/files - new: https://git.openjdk.org/jdk/pull/12399/files/517b9fc4..650c8163 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12399&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12399&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12399.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12399/head:pull/12399 PR: https://git.openjdk.org/jdk/pull/12399
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java
On Sun, 5 Feb 2023 03:05:55 GMT, Joe Darcy wrote:
> Initial pass of porting FDLIBM sinh/cosh/tanh to Java. I do intend to
> refactor the regression tests a bit to reduce duplication, but the actual
> ports should be ready for review.
>
> Diff'ing the ports as before, original vs transliteration port:
>
>
> $ diff -w Hyperbolic.c Hyperbolic.translit.java
> 1c1
> < /* __ieee754_sinh(x)
> ---
>> /**
> 17a18,19
>> static class Sinh {
>> private static final double one = 1.0, shuge = 1.0e307;
> 19,33c21
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, shuge = 1.0e307;
> < #else
> < static double one = 1.0, shuge = 1.0e307;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_sinh(double x)
> < #else
> < double __ieee754_sinh(x)
> < double x;
> < #endif
> < {
> ---
>> private static double compute(double x) {
> 36c24
> < unsigned lx;
> ---
>> /* unsigned */ int lx;
> 51c39
> < t = expm1(fabs(x));
> ---
>> t = FdlibmTranslit.expm1(Math.abs(x));
> 57c45
> < if (ix < 0x40862E42) return h*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return h*StrictMath.exp(Math.abs(x)); //
>> TODO switch to translit
> 60,62c48,52
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> < if (ix<0x408633CE ||
> ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(0.5*fabs(x));
> ---
>> // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
>> // lx = (((*(unsigned*)&one)>>29)) + (unsigned*)&x ;
>> lx = __LO(x);
>> if (ix<0x408633CE ||
>> ((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 ))) {
>> w = StrictMath.exp(0.5*Math.abs(x)); // TODO switch to
>> translit
> 70c60,62
> < /* __ieee754_cosh(x)
> ---
>> }
>>
>> /**
> 90,105c82,84
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, half=0.5, huge = 1.0e300;
> < #else
> < static double one = 1.0, half=0.5, huge = 1.0e300;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_cosh(double x)
> < #else
> < double __ieee754_cosh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Cosh {
>> private static final double one = 1.0, half=0.5, huge = 1.0e300;
>> private static double compute(double x) {
> 108c87
> < unsigned lx;
> ---
>> /*unsigned*/ int lx;
> 119c98
> < t = expm1(fabs(x));
> ---
>> t = expm1(Math.abs(x));
> 127c106
> < t = __ieee754_exp(fabs(x));
> ---
>> t = StrictMath.exp(Math.abs(x)); // TODO switch to translit
> 132c111
> < if (ix < 0x40862E42) return half*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return half*StrictMath.exp(Math.abs(x));
>> // TODO switch to translit
> 135c114
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> ---
>> lx = __LO(x);
> 137,138c116,117
> < ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(half*fabs(x));
> ---
>> ((ix==0x408633ce)&&(Integer.compareUnsigned(lx, 0x8fb9f87d)
>> <= 0))) {
>> w = StrictMath.exp(half*Math.abs(x)); // TODO switch to
>> translit
> 146c125,127
> < /* Tanh(x)
> ---
>> }
>>
>> /**
> 169,184c150,152
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one=1.0, two=2.0, tiny = 1.0e-300;
> < #else
> < static double one=1.0, two=2.0, tiny = 1.0e-300;
> < #endif
> <
> < #ifdef __STDC__
> < double tanh(double x)
> < #else
> < double tanh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Tanh {
>> private static final double one=1.0, two=2.0, tiny = 1.0e-300;
>> static double compute(double x) {
> 203c171
> < t = expm1(two*fabs(x));
> ---
>> t = expm1(two*Math.abs(x));
> 206c174
> < t = expm1(-two*fabs(x));
> ---
>> t = expm1(-two*Math.abs(x));
> 214a183
>> }
>
>
> Note that the original has a in-line version of the "__LO" macro rather than
> using the macro.
>
>
> And transliteration vs more idiomatic:
>
>
> $ diff -w Hyperbolic.translit.java Hyperbolic.fdlibm.java
> 21c21
> < private static double compute(double x) {
> ---
>> static double compute(double x) {
> 26c26
> < /* High word of |x|. */
> ---
>> // High word of |x|
> 28c28
> < ix = jx&0x7fff;
> ---
>> ix = jx & 0x7fff_;
> 30,31c30,33
> < /* x is INF or NaN */
> < if(ix>=0x7ff0) return x+x;
> ---
>> // x is INF or NaN
>> if ( ix >= 0x7ff0_) {
>> return x + x;
>> }
> 34,40c36,48
> < if (jx<0) h = -h;
> < /* |x| i
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v4]
On Mon, 6 Feb 2023 10:01:49 GMT, Quan Anh Mai wrote: > Suggestion: Refactor the uncommon cases into separate functions so the > compiler can have easier time inlining the common path. Thanks @merykitty this probably needs confirmation from VM folks. I'm pretty sure that C2 will be capable to do this having the profiling information, and I'm not sure whether we should optimize for C1. In any case, this could be done separately later, after careful benchmarking. - PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v4]
On Mon, 6 Feb 2023 09:46:15 GMT, Tagir F. Valeev wrote: >> clamp() methods added to Math and StrictMath >> >> `int clamp(long, int, int)` is somewhat different, as it accepts a `long` >> value and safely clamps it to an `int` range. Other overloads work with a >> particular type (long, float and double). Using similar approach in other >> cases (e.g. `float clamp(double, float, float)`) may cause accidental >> precision loss even if the value is within range, so I decided to avoid this. >> >> In all cases, `max >= min` precondition should met. For double and float we >> additionally order `-0.0 < 0.0`, similarly to what Math.max or >> Double.compare do. In double and float overloads I try to keep at most one >> arg-check comparison on common path, so the order of checks might look >> unusual. >> >> For tests, I noticed that tests in java/lang/Math don't use any testing >> framework (even newer tests), so I somehow mimic the approach of neighbour >> tests. > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Added explanatory comments Suggestion: Refactor the uncommon cases into separate functions so the compiler can have easier time inlining the common path. Thanks - PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v2]
On Mon, 6 Feb 2023 09:03:48 GMT, Quan Anh Mai wrote: >> @ExE-Boss I think that immediately following `isNaN` checks give enough hint >> that we want NaN to be here. > > Ah I see, a comment explaining the intention would be helpful here, then Ok, added some explanatory comments. Hopefully they clarify the intention. - PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v4]
> clamp() methods added to Math and StrictMath > > `int clamp(long, int, int)` is somewhat different, as it accepts a `long` > value and safely clamps it to an `int` range. Other overloads work with a > particular type (long, float and double). Using similar approach in other > cases (e.g. `float clamp(double, float, float)`) may cause accidental > precision loss even if the value is within range, so I decided to avoid this. > > In all cases, `max >= min` precondition should met. For double and float we > additionally order `-0.0 < 0.0`, similarly to what Math.max or Double.compare > do. In double and float overloads I try to keep at most one arg-check > comparison on common path, so the order of checks might look unusual. > > For tests, I noticed that tests in java/lang/Math don't use any testing > framework (even newer tests), so I somehow mimic the approach of neighbour > tests. Tagir F. Valeev has updated the pull request incrementally with one additional commit since the last revision: Added explanatory comments - Changes: - all: https://git.openjdk.org/jdk/pull/12428/files - new: https://git.openjdk.org/jdk/pull/12428/files/9b73965a..065018f4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12428&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12428&range=02-03 Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12428.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12428/head:pull/12428 PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v2]
On Sun, 5 Feb 2023 18:14:28 GMT, ExE Boss wrote:
>> Tagir F. Valeev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Typo in doc fixed
>
> src/java.base/share/classes/java/lang/Math.java line 2215:
>
>> 2213: public static int clamp(long value, int min, int max) {
>> 2214: if (min > max) {
>> 2215: throw new IllegalArgumentException(min + ">" + max);
>
> These should probably have some spacing or a better error message:
> Suggestion:
>
> throw new IllegalArgumentException(min + " > " + max);
Updated, thanks
-
PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v3]
> clamp() methods added to Math and StrictMath > > `int clamp(long, int, int)` is somewhat different, as it accepts a `long` > value and safely clamps it to an `int` range. Other overloads work with a > particular type (long, float and double). Using similar approach in other > cases (e.g. `float clamp(double, float, float)`) may cause accidental > precision loss even if the value is within range, so I decided to avoid this. > > In all cases, `max >= min` precondition should met. For double and float we > additionally order `-0.0 < 0.0`, similarly to what Math.max or Double.compare > do. In double and float overloads I try to keep at most one arg-check > comparison on common path, so the order of checks might look unusual. > > For tests, I noticed that tests in java/lang/Math don't use any testing > framework (even newer tests), so I somehow mimic the approach of neighbour > tests. Tagir F. Valeev has updated the pull request incrementally with one additional commit since the last revision: Spaces in exception message - Changes: - all: https://git.openjdk.org/jdk/pull/12428/files - new: https://git.openjdk.org/jdk/pull/12428/files/2ecba76e..9b73965a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12428&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12428&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12428.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12428/head:pull/12428 PR: https://git.openjdk.org/jdk/pull/12428
Re: JEP415: FilterInThread Example
Hi Roger,
thanks for your quick response. That does seem to work better.
Regards
Heinz
--
Dr Heinz M. Kabutz (PhD CompSci)
Author of "The Java™ Specialists' Newsletter" -www.javaspecialists.eu
Java Champion -www.javachampions.org
JavaOne Rock Star Speaker
Tel: +30 69 75 595 262
Skype: kabutz
On 2023/02/06 06:55, Roger Riggs wrote:
*// Returns a composite filter of the static JVM-wide filter, a
thread-specific filter, *// and the stream-specific filter. *public
ObjectInputFilter apply(ObjectInputFilter curr, ObjectInputFilter
next) { *if (curr == null) { *// Called from the OIS constructor or
perhaps OIS.setObjectInputFilter with no current filter *var filter =
filterThreadLocal.get(); *if (filter != null) { *// Merge to invoke
the thread local filter and then the JVM-wide filter (if any) *filter
= ObjectInputFilter.merge(filter, next); *return
ObjectInputFilter.rejectUndecidedClass(filter); *} *return (next ==
null) ? null : ObjectInputFilter.rejectUndecidedClass(next); *} else {
*// Called from OIS.setObjectInputFilter with a current filter and a
stream-specific filter. *// The curr filter already incorporates the
thread filter and static JVM-wide filter *// and rejection of
undecided classes *// If there is a stream-specific filter merge to
invoke it and then the current filter. *if (next != null) { *return
ObjectInputFilter.merge(next, curr); *} *return curr; *} *}
Re: RFR: JDK-8301833: Add manual tests for FDLIBM porting
On Mon, 6 Feb 2023 01:50:55 GMT, Joe Darcy wrote: > To help add assurances that the main-line port of FDLIBM to Java is working > correctly, added some long-running manual tests to probe that the > transliteration port and the corresponding StrictMath method are in agreement > on a large number of argument, say, all float values. > > To test the transliteration port, this test can be run against a build where > the JDK has *not* had the FDLIBM code used for StrictMath ported to Java. Manual tests are the tests of last resort :-) This test may be useful for the current transliteration work but it's not clear how this manual test would be run by someone tasked with running the manual tests. Right now, it looks like it's more of a long running stress test but I think the expectation is that someone running this test needs to do a special build or somehow compare results with an older JDK release. Have you explored alternatives to adding a manual test? Long running HotSpot stress tests run in higher tiers. For comparison, I'm wondering if samples could be captured in a golden file for the test to use. If we are adding a manual test here then I think it will need good instructions. @bwhuang-us has done work in recent times on making sure that the manual tests are in test groups with instructions. - PR: https://git.openjdk.org/jdk/pull/12430
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v2]
On Mon, 6 Feb 2023 08:46:54 GMT, Tagir F. Valeev wrote: >> That should probably include a comment then. > > @ExE-Boss I think that immediately following `isNaN` checks give enough hint > that we want NaN to be here. Ah I see, a comment explaining the intention would be helpful here, then - PR: https://git.openjdk.org/jdk/pull/12428
Withdrawn: 8301834: Templated Buffer classes leave a lot of empty lines in the generated source
On Mon, 6 Feb 2023 06:57:43 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8301834? > > Some classes in `java.nio` package are generated from template files, during > the build. The template files are processed by a build tool implemented by a > Java class `make/jdk/src/classes/build/tools/spp/Spp.java`. This template > processing tool allows for an (optional) parameter called `-nel` which as per > its documentation: > >> If -nel is declared then empty lines will not be substituted for lines of >> text in the template that do not appear in the output. > > Various places in the JDK build where this tool is used to generate source > from template files, already use the `-nel` option to not generate the empty > lines in the source files. However, the `GensrcBuffer.gmk` which generates > the source for `java.nio` classes doesn't use this option. The commit in this > PR adds this option when generating the `java.nio` classes. > > Existing tests in `test/jdk/java/nio` continue to pass after this change. > I've checked the generated content and compared it with the older versions to > verify that these empty lines no longer appear in these generated classes. > > Additional `tier` testing has been triggered to make sure no regression is > introduced. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/12431
Re: RFR: 8301834: Templated Buffer classes leave a lot of empty lines in the generated source
On Mon, 6 Feb 2023 06:57:43 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8301834? > > Some classes in `java.nio` package are generated from template files, during > the build. The template files are processed by a build tool implemented by a > Java class `make/jdk/src/classes/build/tools/spp/Spp.java`. This template > processing tool allows for an (optional) parameter called `-nel` which as per > its documentation: > >> If -nel is declared then empty lines will not be substituted for lines of >> text in the template that do not appear in the output. > > Various places in the JDK build where this tool is used to generate source > from template files, already use the `-nel` option to not generate the empty > lines in the source files. However, the `GensrcBuffer.gmk` which generates > the source for `java.nio` classes doesn't use this option. The commit in this > PR adds this option when generating the `java.nio` classes. > > Existing tests in `test/jdk/java/nio` continue to pass after this change. > I've checked the generated content and compared it with the older versions to > verify that these empty lines no longer appear in these generated classes. > > Additional `tier` testing has been triggered to make sure no regression is > introduced. Thank you Alan for pointing to the previous discussion. - PR: https://git.openjdk.org/jdk/pull/12431
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v2]
On Sun, 5 Feb 2023 18:08:47 GMT, ExE Boss wrote: >> No. I want NaNs to go into this branch > > That should probably include a comment then. @ExE-Boss I think that immediately following `isNaN` checks give enough hint that we want NaN to be here. - PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v2]
On Mon, 6 Feb 2023 03:25:50 GMT, Quan Anh Mai wrote: >> No. I want NaNs to go into this branch > > @amaembo Should that be `if (!(min <= max))` instead? @merykitty no. I want `min = +0.0` and `max = -0.0` to go into this branch, so we can report it. A marginal case when `min` and `max` are exactly equal goes through all the checks and still succeeds (it's covered by unit-tests). - PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v2]
On Sun, 5 Feb 2023 18:13:35 GMT, ExE Boss wrote:
>> Tagir F. Valeev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Typo in doc fixed
>
> src/java.base/share/classes/java/lang/Math.java line 2213:
>
>> 2211: * @since 21
>> 2212: */
>> 2213: public static int clamp(long value, int min, int max) {
>
> There should probably also be a pure `int` overload:
>
> public static int clamp(int value, int min, int max)
@ExE-Boss which problem such an overload would solve? It looks like, `int
clamp(long, int, int)` can be used everywhere where proposed `int clamp(int,
int, int)` could be useful.
-
PR: https://git.openjdk.org/jdk/pull/12428
Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java
On Sun, 5 Feb 2023 03:05:55 GMT, Joe Darcy wrote:
> Initial pass of porting FDLIBM sinh/cosh/tanh to Java. I do intend to
> refactor the regression tests a bit to reduce duplication, but the actual
> ports should be ready for review.
>
> Diff'ing the ports as before, original vs transliteration port:
>
>
> $ diff -w Hyperbolic.c Hyperbolic.translit.java
> 1c1
> < /* __ieee754_sinh(x)
> ---
>> /**
> 17a18,19
>> static class Sinh {
>> private static final double one = 1.0, shuge = 1.0e307;
> 19,33c21
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, shuge = 1.0e307;
> < #else
> < static double one = 1.0, shuge = 1.0e307;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_sinh(double x)
> < #else
> < double __ieee754_sinh(x)
> < double x;
> < #endif
> < {
> ---
>> private static double compute(double x) {
> 36c24
> < unsigned lx;
> ---
>> /* unsigned */ int lx;
> 51c39
> < t = expm1(fabs(x));
> ---
>> t = FdlibmTranslit.expm1(Math.abs(x));
> 57c45
> < if (ix < 0x40862E42) return h*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return h*StrictMath.exp(Math.abs(x)); //
>> TODO switch to translit
> 60,62c48,52
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> < if (ix<0x408633CE ||
> ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(0.5*fabs(x));
> ---
>> // lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
>> // lx = (((*(unsigned*)&one)>>29)) + (unsigned*)&x ;
>> lx = __LO(x);
>> if (ix<0x408633CE ||
>> ((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 ))) {
>> w = StrictMath.exp(0.5*Math.abs(x)); // TODO switch to
>> translit
> 70c60,62
> < /* __ieee754_cosh(x)
> ---
>> }
>>
>> /**
> 90,105c82,84
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one = 1.0, half=0.5, huge = 1.0e300;
> < #else
> < static double one = 1.0, half=0.5, huge = 1.0e300;
> < #endif
> <
> < #ifdef __STDC__
> < double __ieee754_cosh(double x)
> < #else
> < double __ieee754_cosh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Cosh {
>> private static final double one = 1.0, half=0.5, huge = 1.0e300;
>> private static double compute(double x) {
> 108c87
> < unsigned lx;
> ---
>> /*unsigned*/ int lx;
> 119c98
> < t = expm1(fabs(x));
> ---
>> t = expm1(Math.abs(x));
> 127c106
> < t = __ieee754_exp(fabs(x));
> ---
>> t = StrictMath.exp(Math.abs(x)); // TODO switch to translit
> 132c111
> < if (ix < 0x40862E42) return half*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42) return half*StrictMath.exp(Math.abs(x));
>> // TODO switch to translit
> 135c114
> < lx = *( (((*(unsigned*)&one)>>29)) + (unsigned*)&x);
> ---
>> lx = __LO(x);
> 137,138c116,117
> < ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(half*fabs(x));
> ---
>> ((ix==0x408633ce)&&(Integer.compareUnsigned(lx, 0x8fb9f87d)
>> <= 0))) {
>> w = StrictMath.exp(half*Math.abs(x)); // TODO switch to
>> translit
> 146c125,127
> < /* Tanh(x)
> ---
>> }
>>
>> /**
> 169,184c150,152
> <
> < #include "fdlibm.h"
> <
> < #ifdef __STDC__
> < static const double one=1.0, two=2.0, tiny = 1.0e-300;
> < #else
> < static double one=1.0, two=2.0, tiny = 1.0e-300;
> < #endif
> <
> < #ifdef __STDC__
> < double tanh(double x)
> < #else
> < double tanh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Tanh {
>> private static final double one=1.0, two=2.0, tiny = 1.0e-300;
>> static double compute(double x) {
> 203c171
> < t = expm1(two*fabs(x));
> ---
>> t = expm1(two*Math.abs(x));
> 206c174
> < t = expm1(-two*fabs(x));
> ---
>> t = expm1(-two*Math.abs(x));
> 214a183
>> }
>
>
> Note that the original has a in-line version of the "__LO" macro rather than
> using the macro.
>
>
> And transliteration vs more idiomatic:
>
>
> $ diff -w Hyperbolic.translit.java Hyperbolic.fdlibm.java
> 21c21
> < private static double compute(double x) {
> ---
>> static double compute(double x) {
> 26c26
> < /* High word of |x|. */
> ---
>> // High word of |x|
> 28c28
> < ix = jx&0x7fff;
> ---
>> ix = jx & 0x7fff_;
> 30,31c30,33
> < /* x is INF or NaN */
> < if(ix>=0x7ff0) return x+x;
> ---
>> // x is INF or NaN
>> if ( ix >= 0x7ff0_) {
>> return x + x;
>> }
> 34,40c36,48
> < if (jx<0) h = -h;
> < /* |x| i
Re: RFR: JDK-8301396: Port fdlibm expm1 to Java [v2]
On Fri, 3 Feb 2023 21:04:15 GMT, Joe Darcy wrote:
>> Next on the FDLIBM C -> Java port, expm1.
>> Coming soon, hyperbolic transcendentals (sinh, cosh, tanh)!
>>
>> For expm1, the C vs transliteration port show the usual kind of differences,
>> beside formatting of the constants, the use of the __HI macro on the
>> left-hand side needs to be replaced by a method call and an assignment, as
>> seen below:
>>
>>
>> <
>> < #include "fdlibm.h"
>> <
>> < #ifdef __STDC__
>> < static const double
>> < #else
>> < static double
>> < #endif
>> < one = 1.0,
>> < huge= 1.0e+300,
>> < tiny= 1.0e-300,
>> < o_threshold = 7.09782712893383973096e+02,/* 0x40862E42, 0xFEFA39EF */
>> < ln2_hi = 6.93147180369123816490e-01,/* 0x3fe62e42, 0xfee0 */
>> < ln2_lo = 1.90821492927058770002e-10,/* 0x3dea39ef, 0x35793c76 */
>> < invln2 = 1.44269504088896338700e+00,/* 0x3ff71547, 0x652b82fe */
>> ---
>>> static class Expm1 {
>>> private static final double one = 1.0;
>>> private static final double huge= 1.0e+300;
>>> private static final double tiny= 1.0e-300;
>>> private static final double o_threshold =
>>> 7.09782712893383973096e+02; /* 0x40862E42, 0xFEFA39EF */
>>> private static final double ln2_hi =
>>> 6.93147180369123816490e-01; /* 0x3fe62e42, 0xfee0 */
>>> private static final double ln2_lo =
>>> 1.90821492927058770002e-10; /* 0x3dea39ef, 0x35793c76 */
>>> private static final double invln2 =
>>> 1.44269504088896338700e+00; /* 0x3ff71547, 0x652b82fe */
>> 111,115c104,108
>> < Q1 = -3.31316428e-02, /* BFA1 10F4 */
>> < Q2 = 1.58730158725481460165e-03, /* 3F5A01A0 19FE5585 */
>> < Q3 = -7.93650757867487942473e-05, /* BF14CE19 9EAADBB7 */
>> < Q4 = 4.00821782732936239552e-06, /* 3ED0CFCA 86E65239 */
>> < Q5 = -2.01099218183624371326e-07; /* BE8AFDB7 6E09C32D */
>> ---
>>> private static final double Q1 = -3.31316428e-02; /*
>>> BFA1 10F4 */
>>> private static final double Q2 = 1.58730158725481460165e-03; /*
>>> 3F5A01A0 19FE5585 */
>>> private static final double Q3 = -7.93650757867487942473e-05; /*
>>> BF14CE19 9EAADBB7 */
>>> private static final double Q4 = 4.00821782732936239552e-06; /*
>>> 3ED0CFCA 86E65239 */
>>> private static final double Q5 = -2.01099218183624371326e-07; /*
>>> BE8AFDB7 6E09C32D */
>> 117,123c110
>> < #ifdef __STDC__
>> < double expm1(double x)
>> < #else
>> < double expm1(x)
>> < double x;
>> < #endif
>> < {
>> ---
>>> static double compute(double x) {
>> 126c113
>> < unsigned hx;
>> ---
>>> /*unsigned*/ int hx;
>> 157c144
>> < k = invln2*x+((xsb==0)?0.5:-0.5);
>> ---
>>> k = (int)(invln2*x+((xsb==0)?0.5:-0.5));
>> 188c175
>> < __HI(y) += (k<<20); /* add k to y's exponent */
>> ---
>>> y = __HI(y, __HI(y) + (k<<20)); /* add k to y's
>>> exponent */
>> 193c180
>> < __HI(t) = 0x3ff0 - (0x20>>k); /* t=1-2^-k */
>> ---
>>> t = __HI(t, 0x3ff0 - (0x20>>k)); /* t=1-2^-k */
>> 195c182
>> < __HI(y) += (k<<20); /* add k to y's exponent */
>> ---
>>> y = __HI(y, __HI(y) + (k<<20)); /* add k to y's
>>> exponent */
>> 197c184
>> < __HI(t) = ((0x3ff-k)<<20); /* 2^-k */
>> ---
>>> t = __HI(t, ((0x3ff-k)<<20)); /* 2^-k */
>> 200c187
>> < __HI(y) += (k<<20); /* add k to y's exponent */
>> ---
>>> y = __HI(y, __HI(y) + (k<<20)); /* add k to y's
>>> exponent */
>> 205c192
>> <
>> ---
>>> }
>>
>>
>> When comparing the transliteration port and the more idiomatic port, there
>> were no surprising or notable differences:
>>
>>
>> $ diff -w Expm1.translit.java Expm1.fdlibm.java
>> 99,102c99,102
>> < private static final double o_threshold =
>> 7.09782712893383973096e+02; /* 0x40862E42, 0xFEFA39EF */
>> < private static final double ln2_hi =
>> 6.93147180369123816490e-01; /* 0x3fe62e42, 0xfee0 */
>> < private static final double ln2_lo =
>> 1.90821492927058770002e-10; /* 0x3dea39ef, 0x35793c76 */
>> < private static final double invln2 =
>> 1.44269504088896338700e+00; /* 0x3ff71547, 0x652b82fe */
>> ---
>>> private static final double o_threshold = 0x1.62e42fefa39efp9;
>>> // 7.09782712893383973096e+02
>>> private static final double ln2_hi = 0x1.62e42feep-1;
>>> // 6.93147180369123816490e-01
>>> private static final double ln2_lo = 0x1.a39ef35793c76p-33;
>>> // 1.90821492927058770002e-10
>>> private static final double invln2 = 0x1.71547652b82fep
Re: RFR: 8301834: Templated Buffer classes leave a lot of empty lines in the generated source
On Mon, 6 Feb 2023 06:57:43 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8301834? > > Some classes in `java.nio` package are generated from template files, during > the build. The template files are processed by a build tool implemented by a > Java class `make/jdk/src/classes/build/tools/spp/Spp.java`. This template > processing tool allows for an (optional) parameter called `-nel` which as per > its documentation: > >> If -nel is declared then empty lines will not be substituted for lines of >> text in the template that do not appear in the output. > > Various places in the JDK build where this tool is used to generate source > from template files, already use the `-nel` option to not generate the empty > lines in the source files. However, the `GensrcBuffer.gmk` which generates > the source for `java.nio` classes doesn't use this option. The commit in this > PR adds this option when generating the `java.nio` classes. > > Existing tests in `test/jdk/java/nio` continue to pass after this change. > I've checked the generated content and compared it with the older versions to > verify that these empty lines no longer appear in these generated classes. > > Additional `tier` testing has been triggered to make sure no regression is > introduced. See JDK-8207804 for the last discussion on this issue. - PR: https://git.openjdk.org/jdk/pull/12431
