Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v7]
On Tue, 22 Mar 2022 21:25:28 GMT, Valerie Peng wrote: >> It's been several years since we increased the default key sizes. Before >> shifting to PQC, NSA replaced its Suite B cryptography recommendations with >> the Commercial National Security Algorithm Suite which suggests: >> >> - SHA-384 for secure hashing >> - AES-256 for symmetric encryption >> - RSA with 3072 bit keys for digital signatures and for key exchange >> - Diffie Hellman (DH) with 3072 bit keys for key exchange >> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures >> (ECDSA) >> >> So, this proposed changes made the suggested key size and algorithm changes. >> The changes are mostly in keytool, jarsigner and their regression tests, so >> @wangweij Could you please take a look? >> >> Thanks! > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Minor code refactoring src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java line 129: > 127: return currVal; > 128: } > 129: I'm not very sure of this method. Is it performance friendly if making the default key size calculation in the static block (from line 142 to the end of the file)? Then, the DEF_AES_KEY_SIZE could be a public primitive int. Or did I miss something? src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java line 137: > 135: public static final int DEF_ED_KEY_SIZE; > 136: public static final int DEF_XEC_KEY_SIZE; > 137: private static final AtomicInteger DEF_AES_KEY_SIZE; See the previous comment, maybe it could be `public static final int DEF_AES_KEY_SIZE.` - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v3]
On Wed, 23 Mar 2022 02:01:08 GMT, ExE Boss wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add exception message
>
> src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 424:
>
>> 422: throw new IncompatibleClassChangeError(msg);
>> 423: } else {
>> 424: throw new NullPointerException("Cannot invoke " +
>> member + " with null receiver");
>
> Suggestion:
>
> String msg = String.format("Cannot invoke %s with null
> receiver",
>member);
> throw new NullPointerException(msg);
Good catch. Will update it.
-
PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: 8283279: [Testbug] Improve TestGetSwapSpaceSize
On Thu, 17 Mar 2022 13:40:53 GMT, Severin Gehwolf wrote:
> Please review this container test improvement. The test in question only
> makes sense iff the total swap space size as reported by the container aware
> OperatingSystemMXBean is `0`. If that's not the case, then something else
> might be amiss, e.g. OperatingSystemMXBean reporting the host swap limits. In
> such a case a passing test tells us nothing. Certainly not if the
> fix from [JDK-8242480](https://bugs.openjdk.java.net/browse/JDK-8242480) is
> present or not.
>
> Testing:
> - [x] Manual with and without the code fix of JDK-8242480. Still passes with
> the fix, and fails without. Tested the test on cgroups v1 and cgroups v2.
Please also update the copyright year.
Thanks.
test/jdk/jdk/internal/platform/docker/GetFreeSwapSpaceSize.java line 28:
> 26:
> 27: // Usage:
> 28: // GetFreeSwapSpaceSize
>
I would suggest
// GetFreeSwapSpaceSize
test/jdk/jdk/internal/platform/docker/GetFreeSwapSpaceSize.java line 32:
> 30: public static void main(String[] args) {
> 31: if (args.length != 4) {
> 32: throw new RuntimeException("Unexpected arguments. Expected 2,
> got " + args.length);
Shouldn't be `Expected 4` ?
-
PR: https://git.openjdk.java.net/jdk/pull/7854
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
On Tue, 22 Mar 2022 22:02:22 GMT, Naoto Sato wrote: >> Fixing the out-of-date number of entries in >> `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been >> renamed and now repurposed just to examine whether the `NUM_ENTITIES` >> correctly has the `map.size()` value. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Comment adjusted per review suggestion Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
On Tue, 22 Mar 2022 21:58:27 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/lang/Character.java line 740:
>>
>>> 738: public static final class UnicodeBlock extends Subset {
>>> 739: /**
>>> 740: * 737 - the expected number of entities
>>
>> Just a quibble about this comment... it's probably not worth repeating the
>> actual value. But it probably is worth mentioning that the actual value
>> should (or must) match the number of entries added to the map by
>> constructors called from the static initializers in this class. Whenever
>> aliases or new blocks are added, this number must be adjusted.
>
> Not a "quibble" at all. In fact, I thought the same just after I submitted
> the PR that the number in the comment would be easily overlooked and got
> stale, which would defy this cleanup. Removed the actual number and put some
> explanation there.
Thanks, looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v4]
On Wed, 23 Mar 2022 02:03:26 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix copyright header
Update looks good.
Testing results are also good.
-
Marked as reviewed by kvn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v3]
On Tue, 22 Mar 2022 17:55:13 GMT, Mandy Chung wrote:
>> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null
>> receiver check rather than NPE thrown by `Object::getClass`. The message of
>> NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be
>> helpful but not in this case.
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add exception message
This needs to use `String.format(…)` in order to avoid circular dependencies in
bootstraps on `java.lang.invoke.StringConcatFactory`.
src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 424:
> 422: throw new IncompatibleClassChangeError(msg);
> 423: } else {
> 424: throw new NullPointerException("Cannot invoke " +
> member + " with null receiver");
Suggestion:
String msg = String.format("Cannot invoke %s with null
receiver",
member);
throw new NullPointerException(msg);
src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 456:
> 454: throw new IncompatibleClassChangeError(msg);
> 455: } else {
> 456: throw new NullPointerException("Cannot invoke " +
> member + " with null receiver");
Suggestion:
String msg = String.format("Cannot invoke %s with null
receiver",
member);
throw new NullPointerException(msg);
-
PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Wed, 23 Mar 2022 01:57:25 GMT, Fei Yang wrote: >> src/hotspot/cpu/riscv/disassembler_riscv.hpp line 18: >> >>> 16: * >>> 17: * You should have received a copy of the GNU General Public License >>> version >>> 18: * 2 along with this work; if not, write to the Free Software >>> Foundation, * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. >> >> These 2 lines merged into 1 accidentally causing failure in copyright >> headers verification. > >> I looked on C1/C2 changes and compiler tests. Seems reasonable. But before >> approval I would need to run changes through our testing. > > That's great to hear :-) Thanks for the efforts. I have fixed the copyright headers verification problem. Please take another look. - PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 17:34:18 GMT, Vladimir Kozlov wrote: >> Fei Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address review comments > > src/hotspot/cpu/riscv/disassembler_riscv.hpp line 18: > >> 16: * >> 17: * You should have received a copy of the GNU General Public License >> version >> 18: * 2 along with this work; if not, write to the Free Software >> Foundation, * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. > > These 2 lines merged into 1 accidentally causing failure in copyright > headers verification. > I looked on C1/C2 changes and compiler tests. Seems reasonable. But before > approval I would need to run changes through our testing. That's great to hear :-) Thanks for the efforts. - PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 14:01:28 GMT, Roger Riggs wrote: > The test/jdk files look ok. (I didn't look at the rest) Thank you for looking at that part. - PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v4]
> This PR implements JEP 422: Linux/RISC-V Port [1].
> The PR starts as a squashed merge of the
> https://openjdk.java.net/projects/riscv-port branch.
>
> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are also
> carried out regularly. So it should be good enough to run most Java programs.
>
> [1] https://openjdk.java.net/jeps/422
Fei Yang has updated the pull request incrementally with one additional commit
since the last revision:
Fix copyright header
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/6294/files
- new: https://git.openjdk.java.net/jdk/pull/6294/files/b7a31729..d8bef7fa
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=6294=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=6294=02-03
Stats: 28 lines in 13 files changed: 14 ins; 0 del; 14 mod
Patch: https://git.openjdk.java.net/jdk/pull/6294.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6294/head:pull/6294
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v7]
On Tue, 22 Mar 2022 21:25:28 GMT, Valerie Peng wrote: >> It's been several years since we increased the default key sizes. Before >> shifting to PQC, NSA replaced its Suite B cryptography recommendations with >> the Commercial National Security Algorithm Suite which suggests: >> >> - SHA-384 for secure hashing >> - AES-256 for symmetric encryption >> - RSA with 3072 bit keys for digital signatures and for key exchange >> - Diffie Hellman (DH) with 3072 bit keys for key exchange >> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures >> (ECDSA) >> >> So, this proposed changes made the suggested key size and algorithm changes. >> The changes are mostly in keytool, jarsigner and their regression tests, so >> @wangweij Could you please take a look? >> >> Thanks! > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Minor code refactoring Everything looks fine to me. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings [v5]
> If AbstractStringBuilder only grow, the inflated value which has been encoded > in UTF16 can't be compressed. > toString() can skip compression in this case. This can save an > ArrayAllocation in StringUTF16::compress(). > > java.io.BufferedRead::readLine() is a case that StringBuilder grows only. > > In microbench, we expect to see that allocation/op reduces 20%. The initial > capacity of StringBuilder is S in bytes. When it encounters the 1st character > that can't be encoded in LATIN1, it inflates and allocate a new array of 2*S. > `toString()` will try to compress that value so it need to allocate S bytes. > The last step allocates 2*S bytes because it has to copy the string. so it > requires to allocate 5 * S bytes in total. By skipping the failed > compression, it only allocates 4 * S bytes. that's 20%. In real execution, > we observe 16% allocation reduction, tracked by JMH GC profiler > `gc.alloc.rate.norm `. I think it's because HotSpot can't track all > allocations. > > Not only allocation drops, the runtime performance(ns/op) also increases from > 3.34% to 18.91%. > > Before: > > $$make test > TEST="micro:org.openjdk.bench.java.lang.StringBuilders.toStringWithMixedChars" > MICRO="OPTIONS=-prof gc -gc true -o before.log -jvm > $HOME/Devel/jdk_baseline/bin/java" > > Benchmark > (MIXED_SIZE) Mode Cnt Score Error Units > StringBuilders.toStringWithMixedChars >128 avgt 15 649.846 ± 76.291 ns/op > StringBuilders.toStringWithMixedChars:·gc.alloc.rate >128 avgt 15 872.855 ± 128.259 MB/sec > StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm >128 avgt 15 880.121 ± 0.050B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space >128 avgt 15 707.730 ± 194.421 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm >128 avgt 15 706.602 ± 94.504B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space >128 avgt 15 0.001 ± 0.002 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm >128 avgt 15 0.001 ± 0.001B/op > StringBuilders.toStringWithMixedChars:·gc.count >128 avgt 15 113.000counts > StringBuilders.toStringWithMixedChars:·gc.time >128 avgt 1585.000ms > StringBuilders.toStringWithMixedChars >256 avgt 15 1316.652 ± 112.771 ns/op > StringBuilders.toStringWithMixedChars:·gc.alloc.rate >256 avgt 15 800.864 ± 76.869 MB/sec > StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm >256 avgt 15 1648.288 ± 0.162B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space >256 avgt 15 599.736 ± 174.001 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm >256 avgt 15 1229.669 ± 318.518B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space >256 avgt 15 0.001 ± 0.001 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm >256 avgt 15 0.001 ± 0.002B/op > StringBuilders.toStringWithMixedChars:·gc.count >256 avgt 15 133.000counts > StringBuilders.toStringWithMixedChars:·gc.time >256 avgt 1592.000ms > StringBuilders.toStringWithMixedChars > 1024 avgt 15 5204.303 ± 418.115 ns/op > StringBuilders.toStringWithMixedChars:·gc.alloc.rate > 1024 avgt 15 768.730 ± 72.945 MB/sec > StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm > 1024 avgt 15 6256.844 ± 0.358B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space > 1024 avgt 15 655.852 ± 121.602 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm > 1024 avgt 15 5315.265 ± 578.878B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space > 1024 avgt 15 0.002 ± 0.002 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm > 1024 avgt 15 0.014 ± 0.011B/op > StringBuilders.toStringWithMixedChars:·gc.count > 1024 avgt 1596.000counts > StringBuilders.toStringWithMixedChars:·gc.time > 1024 avgt 1586.000ms > > > After > > $make test >
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test Ah, that's great. Seems all tests that have `@library`... `/vmTestbase/vm/mlvm/patches` are ignored (you missed `test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/manyIndyOneCPX`) Safe to revert all the test changes. Now waiting for build again. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v4]
> Change `CallSite` to a sealed class, as `CallSite` is an abstract class which > does not allow direct subclassing by users per its documentation. Since I > don't have a JBS account, I posted the content for the CSR in a GitHub Gist > at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and wish someone > can submit a CSR for me. liach has updated the pull request incrementally with one additional commit since the last revision: Revert "Remove homemade callsite from test" This reverts commit 1bcd779f677420326bf365c3580ceab5a6b5e3fa. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7840/files - new: https://git.openjdk.java.net/jdk/pull/7840/files/1bcd779f..c0a10f6e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7840=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7840=02-03 Stats: 119 lines in 3 files changed: 113 ins; 2 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7840.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7840/head:pull/7840 PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test Which tests do you see? The following tests are all `@ignored`: test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/stress/gc/createLotsOfMHConsts/Test.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/mt/TestDescription.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/mh/TestDescription.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/incorrectBootstrap/TestDescription.java test/hotspot/jtreg/vmTestbase/vm/mlvm/cp/stress/classfmt/correctBootstrap/TestDescription.java - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Tue, 22 Mar 2022 21:55:34 GMT, Mandy Chung wrote: > I created CSR (https://bugs.openjdk.java.net/browse/JDK-8283530). I took the > liberty to adjust the CSR to include only the needed information. Since I'm > the RE for this CSR (you don't have JBS account), you will need another > reviewer for this CSR. I've added myself as a reviewer on the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test I see a few other constant pool stress tests compile the `java.base` patches, which includes this broken `CallSite` implementation. Should we remove that implementation, or does it not cause compilation failure as is? - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
On Tue, 22 Mar 2022 22:02:22 GMT, Naoto Sato wrote: >> Fixing the out-of-date number of entries in >> `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been >> renamed and now repurposed just to examine whether the `NUM_ENTITIES` >> correctly has the `map.size()` value. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Comment adjusted per review suggestion Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
On Tue, 22 Mar 2022 22:02:22 GMT, Naoto Sato wrote: >> Fixing the out-of-date number of entries in >> `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been >> renamed and now repurposed just to examine whether the `NUM_ENTITIES` >> correctly has the `map.size()` value. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Comment adjusted per review suggestion Added comments look good. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test I created CSR (https://bugs.openjdk.java.net/browse/JDK-8283530). I took the liberty to adjust the CSR to include only the needed information. Since I'm the RE for this CSR (you don't have JBS account), you will need another reviewer for this CSR. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
> Fixing the out-of-date number of entries in > `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been > renamed and now repurposed just to examine whether the `NUM_ENTITIES` > correctly has the `map.size()` value. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Comment adjusted per review suggestion - Changes: - all: https://git.openjdk.java.net/jdk/pull/7909/files - new: https://git.openjdk.java.net/jdk/pull/7909/files/7328e1e7..ec77e17d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7909=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7909=00-01 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7909.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7909/head:pull/7909 PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
On Tue, 22 Mar 2022 21:42:04 GMT, Stuart Marks wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Comment adjusted per review suggestion
>
> src/java.base/share/classes/java/lang/Character.java line 740:
>
>> 738: public static final class UnicodeBlock extends Subset {
>> 739: /**
>> 740: * 737 - the expected number of entities
>
> Just a quibble about this comment... it's probably not worth repeating the
> actual value. But it probably is worth mentioning that the actual value
> should (or must) match the number of entries added to the map by constructors
> called from the static initializers in this class. Whenever aliases or new
> blocks are added, this number must be adjusted.
Not a "quibble" at all. In fact, I thought the same just after I submitted the
PR that the number in the comment would be easily overlooked and got stale,
which would defy this cleanup. Removed the actual number and put some
explanation there.
-
PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date
On Tue, 22 Mar 2022 18:44:09 GMT, Naoto Sato wrote:
> Fixing the out-of-date number of entries in
> `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been
> renamed and now repurposed just to examine whether the `NUM_ENTITIES`
> correctly has the `map.size()` value.
Thanks for doing this! I was going to offer to do this myself but you beat me
to it. Rewritten test looks great.
src/java.base/share/classes/java/lang/Character.java line 740:
> 738: public static final class UnicodeBlock extends Subset {
> 739: /**
> 740: * 737 - the expected number of entities
Just a quibble about this comment... it's probably not worth repeating the
actual value. But it probably is worth mentioning that the actual value should
(or must) match the number of entries added to the map by constructors called
from the static initializers in this class. Whenever aliases or new blocks are
added, this number must be adjusted.
-
Marked as reviewed by smarks (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On Mar 22, 2022, at 12:28 PM, Volker Simonis
mailto:volker.simo...@gmail.com>> wrote:
On Mon, Mar 21, 2022 at 8:24 PM Lance Andersen
mailto:lance.ander...@oracle.com>> wrote:
Hi Lance,
Thanks for looking into this issue. Please find my answers inline:
Hi Volker,
I have read through what you have provided/pointed to, thank you, and on the
surface what you are suggesting sounds reasonable.
That being said given that this API dates back to 1997ish, I think we have to
be careful not introduce any regressions with existing applications with the
proposal you suggest(even though it is just relaxes the spec), as you mentioned
their is a jtreg test that seems to fail.
Have you run the JCK with your ZLIB implementation? I only skimmed the tests
but looks like there might be a couple of tests which validate the array’s
contents.
Yes, I did run the JCK tests with the optimized zlib implementations
and couldn't find any problems related to this issue. We've also using
the optimized version of zlib in production for quite a while without
any problems. However, I did found a problem related to a test which
was copied from "test/jdk/java/util/zip/DeflateIn_InflateOut.java".
That was the test on a quick scan I wondered about actually...
That jtreg test was fixed with "8240226: DeflateIn_InflateOut.java
test incorrectly assumes size of compressed" [2] in JDK 15 but
apparently that fix didn't made it into the JCK version. The test has
now been excluded from JCK 17/18.
Finally, the currently failing "nio/zipfs/ZipFSOutputStreamTest.java"
[3] jtreg test is not failing because it specifically checks that the
bytes in the output buffer beyond the last inflated byte remains
untouched. It's just because they use a poor, "lazy" method of
comparing inflated content to the original content and can easily be
fixed. Instead of:
```
while ((numRead = is.read(buf)) != -1) {
totalRead += numRead;
// verify the content
Assert.assertEquals(Arrays.mismatch(buf, chunk), -1, "Unexpected content");
}
```
just use:
```
while ((numRead = is.read(buf)) != -1) {
totalRead += numRead;
// verify the content
Assert.assertEquals(Arrays.mismatch(buf, 0, numRead, chunk, 0,
numRead), -1, "Unexpected content");
}
```
Why don’t you generate a PR to fix this separate from the change in spec
proposal?
[1]
https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/DeflateIn_InflateOut.java__;!!ACWV5N9M2RV99hQ!d0HFq-TCYrPXsCP8R_ja46jCDmVAXQCpz58ddo4rzXdRdDikJ27vpsKzBuKmMKov2g$
[2] https://bugs.openjdk.java.net/browse/JDK-8240226
[3]
https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java__;!!ACWV5N9M2RV99hQ!d0HFq-TCYrPXsCP8R_ja46jCDmVAXQCpz58ddo4rzXdRdDikJ27vpsKzBuJSeRPJgw$
We don’t have a lot of test coverage so if we were to consider moving forward
with your proposal, I believe additional test coverage would be warranted.
Sure, I'll be happy to add some more testing. Do you have specific
ideas? In fact my suggestion relaxes the specification of read(..)
which would be hard to check :)
Just some general coverage of the API would be great not expecting tests for
the relaxed specification
I think you are probably at a point where you can craft a draft CSR and PR and
we can continue to review and discuss from there
Best
Lance
Thank you and best regards,
Volker
Best
Lance
On Mar 4, 2022, at 5:04 AM, Volker Simonis
mailto:volker.simo...@gmail.com>> wrote:
`java.util.zip.Inflater` is the Java wrapper class for zlib's inflater
functionality. `Inflater::inflate(byte[] output, int off, int len)`
currently calls zlib's native `inflate(..)` function and passes the
address of `output[off]` and `len` to it via JNI.
The specification of zlib's `inflate(..)` function (i.e. the [API
documentation in the original zlib
implementation](https://urldefense.com/v3/__https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h*L400__;Iw!!ACWV5N9M2RV99hQ!d0HFq-TCYrPXsCP8R_ja46jCDmVAXQCpz58ddo4rzXdRdDikJ27vpsKzBuKV0E1HqA$
))
doesn't give any guarantees with regard to usage of the output buffer.
It only states that upon completion the function will return the
number of bytes that have been written (i.e. "inflated") into the
output buffer.
The original zlib implementation only wrote as many bytes into the
output buffer as it inflated. However, this is not a hard requirement
and newer, more performant implementations of the zlib library like
[zlib-chromium](https://urldefense.com/v3/__https://chromium.googlesource.com/chromium/src/third_party/zlib/__;!!ACWV5N9M2RV99hQ!d0HFq-TCYrPXsCP8R_ja46jCDmVAXQCpz58ddo4rzXdRdDikJ27vpsKzBuJVkxWFew$
)
or
[zlib-cloudflare](https://urldefense.com/v3/__https://github.com/cloudflare/zlib__;!!ACWV5N9M2RV99hQ!d0HFq-TCYrPXsCP8R_ja46jCDmVAXQCpz58ddo4rzXdRdDikJ27vpsKzBuIpQS_D6w$
) can use more
bytes of the output buffer than they actually inflate as a scratch
buffer. See
Re: RFR: 8283237: CallSite should be a sealed class [v3]
On Fri, 18 Mar 2022 22:12:10 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Remove homemade callsite from test This `GenManyIndyCorrectBootstrap.java` test is currently ignored (JDK-8194951). Since this test is not executed and will need to be reworked anyway, I would suggest to leave this test as is and let JDK-8194951 to handle it properly. We should add a comment in JDK-8194951 to describe this change. P.S. Sorry for the late comment as I was on vacation last week. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v7]
> It's been several years since we increased the default key sizes. Before > shifting to PQC, NSA replaced its Suite B cryptography recommendations with > the Commercial National Security Algorithm Suite which suggests: > > - SHA-384 for secure hashing > - AES-256 for symmetric encryption > - RSA with 3072 bit keys for digital signatures and for key exchange > - Diffie Hellman (DH) with 3072 bit keys for key exchange > - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures > (ECDSA) > > So, this proposed changes made the suggested key size and algorithm changes. > The changes are mostly in keytool, jarsigner and their regression tests, so > @wangweij Could you please take a look? > > Thanks! Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Minor code refactoring - Changes: - all: https://git.openjdk.java.net/jdk/pull/7652/files - new: https://git.openjdk.java.net/jdk/pull/7652/files/c8ae1655..1eb63292 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7652=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7652=05-06 Stats: 20 lines in 1 file changed: 1 ins; 7 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/7652.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7652/head:pull/7652 PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v4]
On Tue, 22 Mar 2022 19:07:12 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation in Lib.gmk Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v4]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix indentation in Lib.gmk - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/4b2760d3..7ec71f73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=02-03 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date
On Tue, 22 Mar 2022 18:44:09 GMT, Naoto Sato wrote: > Fixing the out-of-date number of entries in > `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been > renamed and now repurposed just to examine whether the `NUM_ENTITIES` > correctly has the `map.size()` value. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date
On Tue, 22 Mar 2022 18:44:09 GMT, Naoto Sato wrote: > Fixing the out-of-date number of entries in > `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been > renamed and now repurposed just to examine whether the `NUM_ENTITIES` > correctly has the `map.size()` value. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7909
RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date
Fixing the out-of-date number of entries in `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been renamed and now repurposed just to examine whether the `NUM_ENTITIES` correctly has the `map.size()` value. - Commit messages: - 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date Changes: https://git.openjdk.java.net/jdk/pull/7909/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7909=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283465 Stats: 125 lines in 3 files changed: 50 ins; 73 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7909.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7909/head:pull/7909 PR: https://git.openjdk.java.net/jdk/pull/7909
RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar
Please review this change to the java/util/prefs/AddNodeChangeListener.jar test. Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh Preferences on each test run, MacOS does not seem to honor this, and still stores prefs in Library/. This test has long suffered intermittent failures. 8255031 added some debugging code; as of that change, the test fails fast as soon as an expected change event is not received. In particular, if the expected event is not received for adding the node, the test exits without removing the node. In this way, on Mac, the node can get "stuck" in the preferences of the machine. Future runs on the machine will already have the node, no node added change event will be generated (because the node was already there), the test will continue to fail without removing the node, etc. This matches observations on some CI machines. This change ensures that: * the test node is not present before the test * the test runs to completion, including removing the test node * the test node is not left behind after the test Thanks. - Commit messages: - Ensure test node does not already exist, and is always removed Changes: https://git.openjdk.java.net/jdk/pull/7908/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7908=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283349 Stats: 70 lines in 1 file changed: 30 ins; 17 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/7908.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7908/head:pull/7908 PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver [v3]
> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Add exception message - Changes: - all: https://git.openjdk.java.net/jdk/pull/7766/files - new: https://git.openjdk.java.net/jdk/pull/7766/files/c7224f2f..62f74c70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7766=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7766=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7766.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7766/head:pull/7766 PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
src/hotspot/cpu/riscv/disassembler_riscv.hpp line 18:
> 16: *
> 17: * You should have received a copy of the GNU General Public License
> version
> 18: * 2 along with this work; if not, write to the Free Software Foundation,
> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
These 2 lines merged into 1 accidentally causing failure in copyright headers
verification.
-
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
I looked on C1/C2 changes and compiler tests. Seems reasonable.
But before approval I would need to run changes through our testing.
-
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]
On Tue, 22 Mar 2022 14:04:07 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with three > additional commits since the last revision: > > - rename syslookup lib on Windows > - Add missing LIBS flag > - Simplify syslookup build changes make/modules/java.base/Lib.gmk line 217: > 215: LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \ > 216: LIBS := $(LIBCXX), \ > 217: LIBS_linux := -lc -lm -ldl, \ This looks much better, thanks! Now if you could just fix the indentation of the parameters to 4 spaces, it would be perfect. - PR: https://git.openjdk.java.net/jdk/pull/7888
Integrated: JDK-8283480: Make AbstractStringBuilder sealed
On Tue, 22 Mar 2022 00:01:59 GMT, Joe Darcy wrote: > As part of updating the core libraries to be sealed, the package-access > AbstractStringBuilder, implementation superclass of StringBuilder and > StringBuffer, can be marked as sealed with those two subclasses on its > permits list. This pull request has now been integrated. Changeset: f7d21c35 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/f7d21c3523d87584b62a1143bfe52d067cf77519 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod 8283480: Make AbstractStringBuilder sealed Reviewed-by: jjg, rriggs, jlaskey, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/7898
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On Mon, Mar 21, 2022 at 8:24 PM Lance Andersen
wrote:
Hi Lance,
Thanks for looking into this issue. Please find my answers inline:
> Hi Volker,
>
> I have read through what you have provided/pointed to, thank you, and on the
> surface what you are suggesting sounds reasonable.
>
> That being said given that this API dates back to 1997ish, I think we have to
> be careful not introduce any regressions with existing applications with the
> proposal you suggest(even though it is just relaxes the spec), as you
> mentioned their is a jtreg test that seems to fail.
>
> Have you run the JCK with your ZLIB implementation? I only skimmed the tests
> but looks like there might be a couple of tests which validate the array’s
> contents.
Yes, I did run the JCK tests with the optimized zlib implementations
and couldn't find any problems related to this issue. We've also using
the optimized version of zlib in production for quite a while without
any problems. However, I did found a problem related to a test which
was copied from "test/jdk/java/util/zip/DeflateIn_InflateOut.java".
That jtreg test was fixed with "8240226: DeflateIn_InflateOut.java
test incorrectly assumes size of compressed" [2] in JDK 15 but
apparently that fix didn't made it into the JCK version. The test has
now been excluded from JCK 17/18.
Finally, the currently failing "nio/zipfs/ZipFSOutputStreamTest.java"
[3] jtreg test is not failing because it specifically checks that the
bytes in the output buffer beyond the last inflated byte remains
untouched. It's just because they use a poor, "lazy" method of
comparing inflated content to the original content and can easily be
fixed. Instead of:
```
while ((numRead = is.read(buf)) != -1) {
totalRead += numRead;
// verify the content
Assert.assertEquals(Arrays.mismatch(buf, chunk), -1, "Unexpected content");
}
```
just use:
```
while ((numRead = is.read(buf)) != -1) {
totalRead += numRead;
// verify the content
Assert.assertEquals(Arrays.mismatch(buf, 0, numRead, chunk, 0,
numRead), -1, "Unexpected content");
}
```
[1]
https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/DeflateIn_InflateOut.java
[2] https://bugs.openjdk.java.net/browse/JDK-8240226
[3]
https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java
>
> We don’t have a lot of test coverage so if we were to consider moving forward
> with your proposal, I believe additional test coverage would be warranted.
Sure, I'll be happy to add some more testing. Do you have specific
ideas? In fact my suggestion relaxes the specification of read(..)
which would be hard to check :)
Thank you and best regards,
Volker
> Best
> Lance
>
>
> On Mar 4, 2022, at 5:04 AM, Volker Simonis wrote:
>
> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater
> functionality. `Inflater::inflate(byte[] output, int off, int len)`
> currently calls zlib's native `inflate(..)` function and passes the
> address of `output[off]` and `len` to it via JNI.
>
> The specification of zlib's `inflate(..)` function (i.e. the [API
> documentation in the original zlib
> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400))
> doesn't give any guarantees with regard to usage of the output buffer.
> It only states that upon completion the function will return the
> number of bytes that have been written (i.e. "inflated") into the
> output buffer.
>
> The original zlib implementation only wrote as many bytes into the
> output buffer as it inflated. However, this is not a hard requirement
> and newer, more performant implementations of the zlib library like
> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/)
> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more
> bytes of the output buffer than they actually inflate as a scratch
> buffer. See https://github.com/simonis/zlib-chromium for a more
> detailed description of their approach and its performance benefit.
>
> These new zlib versions can still be used transparently from Java
> (e.g. by putting them into the `LD_LIBRARY_PATH` or by using
> `LD_PRELOAD`), because they still fully comply to specification of
> `Inflater::inflate(..)`. However, we might run into problems when
> using the `Inflater` functionality from the `InflaterInputStream`
> class. `InflaterInputStream` is derived from from `InputStream` and as
> such, its `read(byte[] b, int off, int len)` method is quite
> constrained. It specifically specifies that if *k* bytes have been
> read, then "these bytes will be stored in elements `b[off]` through
> `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through
> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[]
> b, int off, int len)` (which is constrained by
> `InputStream::read(..)`'s specification) calls
> `Inflater::inflate(byte[] b, int off, int len)` and directly passes
> its output buffer down to the
Integrated: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly
On Fri, 4 Mar 2022 17:54:20 GMT, Jim Laskey wrote:
> Several attempts have been made to improve Formatter's numeric performance by
> caching the current Locale zero. Such fixes, however, ignore the real issue,
> which is the slowness of fetching DecimalFormatSymbols. By directly caching
> DecimalFormatSymbols in the Formatter, this enhancement streamlines the
> process of accessing Locale DecimalFormatSymbols and specifically
> getZeroDigit(). The result is a general improvement in the performance of
> numeric formatting.
>
>
> @Benchmark
> public void bigDecimalDefaultLocale() {
> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f", X);
> }
>
> @Benchmark
> public void bigDecimalLocaleAlternate() {
> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> }
>
> @Benchmark
> public void bigDecimalThaiLocale() {
> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> }
>
> @Benchmark
> public void integerDefaultLocale() {
> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d", x);
> }
>
> @Benchmark
> public void integerLocaleAlternate() {
> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> }
>
> @Benchmark
> public void integerThaiLocale() {
> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> }
>
>
> Before:
> Benchmark Mode Cnt Score Error Units
> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>
> After:
> Benchmark Mode Cnt Score Error Units
> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
This pull request has now been integrated.
Changeset: 557ff4b3
Author:Jim Laskey
URL:
https://git.openjdk.java.net/jdk/commit/557ff4b3558f95723ebaff680b8524b0cb979559
Stats: 93 lines in 3 files changed: 51 ins; 35 del; 7 mod
8282625: Formatter caches Locale/DecimalFormatSymbols poorly
Reviewed-by: naoto, rriggs, jpai
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
The test/jdk files look ok. (I didn't look at the rest)
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]
On Mon, 21 Mar 2022 17:36:53 GMT, Maurizio Cimadamore wrote: >> make/modules/java.base/Lib.gmk line 217: >> >>> 215: CXXFLAGS := $(CXXFLAGS_JDKLIB), \ >>> 216: LDFLAGS := $(LDFLAGS_JDKLIB) -Wl$(COMMA)--no-as-needed, \ >>> 217: LIBS := $(LIBCXX) -lc -lm -ldl, \ >> >> Instead of repeating this whole macro call for both Linux and non Linux, you >> can use parameters of the form LDFLAGS_linux and LIBS_linux to add the Linux >> specific flags. Something like this: >> >> >> LDFLAGS := $(LDFLAGS_JDKLIB), \ >> LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \ >> >> >> For the NAME field, there is no such support, so the way we usually do that >> is through a variable and conditionals before the macro call. What's the >> reason to have a different lib name on Windows? If they were the same, and >> the source file in windows/native/... had the same name, it would just >> automatically override in the build. >> >> I realize now that this is just moved code from jdk.incubator.foreign, and >> this patch is probably big enough as it is so no need to refactor the build >> logic at the same time. > > Good points - there is really no need AFAIK for the lib name to be different. > I'll do few experiments. I've fixed the makefile as you suggested - I agree the result is much simpler. I've tested the changes on mac/linux/win and everything looks good. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with three additional commits since the last revision: - rename syslookup lib on Windows - Add missing LIBS flag - Simplify syslookup build changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6bb1b5c9..4b2760d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=01-02 Stats: 28 lines in 3 files changed: 1 ins; 23 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v12]
> Several attempts have been made to improve Formatter's numeric performance by
> caching the current Locale zero. Such fixes, however, ignore the real issue,
> which is the slowness of fetching DecimalFormatSymbols. By directly caching
> DecimalFormatSymbols in the Formatter, this enhancement streamlines the
> process of accessing Locale DecimalFormatSymbols and specifically
> getZeroDigit(). The result is a general improvement in the performance of
> numeric formatting.
>
>
> @Benchmark
> public void bigDecimalDefaultLocale() {
> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f", X);
> }
>
> @Benchmark
> public void bigDecimalLocaleAlternate() {
> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> }
>
> @Benchmark
> public void bigDecimalThaiLocale() {
> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> }
>
> @Benchmark
> public void integerDefaultLocale() {
> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d", x);
> }
>
> @Benchmark
> public void integerLocaleAlternate() {
> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> }
>
> @Benchmark
> public void integerThaiLocale() {
> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> }
>
>
> Before:
> Benchmark Mode Cnt Score Error Units
> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>
> After:
> Benchmark Mode Cnt Score Error Units
> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
Jim Laskey has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains 16 additional commits since the
last revision:
- Merge branch 'master' into 8282625
- Merge branch 'master' into 8282625
- Merge branch 'master' into 8282625
- Correct indentation
- Add unit test for DecimalFormatSymbols.getLocale()
- Add comment about DecimalFormatSymbols being mutable objects.
- Revert "Cache DecimalFormatSymbols in DecimalFormatSymbols instead of
Formatter. No significant performance degradation."
This reverts commit fcbf66a2fe9641d3c3349f12cc7b32d8b84c6f72.
- Revert "Drop DecimalFormatSymbols.getLocale change"
This reverts commit b93cdb03ec68f24f4b8851c0966bb144c30b5110.
- Revert "Correct caching test"
This reverts commit bf7975396aaad4ed58d053bde8f54984215eeba5.
- Correct caching test
- ... and 6 more: https://git.openjdk.java.net/jdk/compare/cfdf1030...38251f9e
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7703/files
- new: https://git.openjdk.java.net/jdk/pull/7703/files/ffb39f97..38251f9e
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7703=11
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7703=10-11
Stats: 1783 lines in 672 files changed: 858 ins; 698 del; 227 mod
Patch: https://git.openjdk.java.net/jdk/pull/7703.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7703/head:pull/7703
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v11]
> Several attempts have been made to improve Formatter's numeric performance by
> caching the current Locale zero. Such fixes, however, ignore the real issue,
> which is the slowness of fetching DecimalFormatSymbols. By directly caching
> DecimalFormatSymbols in the Formatter, this enhancement streamlines the
> process of accessing Locale DecimalFormatSymbols and specifically
> getZeroDigit(). The result is a general improvement in the performance of
> numeric formatting.
>
>
> @Benchmark
> public void bigDecimalDefaultLocale() {
> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f", X);
> }
>
> @Benchmark
> public void bigDecimalLocaleAlternate() {
> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> }
>
> @Benchmark
> public void bigDecimalThaiLocale() {
> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
> %1$f %1$f", X);
> }
>
> @Benchmark
> public void integerDefaultLocale() {
> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d", x);
> }
>
> @Benchmark
> public void integerLocaleAlternate() {
> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> }
>
> @Benchmark
> public void integerThaiLocale() {
> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
> %1$d %1$d", x);
> }
>
>
> Before:
> Benchmark Mode Cnt Score Error Units
> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>
> After:
> Benchmark Mode Cnt Score Error Units
> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
Jim Laskey has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains 15 additional commits since the
last revision:
- Merge branch 'master' into 8282625
- Merge branch 'master' into 8282625
- Correct indentation
- Add unit test for DecimalFormatSymbols.getLocale()
- Add comment about DecimalFormatSymbols being mutable objects.
- Revert "Cache DecimalFormatSymbols in DecimalFormatSymbols instead of
Formatter. No significant performance degradation."
This reverts commit fcbf66a2fe9641d3c3349f12cc7b32d8b84c6f72.
- Revert "Drop DecimalFormatSymbols.getLocale change"
This reverts commit b93cdb03ec68f24f4b8851c0966bb144c30b5110.
- Revert "Correct caching test"
This reverts commit bf7975396aaad4ed58d053bde8f54984215eeba5.
- Correct caching test
- Drop DecimalFormatSymbols.getLocale change
- ... and 5 more: https://git.openjdk.java.net/jdk/compare/b8892a70...ffb39f97
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7703/files
- new: https://git.openjdk.java.net/jdk/pull/7703/files/84fa1fe7..ffb39f97
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7703=10
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7703=09-10
Stats: 9802 lines in 494 files changed: 4773 ins; 2477 del; 2552 mod
Patch: https://git.openjdk.java.net/jdk/pull/7703.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7703/head:pull/7703
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v11]
On Tue, 22 Mar 2022 12:56:02 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/lang/runtime/Carrier.java line 574:
>>
>>> 572: try {
>>> 573: Lookup lookup = MethodHandles.lookup();
>>> 574: return lookup.defineHiddenClass(bytes, false,
>>> ClassOption.STRONG);
>>
>> Actually, this lookup object should probably be kept cached.
>
> Which one, the context lookup or the hidden class lookup?
The context lookup used to define the hidden class, not the one returned. Don't
know how costly calling the caller sensitive `lookup()` is, if the calls are
too expensive then we'd rather cache it in a static final field
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v12]
On Tue, 22 Mar 2022 13:00:14 GMT, Jim Laskey wrote: >> We propose to provide a runtime anonymous carrier class object generator; >> java.lang.runtime.Carrier. This generator class is designed to share >> anonymous classes when shapes are similar. For example, if several clients >> require objects containing two integer fields, then Carrier will ensure that >> each client generates carrier objects using the same underlying anonymous >> class. >> >> See JBS for details. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Fix spacing on if( Just curious, when valhalla is out, does the specification of Carrier allow the implementation to move to value classes? - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8283480: Make AbstractStringBuilder sealed
On Tue, 22 Mar 2022 00:01:59 GMT, Joe Darcy wrote: > As part of updating the core libraries to be sealed, the package-access > AbstractStringBuilder, implementation superclass of StringBuilder and > StringBuffer, can be marked as sealed with those two subclasses on its > permits list. LGTM. The two subclasses are final. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7898
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On 22/03/2022 4:56 pm, Rémi Forax wrote:
On Tue, 22 Mar 2022 04:38:15 GMT, ExE Boss wrote:
javac should be changed to allow fully qualified access to private inner
classes in the permits clause of an enclosing class.
This is not how private works, private means accessible between the '{' and the
'}' of the class.
The permits clause is declared outside of the '{' and the '}' of the class,
thus a private member class is not visible from the permits clause.
This seems like an oversight in the Sealed classes specification. It
makes perfect sense to have a sealed class that can only be extended by
its own nested classes, so you should be able to clearly state that.
David
-
-
PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v11]
On Mon, 21 Mar 2022 18:02:07 GMT, ExE Boss wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 18 additional
>> commits since the last revision:
>>
>> - Remove LOOKUP static final
>> - Merge branch 'master' into 8282798
>> - Typos
>> - Update Carrier.java
>> - Redo API to use list, bring Carrier.component back
>> - Clean up API
>> - Remove redundant MethodHandle component(MethodType methodType, int i) API
>> - Revert to {@return} style comments.
>> - Clarify primitive store in array carriers.
>> - Use long array for primitives
>> - ... and 8 more:
>> https://git.openjdk.java.net/jdk/compare/b726367e...a8657bbe
>
> src/java.base/share/classes/java/lang/runtime/Carrier.java line 574:
>
>> 572: try {
>> 573: Lookup lookup = MethodHandles.lookup();
>> 574: return lookup.defineHiddenClass(bytes, false,
>> ClassOption.STRONG);
>
> Actually, this lookup object should probably be kept cached.
Which one, the context lookup or the hidden class lookup?
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v11]
On Tue, 22 Mar 2022 10:20:15 GMT, Daniel Fuchs wrote:
>> Jim Laskey has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 18 additional
>> commits since the last revision:
>>
>> - Remove LOOKUP static final
>> - Merge branch 'master' into 8282798
>> - Typos
>> - Update Carrier.java
>> - Redo API to use list, bring Carrier.component back
>> - Clean up API
>> - Remove redundant MethodHandle component(MethodType methodType, int i) API
>> - Revert to {@return} style comments.
>> - Clarify primitive store in array carriers.
>> - Use long array for primitives
>> - ... and 8 more:
>> https://git.openjdk.java.net/jdk/compare/b726367e...a8657bbe
>
> src/java.base/share/classes/java/lang/runtime/Carrier.java line 166:
>
>> 164: from = longIndex++;
>> 165: filter = DOUBLE_TO_LONG;
>> 166: } else if(ptype == float.class) {
>
> Trivial: space missing after `if`
Updated
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v11]
On Mon, 21 Mar 2022 14:24:30 GMT, Jim Laskey wrote:
>> We propose to provide a runtime anonymous carrier class object generator;
>> java.lang.runtime.Carrier. This generator class is designed to share
>> anonymous classes when shapes are similar. For example, if several clients
>> require objects containing two integer fields, then Carrier will ensure that
>> each client generates carrier objects using the same underlying anonymous
>> class.
>>
>> See JBS for details.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 18 additional commits since
> the last revision:
>
> - Remove LOOKUP static final
> - Merge branch 'master' into 8282798
> - Typos
> - Update Carrier.java
> - Redo API to use list, bring Carrier.component back
> - Clean up API
> - Remove redundant MethodHandle component(MethodType methodType, int i) API
> - Revert to {@return} style comments.
> - Clarify primitive store in array carriers.
> - Use long array for primitives
> - ... and 8 more:
> https://git.openjdk.java.net/jdk/compare/b726367e...a8657bbe
Can I get some CSR reviews - thank you?
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v12]
> We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Fix spacing on if( - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/a8657bbe..7f8de3b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7744=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7744=10-11 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7744.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744 PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v2]
On Tue, 22 Mar 2022 12:08:01 GMT, Fei Yang wrote: >> make/autoconf/libraries.m4 line 152: >> >>> 150: fi >>> 151: >>> 152: # Programs which use C11 or C++11 atomics, like #include , >> >> Use of C++ atomics is not allowed in hotspot code base. See the style guide: >> https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md >> >> That said, I don't see any actual use of C++ atomics. ?? > > I think the old code comment here is a bit too general. It does not mean we > introduce any use of C++ atomics here. > The fact is that RISC-V only has word-sized atomics, it requries libatomic > where other common architectures do not [1]. > So atomic support would require explicit linking against -latomic on RISC-V. > Otherwise we got build errors like: New comment looks good - thanks for clarifying. - PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
Marked as reviewed by dholmes (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
On Mon, Mar 21, 2022 at 8:19 PM Alan Bateman wrote: > Hi Alan, Thanks for looking at this issue. Please find my answers to your questions inline: > On 04/03/2022 10:04, Volker Simonis wrote: > > : > > > > 1. Relax the specification of `InflaterInputStream::read(..)` and > > specifically note in the API documentation that a call to > > `InflaterInputStream::read(byte[] b, int off, int len)` may write more > > than *k* characters into `b` where *k* is the returned number of > > inflated bytes. Notice that there's a precedence for this approach in > > `java.io.ByteArrayInputStream::read(..)` which "*unlike the overridden > > method of InputStream, returns -1 instead of zero if the end of the > > stream has been reached and len==0*". > > > On the surface this is probably okay. It wouldn't really be relaxing the > specification, rather than it would say for a return value n, the bytes > in b[n] to b[len-1] are undefined as the read operate may have clobbered > their original values. Yes, that's exactly what I'd like to propose. > The risk is that it becomes a performance "trick" > to provide a larger buffer. A larger buffer is always beneficial because it saves us from multiple native down-calls to zlib's inflate function. That's already true today, with the current implementation, where ideally the whole inflated content fits into the output buffer such that it can be inflated by a single call to inflate(). The issue I'd like to solve here is really a corner case for the case where the size of the inflated content is `m` but the buffer size is `n` with `n < m`. In that case we need `(m + (n-1)) / n)` calls to inflate and if `(m % n) > 0` the last call will write fewer than ´n` content bytes (i.e. `m % n` bytes) to the output buffer. Only during the last call to inflate() some of the remaining bytes in the buffer after the `m % n` content bytes can be clobbered. Calling that out in the specification won't change the performance benefits of a larger output buffer except for the writing of the 16 (i.e. the size of a vector store) very last inflated bytes. This last write could be potentially optimized with a 16-byte vector store instruction if the output buffer was larger than the deflated content `m`. But that's really a corner case because in reality the output buffer is usually smaller than `m` and the size of the inflated content `m` is usually much, much larger than 16 (so optimizing the output of the last 16 bytes won't have any measurable effects). > That said, I think the main thing we need to > satisfy ourselves on is security. One part of that is whether anything > can be gleaned by reading from the byte array during or after an > inflate. I can't see any security risks here. The optimized vector store instructions are only used to copy repeated content from the history buffer (aka "sliding window") to the output (i.e. inflate LZ77 compressed data). So the "clobber" bytes will always only contain bytes which have already been written before. See [1, 2] for details. [1] https://www.zlib.net/manual.html [2] https://www.euccas.me/zlib/#deflate_sliding_window > The other part is how the implementation behaves when there is > a tampering of the array contents during an inflate. I don't really understand this concern? Do you mean what happens if another thread is changing the content of the output buffer during an inflate? I think such a use case has never been well-defined and amending the specification won't change anything for such a situation. As I've explained before, the extra clobber bytes won't contain any data which is required by the inflate algorithm for its correct operation. It just contains some additional data from the LZ77 history buffer which can be safely overwritten. The optimized inflate implementations are actually constantly doing this until they reach the end of the output buffer or the end of the deflated data (see [3] for a simple example). [3] https://github.com/simonis/zlib-chromium#inflater-improvements-in-zlib-chromium > > -Alan If you don't mind I'll send out a pull request with a draft of the amended API-doc and open a CSR for this issue. Thank you and best regards, Volker
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v2]
On Tue, 22 Mar 2022 04:13:17 GMT, David Holmes wrote: >> Fei Yang has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains two additional commits >> since the last revision: >> >> - Merge remote-tracking branch 'upstream/master' into JDK-8276799 >> - 8276799: Implementation of JEP 422: Linux/RISC-V Port > > make/autoconf/libraries.m4 line 152: > >> 150: fi >> 151: >> 152: # Programs which use C11 or C++11 atomics, like #include , > > Use of C++ atomics is not allowed in hotspot code base. See the style guide: > https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md > > That said, I don't see any actual use of C++ atomics. ?? I think the old code comment here is a bit too general. It does not mean we introduce any use of C++ atomics here. The fact is that RISC-V only has word-sized atomics, it requries libatomic where other common architectures do not [1]. So atomic support would require explicit linking against -latomic on RISC-V. Otherwise we got build errors like: - PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v2]
On Tue, 22 Mar 2022 05:12:46 GMT, David Holmes wrote: > Hi, > > I've looked at everything that is not a RISC-V specific file, except for the > C1 changes as the compiler folk will need to approve those. > > Some copyrights will need updating to 2022 on the Oracle copyright line > please. Hi David, I have pushed one more commit updating the Oralce copyright line for existing files touched. Thanks for looking at this. - PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v2]
On Tue, 22 Mar 2022 03:31:16 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains two additional commits since
> the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into JDK-8276799
> - 8276799: Implementation of JEP 422: Linux/RISC-V Port
> Build changes look good. I can't say anything about the rest of the code.
>
> /reviewers 3
Thanks again for looking at the build changes :-)
-
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
> This PR implements JEP 422: Linux/RISC-V Port [1].
> The PR starts as a squashed merge of the
> https://openjdk.java.net/projects/riscv-port branch.
>
> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are also
> carried out regularly. So it should be good enough to run most Java programs.
>
> [1] https://openjdk.java.net/jeps/422
Fei Yang has updated the pull request incrementally with one additional commit
since the last revision:
Address review comments
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/6294/files
- new: https://git.openjdk.java.net/jdk/pull/6294/files/a144cd20..b7a31729
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=6294=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=6294=01-02
Stats: 44 lines in 41 files changed: 0 ins; 1 del; 43 mod
Patch: https://git.openjdk.java.net/jdk/pull/6294.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6294/head:pull/6294
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v11]
On Mon, 21 Mar 2022 14:24:30 GMT, Jim Laskey wrote:
>> We propose to provide a runtime anonymous carrier class object generator;
>> java.lang.runtime.Carrier. This generator class is designed to share
>> anonymous classes when shapes are similar. For example, if several clients
>> require objects containing two integer fields, then Carrier will ensure that
>> each client generates carrier objects using the same underlying anonymous
>> class.
>>
>> See JBS for details.
>
> Jim Laskey has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 18 additional commits since
> the last revision:
>
> - Remove LOOKUP static final
> - Merge branch 'master' into 8282798
> - Typos
> - Update Carrier.java
> - Redo API to use list, bring Carrier.component back
> - Clean up API
> - Remove redundant MethodHandle component(MethodType methodType, int i) API
> - Revert to {@return} style comments.
> - Clarify primitive store in array carriers.
> - Use long array for primitives
> - ... and 8 more:
> https://git.openjdk.java.net/jdk/compare/eace55cb...a8657bbe
src/java.base/share/classes/java/lang/runtime/Carrier.java line 166:
> 164: from = longIndex++;
> 165: filter = DOUBLE_TO_LONG;
> 166: } else if(ptype == float.class) {
Trivial: space missing after `if`
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings [v3]
On Tue, 22 Mar 2022 06:54:35 GMT, Xin Liu wrote:
>> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1008:
>>
>>> 1006: this.count = newCount;
>>> 1007: putStringAt(start, str);
>>> 1008: if (end - start > 0) {
>>
>> regardless of value of `end - start` you could also skip setting
>> `maybeLatin1 = true` if:
>> - `str.coder() == UTF16`
>> - `this.coder == LATIN1`
>
> hi, @cl4es,
> you are correct for this and the comment at setCharAt(), but I don't think
> it's necessary to check all cases. this attribute is just like a hint. if
> this.coder == LATIN1, that it doesn't matter if maybeLatin1 is true.
>
> if our attitude is checking all cases, it will become too complex and
> error-prone. deleteCharAt() and setLength() also need to check. if so, it
> will pollute code more. I incline to set this attribute conservatively in all
> deleting methods.
Right, we don't need to check every case and I agree with favoring simplicity
at the expense of some false positives. Perhaps the `if (end - start > 0) {`
test here isn't pulling its weight either and we should just unconditionally
set `maybeLatin1 = true` even if we're not actually replacing anything (which
is very much a corner-case).
-
PR: https://git.openjdk.java.net/jdk/pull/7671
Re: RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings [v4]
On Tue, 22 Mar 2022 08:05:35 GMT, Xin Liu wrote: >> If AbstractStringBuilder only grow, the inflated value which has been >> encoded in UTF16 can't be compressed. >> toString() can skip compression in this case. This can save an >> ArrayAllocation in StringUTF16::compress(). >> >> java.io.BufferedRead::readLine() is a case that StringBuilder grows only. >> >> In microbench, we expect to see that allocation/op reduces 20%. The initial >> capacity of StringBuilder is S in bytes. When it encounters the 1st >> character that can't be encoded in LATIN1, it inflates and allocate a new >> array of 2*S. `toString()` will try to compress that value so it need to >> allocate S bytes. The last step allocates 2*S bytes because it has to copy >> the string. so it requires to allocate 5 * S bytes in total. By skipping >> the failed compression, it only allocates 4 * S bytes. that's 20%. In real >> execution, we observe 16% allocation reduction, tracked by JMH GC profiler >> `gc.alloc.rate.norm `. I think it's because HotSpot can't track all >> allocations. >> >> Not only allocation drops, the runtime performance(ns/op) also increases >> from 3.34% to 18.91%. >> >> Before: >> >> $$make test >> TEST="micro:org.openjdk.bench.java.lang.StringBuilders.toStringWithMixedChars" >> MICRO="OPTIONS=-prof gc -gc true -o before.log -jvm >> $HOME/Devel/jdk_baseline/bin/java" >> >> Benchmark >> (MIXED_SIZE) Mode Cnt Score Error Units >> StringBuilders.toStringWithMixedChars >> 128 avgt 15 649.846 ± 76.291 ns/op >> StringBuilders.toStringWithMixedChars:·gc.alloc.rate >> 128 avgt 15 872.855 ± 128.259 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm >> 128 avgt 15 880.121 ± 0.050B/op >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space >> 128 avgt 15 707.730 ± 194.421 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm >> 128 avgt 15 706.602 ± 94.504B/op >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space >> 128 avgt 15 0.001 ± 0.002 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm >> 128 avgt 15 0.001 ± 0.001B/op >> StringBuilders.toStringWithMixedChars:·gc.count >> 128 avgt 15 113.000counts >> StringBuilders.toStringWithMixedChars:·gc.time >> 128 avgt 1585.000ms >> StringBuilders.toStringWithMixedChars >> 256 avgt 15 1316.652 ± 112.771 ns/op >> StringBuilders.toStringWithMixedChars:·gc.alloc.rate >> 256 avgt 15 800.864 ± 76.869 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm >> 256 avgt 15 1648.288 ± 0.162B/op >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space >> 256 avgt 15 599.736 ± 174.001 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm >> 256 avgt 15 1229.669 ± 318.518B/op >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space >> 256 avgt 15 0.001 ± 0.001 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm >> 256 avgt 15 0.001 ± 0.002B/op >> StringBuilders.toStringWithMixedChars:·gc.count >> 256 avgt 15 133.000counts >> StringBuilders.toStringWithMixedChars:·gc.time >> 256 avgt 1592.000ms >> StringBuilders.toStringWithMixedChars >>1024 avgt 15 5204.303 ± 418.115 ns/op >> StringBuilders.toStringWithMixedChars:·gc.alloc.rate >>1024 avgt 15 768.730 ± 72.945 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm >>1024 avgt 15 6256.844 ± 0.358B/op >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space >>1024 avgt 15 655.852 ± 121.602 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm >>1024 avgt 15 5315.265 ± 578.878B/op >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space >>1024 avgt 15 0.002 ± 0.002 MB/sec >> StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm >>1024 avgt 15 0.014 ± 0.011B/op >> StringBuilders.toStringWithMixedChars:·gc.count >>1024 avgt 1596.000counts >> StringBuilders.toStringWithMixedChars:·gc.time
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v2]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments * Use `new` instead of `fresh` * Drop use of `new` where caching might be used * Remove unused imports * Add static imports to make code more succint * Fix other typos - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/8e6017dc..6bb1b5c9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=00-01 Stats: 83 lines in 10 files changed: 2 ins; 7 del; 74 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Sun, 20 Mar 2022 21:31:15 GMT, Joe Darcy wrote: > Small refactoring to use sealed classes in the MethodHandle implementation > hierarchy. > > DelegatingMethodHandle is non-sealed rather than sealed since it has two > subclasses, one defined as a nested class and only a separate type in the > same package. The permits clause does not allow listed those two kinds of > subclasses. > > Please also review the corresponding CSR > https://bugs.openjdk.java.net/browse/JDK-8283434 But the private inner classes are visible to the compiled `PermittedSubclasses` class attribute (otherwise it wouldn’t be possible for private inner classes to extend a sealed outer class with an implicit `permits` clause). --- Technically, `private` means accessible only within the current nest. - PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: 8282162: [vector] Optimize vector negation API [v2]
> The current vector `"NEG"` is implemented with substraction a vector by zero > in case the architecture does not support the negation instruction. And to > fit the predicate feature for architectures that support it, the masked > vector `"NEG" ` is implemented with pattern `"v.not(m).add(1, m)"`. They both > can be optimized to a single negation instruction for ARM SVE. > And so does the non-masked "NEG" for NEON. Besides, implementing the masked > "NEG" with substraction for architectures that support neither negation > instruction nor predicate feature can also save several instructions than the > current pattern. > > To optimize the VectorAPI negation, this patch moves the implementation from > Java side to hotspot. The compiler will generate different nodes according to > the architecture: > - Generate the (predicated) negation node if architecture supports it, > otherwise, generate "`zero.sub(v)`" pattern for non-masked operation. > - Generate `"zero.sub(v, m)"` for masked operation if the architecture does > not have predicate feature, otherwise generate the original pattern > `"v.xor(-1, m).add(1, m)"`. > > So with this patch, the following transformations are applied: > > For non-masked negation with NEON: > > moviv16.4s, #0x0 > sub v17.4s, v16.4s, v17.4s ==> neg v17.4s, v17.4s > > and with SVE: > > mov z16.s, #0 > sub z18.s, z16.s, z17.s ==> neg z16.s, p7/m, z16.s > > For masked negation with NEON: > > moviv17.4s, #0x1 > mvn v19.16b, v18.16b > mov v20.16b, v16.16b ==> neg v18.4s, v17.4s > bsl v20.16b, v19.16b, v18.16b bsl v19.16b, v18.16b, v17.16b > add v19.4s, v20.4s, v17.4s > mov v18.16b, v16.16b > bsl v18.16b, v19.16b, v20.16b > > and with SVE: > > mov z16.s, #-1 > mov z17.s, #1==> neg z16.s, p0/m, z16.s > eor z18.s, p0/m, z18.s, z16.s > add z18.s, p0/m, z18.s, z17.s > > Here are the performance gains for benchmarks (see [1][2]) on ARM and x86 > machines(note that the non-masked negation benchmarks do not have any > improvement on X86 since no instructions are changed): > > NEON: > BenchmarkGain > Byte128Vector.NEG1.029 > Byte128Vector.NEGMasked 1.757 > Short128Vector.NEG 1.041 > Short128Vector.NEGMasked 1.659 > Int128Vector.NEG 1.005 > Int128Vector.NEGMasked 1.513 > Long128Vector.NEG1.003 > Long128Vector.NEGMasked 1.878 > > SVE with 512-bits: > BenchmarkGain > ByteMaxVector.NEG1.10 > ByteMaxVector.NEGMasked 1.165 > ShortMaxVector.NEG 1.056 > ShortMaxVector.NEGMasked 1.195 > IntMaxVector.NEG 1.002 > IntMaxVector.NEGMasked 1.239 > LongMaxVector.NEG1.031 > LongMaxVector.NEGMasked 1.191 > > X86 (non AVX-512): > BenchmarkGain > ByteMaxVector.NEGMasked 1.254 > ShortMaxVector.NEGMasked 1.359 > IntMaxVector.NEGMasked 1.431 > LongMaxVector.NEGMasked 1.989 > > [1] > https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1881 > [2] > https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1896 Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision: Add a superclass for vector negation - Changes: - all: https://git.openjdk.java.net/jdk/pull/7782/files - new: https://git.openjdk.java.net/jdk/pull/7782/files/828866f8..97c8119a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7782=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7782=00-01 Stats: 64 lines in 4 files changed: 16 ins; 13 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/7782.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7782/head:pull/7782 PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API
On Sat, 19 Mar 2022 03:11:12 GMT, Jie Fu wrote:
>>> Note that in terms of Java semantics, negation of floating point values
>>> needs to be implemented as subtraction from negative zero rather than
>>> positive zero:
>>>
>>> double negate(double arg) {return -0.0 - arg; }
>>>
>>> This is to handle signed zeros correctly.
>>
>> Hi @jddarcy ,thanks for looking at this PR and thanks for the notes on the
>> floating point negation! Yeah, this really makes sense to me. Kindly note
>> that this patch didn't touch the negation of the floating point values. For
>> Vector API, the vector floating point negation has been intrinsified to
>> `NegVF/D` node by compiler that we directly generate the negation
>> instructions for them. Thanks!
>
>> Note that in terms of Java semantics, negation of floating point values
>> needs to be implemented as subtraction from negative zero rather than
>> positive zero:
>>
>> double negate(double arg) {return -0.0 - arg; }
>>
>> This is to handle signed zeros correctly.
>
> This seems easy to be broken by an opt enhancement.
> Just wondering do we have a jtreg test for this point? @jddarcy
> Thanks.
Hi @DamonFool , thanks for your review! All the comments have been addressed.
Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings [v4]
> If AbstractStringBuilder only grow, the inflated value which has been encoded > in UTF16 can't be compressed. > toString() can skip compression in this case. This can save an > ArrayAllocation in StringUTF16::compress(). > > java.io.BufferedRead::readLine() is a case that StringBuilder grows only. > > In microbench, we expect to see that allocation/op reduces 20%. The initial > capacity of StringBuilder is S in bytes. When it encounters the 1st character > that can't be encoded in LATIN1, it inflates and allocate a new array of 2*S. > `toString()` will try to compress that value so it need to allocate S bytes. > The last step allocates 2*S bytes because it has to copy the string. so it > requires to allocate 5 * S bytes in total. By skipping the failed > compression, it only allocates 4 * S bytes. that's 20%. In real execution, > we observe 16% allocation reduction, tracked by JMH GC profiler > `gc.alloc.rate.norm `. I think it's because HotSpot can't track all > allocations. > > Not only allocation drops, the runtime performance(ns/op) also increases from > 3.34% to 18.91%. > > Before: > > $$make test > TEST="micro:org.openjdk.bench.java.lang.StringBuilders.toStringWithMixedChars" > MICRO="OPTIONS=-prof gc -gc true -o before.log -jvm > $HOME/Devel/jdk_baseline/bin/java" > > Benchmark > (MIXED_SIZE) Mode Cnt Score Error Units > StringBuilders.toStringWithMixedChars >128 avgt 15 649.846 ± 76.291 ns/op > StringBuilders.toStringWithMixedChars:·gc.alloc.rate >128 avgt 15 872.855 ± 128.259 MB/sec > StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm >128 avgt 15 880.121 ± 0.050B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space >128 avgt 15 707.730 ± 194.421 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm >128 avgt 15 706.602 ± 94.504B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space >128 avgt 15 0.001 ± 0.002 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm >128 avgt 15 0.001 ± 0.001B/op > StringBuilders.toStringWithMixedChars:·gc.count >128 avgt 15 113.000counts > StringBuilders.toStringWithMixedChars:·gc.time >128 avgt 1585.000ms > StringBuilders.toStringWithMixedChars >256 avgt 15 1316.652 ± 112.771 ns/op > StringBuilders.toStringWithMixedChars:·gc.alloc.rate >256 avgt 15 800.864 ± 76.869 MB/sec > StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm >256 avgt 15 1648.288 ± 0.162B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space >256 avgt 15 599.736 ± 174.001 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm >256 avgt 15 1229.669 ± 318.518B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space >256 avgt 15 0.001 ± 0.001 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm >256 avgt 15 0.001 ± 0.002B/op > StringBuilders.toStringWithMixedChars:·gc.count >256 avgt 15 133.000counts > StringBuilders.toStringWithMixedChars:·gc.time >256 avgt 1592.000ms > StringBuilders.toStringWithMixedChars > 1024 avgt 15 5204.303 ± 418.115 ns/op > StringBuilders.toStringWithMixedChars:·gc.alloc.rate > 1024 avgt 15 768.730 ± 72.945 MB/sec > StringBuilders.toStringWithMixedChars:·gc.alloc.rate.norm > 1024 avgt 15 6256.844 ± 0.358B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space > 1024 avgt 15 655.852 ± 121.602 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Eden_Space.norm > 1024 avgt 15 5315.265 ± 578.878B/op > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space > 1024 avgt 15 0.002 ± 0.002 MB/sec > StringBuilders.toStringWithMixedChars:·gc.churn.G1_Survivor_Space.norm > 1024 avgt 15 0.014 ± 0.011B/op > StringBuilders.toStringWithMixedChars:·gc.count > 1024 avgt 1596.000counts > StringBuilders.toStringWithMixedChars:·gc.time > 1024 avgt 1586.000ms > > > After > > $make test >
Re: RFR: 8279508: Auto-vectorize Math.round API [v18]
On Fri, 18 Mar 2022 20:19:08 GMT, Jatin Bhateja wrote: >> Summary of changes: >> - Intrinsify Math.round(float) and Math.round(double) APIs. >> - Extend auto-vectorizer to infer vector operations on encountering scalar >> IR nodes for above intrinsics. >> - Test creation using new IR testing framework. >> >> Following are the performance number of a JMH micro included with the patch >> >> Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) >> >> >> Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain >> ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio >> -- | -- | -- | -- | -- | -- | -- | -- >> FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | >> 510.36 | 548.39 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | >> 293.48 | 274.01 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | >> 751.83 | 2274.13 | 3.02 >> FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | >> 388.52 | 1334.18 | 3.43 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 22 commits: > > - 8279508: Using an explicit scratch register since rscratch1 is bound to > r10 and its usage is transparent to compiler. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8279508 > - 8279508: Windows build failure fix. > - 8279508: Styling comments resolved. > - 8279508: Creating separate test for round double under feature check. > - 8279508: Reducing the invocation count and compile thresholds for > RoundTests.java. > - 8279508: Review comments resolution. > - 8279508: Preventing domain switch-over penalty for Math.round(float) and > constraining unrolling to prevent code bloating. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8279508 > - 8279508: Removing +LogCompilation flag. > - ... and 12 more: > https://git.openjdk.java.net/jdk/compare/ff0b0927...c17440cf Sure, I'll re-run testing and report back. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: Unused paramter 'boolean newln' in java.lang.VersionProps#print(boolean err, boolean newln)
I mean _inside method body_ itself.It's only declared, but no code in the method refers to this parameter. Andrey Turbanov вт, 22 мар. 2022 г. в 01:37, Mandy Chung : > > VersionProps::print(boolean err, boolean newln) is called by > VersionProps::println(boolean) and VersionProps::print(boolean) which is > called from the launcher java -version and -showversion option [1]. > > Mandy > [1] > https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L1804 > > On 3/19/22 11:20 AM, Andrey Turbanov wrote: > > Hello. > I found a suspicious method java.lang.VersionProps#print with unused > parameter 'boolean newln'. > This class is generated from template - > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/VersionProps.java.template#L203 > It's unused since integration of 'JDK-8169069 Module system > implementation refresh (11/2016)' - > https://github.com/openjdk/jdk/commit/fbe85300bfcc69cb4dd56e4df33ceea632366283 > > > > Andrey Turbanov > >
Re: RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings [v3]
On Tue, 15 Mar 2022 23:25:17 GMT, Claes Redestad wrote:
>> Xin Liu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Change growOnly to maybeLatin.
>>
>> This patch also copys over the attribute from the other
>> AbstractStringBuilder.
>> Add a unit test to cover methods which cause maybeLatin1 becomes true.
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1008:
>
>> 1006: this.count = newCount;
>> 1007: putStringAt(start, str);
>> 1008: if (end - start > 0) {
>
> regardless of value of `end - start` you could also skip setting `maybeLatin1
> = true` if:
> - `str.coder() == UTF16`
> - `this.coder == LATIN1`
hi, @cl4es,
you are correct for this and the comment at setCharAt(), but I don't think it's
necessary to check all cases. this attribute is just like a hint. if this.coder
== LATIN1, that it doesn't matter if maybeLatin1 is true.
if our attitude is checking all cases, it will become too complex and
error-prone. deleteCharAt() and setLength() also need to check. if so, it will
pollute code more. I incline to set this attribute conservatively in all
deleting methods.
-
PR: https://git.openjdk.java.net/jdk/pull/7671
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Tue, 22 Mar 2022 04:38:15 GMT, ExE Boss wrote:
> javac should be changed to allow fully qualified access to private inner
> classes in the permits clause of an enclosing class.
This is not how private works, private means accessible between the '{' and the
'}' of the class.
The permits clause is declared outside of the '{' and the '}' of the class,
thus a private member class is not visible from the permits clause.
-
PR: https://git.openjdk.java.net/jdk/pull/7881
