Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]
On Fri, 27 Jan 2023 13:53:45 GMT, Glavo wrote: >> I checked the `java.base` module, and all the `Collection#toArray()` method >> of collections be implemented correctly. >> >> Their return values can be trusted, so many unnecessary array duplication >> can be eliminated. > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > Collections.isTrustedCollection What about creating a public `Collections.toArray(Collection c)` method that would return `c.toArray` for trusted collections, otherwise return a copy? This would allow any code to get a trustable array, while avoiding the copy when possible. Even if this method was packaged-scoped, it might be a more concise way to access this capability in most cases. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8299807: newStringNoRepl should avoid copying arrays for ASCII compatible charsets
On Fri, 27 Jan 2023 16:04:41 GMT, Roger Riggs wrote:
>> This is the javadoc of `JavaLangAccess::newStringNoRepl`:
>>
>>
>> /**
>> * Constructs a new {@code String} by decoding the specified subarray of
>> * bytes using the specified {@linkplain java.nio.charset.Charset
>> charset}.
>> *
>> * The caller of this method shall relinquish and transfer the ownership
>> of
>> * the byte array to the callee since the later will not make a copy.
>> *
>> * @param bytes the byte array source
>> * @param cs the Charset
>> * @return the newly created string
>> * @throws CharacterCodingException for malformed or unmappable bytes
>> */
>>
>>
>> It is recorded in the document that it should be able to directly construct
>> strings with parameter byte array to reduce array allocation.
>>
>> However, at present, `newStringNoRepl` always copies arrays for UTF-8 or
>> other ASCII compatible charsets.
>>
>> This PR fixes this problem.
>
> It seems odd that the benchmark seems slower for smaller files; can you
> suggest why that might be?
> I'd expect the size distribution for Files.readString to be biased toward the
> smaller files.
> Can you repeat the benchmark using the default file system. OS file caching
> should eliminate the disk speed effects.
@RogerRiggs
I rerun benchmark based on the default file system, and the test file size is
between 0 and 32KiB.
The throughput of reading ASCII files as UTF-8:
The throughput of reading ASCII files as GBK:
The performance has been slightly improved, and there is no performance
degradation.
For UTF-8 and GBK files with non-ASCII characters, the throughput fluctuates by
no more than 4%.
Test code and original results:
https://gist.github.com/Glavo/f3d2060d0bd13cd0ce2add70e6060ea0?permalink_comment_id=4451350#gistcomment-4451350
> It seems odd that the benchmark seems slower for smaller files; can you
> suggest why that might be?
The most likely reason is the cost of the newly added if judgment in
newStringUTF8NoRepl.
I don't think this is an important issue, because when it comes to actual I/O
operations, its impact is negligible.
The main purpose of this PR is to eliminate unnecessary temporary memory
allocation, thus reducing GC pressure. The change in throughput is only a
by-product.
-
PR: https://git.openjdk.org/jdk/pull/12119
Re: RFR: 8299807: newStringNoRepl should avoid copying arrays for ASCII compatible charsets
On Fri, 20 Jan 2023 16:47:27 GMT, Glavo wrote:
> This is the javadoc of `JavaLangAccess::newStringNoRepl`:
>
>
> /**
> * Constructs a new {@code String} by decoding the specified subarray of
> * bytes using the specified {@linkplain java.nio.charset.Charset
> charset}.
> *
> * The caller of this method shall relinquish and transfer the ownership
> of
> * the byte array to the callee since the later will not make a copy.
> *
> * @param bytes the byte array source
> * @param cs the Charset
> * @return the newly created string
> * @throws CharacterCodingException for malformed or unmappable bytes
> */
>
>
> It is recorded in the document that it should be able to directly construct
> strings with parameter byte array to reduce array allocation.
>
> However, at present, `newStringNoRepl` always copies arrays for UTF-8 or
> other ASCII compatible charsets.
>
> This PR fixes this problem.
It seems odd that the benchmark seems slower for smaller files; can you suggest
why that might be?
I'd expect the size distribution for Files.readString to be biased toward the
smaller files.
Can you repeat the benchmark using the default file system. OS file caching
should eliminate the disk speed effects.
-
PR: https://git.openjdk.org/jdk/pull/12119
Re: RFR: 8299807: newStringNoRepl should avoid copying arrays for ASCII compatible charsets
On Fri, 20 Jan 2023 16:47:27 GMT, Glavo wrote:
> This is the javadoc of `JavaLangAccess::newStringNoRepl`:
>
>
> /**
> * Constructs a new {@code String} by decoding the specified subarray of
> * bytes using the specified {@linkplain java.nio.charset.Charset
> charset}.
> *
> * The caller of this method shall relinquish and transfer the ownership
> of
> * the byte array to the callee since the later will not make a copy.
> *
> * @param bytes the byte array source
> * @param cs the Charset
> * @return the newly created string
> * @throws CharacterCodingException for malformed or unmappable bytes
> */
>
>
> It is recorded in the document that it should be able to directly construct
> strings with parameter byte array to reduce array allocation.
>
> However, at present, `newStringNoRepl` always copies arrays for UTF-8 or
> other ASCII compatible charsets.
>
> This PR fixes this problem.
I ran the tier1 and tier2 tests, and there were no new errors.
The only use case affected is `Files.readString`. I tested the performance of
`readString` based on the memory file system.
baseline:
Benchmark (length) Mode CntScore Error Units
NoRepl.testReadAscii 0 thrpt5 5049760.744 ± 3563.324 ops/s
NoRepl.testReadAscii1024 thrpt5 3523083.785 ± 23747.078 ops/s
NoRepl.testReadAscii8192 thrpt5 2415952.140 ± 85884.289 ops/s
NoRepl.testReadAscii 1048576 thrpt532425.563 ± 284.121 ops/s
NoRepl.testReadAscii33554432 thrpt5 872.492 ± 4.311 ops/s
NoRepl.testReadAscii 268435456 thrpt5 58.736 ± 0.224 ops/s
NoRepl.testReadAsciiAsGBK 0 thrpt5 4229547.832 ± 10997.381 ops/s
NoRepl.testReadAsciiAsGBK 1024 thrpt5 3409472.580 ± 819.566 ops/s
NoRepl.testReadAsciiAsGBK 8192 thrpt5 2179865.886 ± 16606.862 ops/s
NoRepl.testReadAsciiAsGBK1048576 thrpt532962.429 ± 249.385 ops/s
NoRepl.testReadAsciiAsGBK 33554432 thrpt5 871.810 ± 1.812 ops/s
NoRepl.testReadAsciiAsGBK 268435456 thrpt5 59.131 ± 0.172 ops/s
NoRepl.testReadGBK 0 thrpt5 4657464.074 ± 51849.667 ops/s
NoRepl.testReadGBK 1024 thrpt5 1199083.242 ± 8653.846 ops/s
NoRepl.testReadGBK 8192 thrpt575823.949 ±46.493 ops/s
NoRepl.testReadGBK 1048576 thrpt5 675.214 ± 1.729 ops/s
NoRepl.testReadGBK 33554432 thrpt5 20.972 ± 2.280 ops/s
NoRepl.testReadGBK 268435456 thrpt52.585 ± 0.394 ops/s
NoRepl.testReadUTF80 thrpt5 5222403.558 ± 44728.740 ops/s
NoRepl.testReadUTF8 1024 thrpt5 1417472.161 ± 23776.534 ops/s
NoRepl.testReadUTF8 8192 thrpt5 185714.265 ± 2096.328 ops/s
NoRepl.testReadUTF8 1048576 thrpt5 1318.763 ±16.051 ops/s
NoRepl.testReadUTF8 33554432 thrpt5 30.663 ± 0.114 ops/s
NoRepl.testReadUTF8268435456 thrpt53.782 ± 0.041 ops/s
This PR:
Benchmark (length) Mode CntScore Error Units
NoRepl.testReadAscii 0 thrpt5 5084610.141 ± 37449.826 ops/s
NoRepl.testReadAscii1024 thrpt5 3425713.961 ± 24060.542 ops/s
NoRepl.testReadAscii8192 thrpt5 2765684.248 ± 20586.103 ops/s
NoRepl.testReadAscii 1048576 thrpt548074.603 ± 371.213 ops/s
NoRepl.testReadAscii33554432 thrpt5 1167.878 ±14.427 ops/s
NoRepl.testReadAscii 268435456 thrpt5 71.028 ± 0.439 ops/s
NoRepl.testReadAsciiAsGBK 0 thrpt5 4783174.805 ± 9789.109 ops/s
NoRepl.testReadAsciiAsGBK 1024 thrpt5 3518265.840 ± 18467.577 ops/s
NoRepl.testReadAsciiAsGBK 8192 thrpt5 2775108.822 ± 19282.776 ops/s
NoRepl.testReadAsciiAsGBK1048576 thrpt546956.963 ± 147.593 ops/s
NoRepl.testReadAsciiAsGBK 33554432 thrpt5 1165.036 ±10.032 ops/s
NoRepl.testReadAsciiAsGBK 268435456 thrpt5 70.878 ± 0.191 ops/s
NoRepl.testReadGBK 0 thrpt5 4910043.054 ± 27295.344 ops/s
NoRepl.testReadGBK 1024 thrpt5 1177675.970 ± 15573.239 ops/s
NoRepl.testReadGBK 8192 thrpt575417.479 ± 233.957 ops/s
NoRepl.testReadGBK 1048576 thrpt5 674.620 ± 5.856 ops/s
NoRepl.testReadGBK 33554432 thrpt5 19.899 ± 1.504 ops/s
NoRepl.testReadGBK 268435456 thrpt52.705 ± 0.002 ops/s
NoRepl.testReadUTF80 thrpt5 4851516.950 ± 9237.743 ops/s
NoRepl.testReadUTF8 1024 thrpt5 1332016.420 ± 9570.465 ops/s
NoRepl.testReadUTF8 8192 thrpt5 184177.766 ± 4662.562 ops/s
RFR: 8299807: newStringNoRepl should avoid copying arrays for ASCII compatible charsets
This is the javadoc of `JavaLangAccess::newStringNoRepl`:
/**
* Constructs a new {@code String} by decoding the specified subarray of
* bytes using the specified {@linkplain java.nio.charset.Charset charset}.
*
* The caller of this method shall relinquish and transfer the ownership of
* the byte array to the callee since the later will not make a copy.
*
* @param bytes the byte array source
* @param cs the Charset
* @return the newly created string
* @throws CharacterCodingException for malformed or unmappable bytes
*/
It is recorded in the document that it should be able to directly construct
strings with parameter byte array to reduce array allocation.
However, at present, `newStringNoRepl` always copies arrays for UTF-8 or other
ASCII compatible charsets.
This PR fixes this problem.
-
Commit messages:
- newStringNoRepl should avoid copying arrays for ASCII compatible charsets
Changes: https://git.openjdk.org/jdk/pull/12119/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=12119=00
Issue: https://bugs.openjdk.org/browse/JDK-8299807
Stats: 7 lines in 2 files changed: 4 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/12119.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12119/head:pull/12119
PR: https://git.openjdk.org/jdk/pull/12119
Why does ZipFile.Source.readFullyAt read in chunks?
Hi,
ZipFile.Source.readFullyAt caps its calls to RandomAccessFile.readFully to
a maximum of 8192 bytes per call, like this:
int N = len;
> while (N > 0) {
> int n = Math.min(BUF_SIZE, N);
> zfile.readFully(buf, off, n);
> off += n;
> N -= n;
> }
I'm observing a ~10% speedup of the ZipFileOpen micro when I replace the
above with simply:
zfile.seek(pos);
> zfile.readFully(buf, off, len);
Does anyone know why readFullyAt is implemented this way? Is it trying to
limit blocking time somehow? Is there some hidden buffering deeper down I'm
not seeing?
If this is safe to remove, I'm happy to submit a PR. But code like this is
usually there for a reason..?
Cheers,
Eiirk.
Integrated: 8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories
On Thu, 12 Jan 2023 18:44:26 GMT, Eirik Bjorsnos wrote: > Some call sites of SignatureFileVerifier.isBlockOrSF fails to check that > files reside in META-INF directly, and not in a subdirectory of META-INF. > > The mentioned call sites needs updates to check and ignore such files. > > A new test IgnoreUnrelatedSignatureFiles is added which verifies that [*.SF, > *.RSA] files in META-INF/ subdirectories are indeed ignored. This pull request has now been integrated. Changeset: 5dfc4ec7 Author:Eirik Bjorsnos Committer: Weijun Wang URL: https://git.openjdk.org/jdk/commit/5dfc4ec7d94af9fe39fdee9d83b06101b827a3c6 Stats: 429 lines in 6 files changed: 405 ins; 8 del; 16 mod 8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories Reviewed-by: weijun - PR: https://git.openjdk.org/jdk/pull/11976
Integrated: Merge jdk20
On Fri, 27 Jan 2023 21:00:03 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 20 -> JDK 21 This pull request has now been integrated. Changeset: 5c59de52 Author:Jesper Wilhelmsson URL: https://git.openjdk.org/jdk/commit/5c59de52a31da937663ad2cef055213489b0516e Stats: 1078 lines in 85 files changed: 220 ins; 114 del; 744 mod Merge - PR: https://git.openjdk.org/jdk/pull/12267
Re: RFR: 8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories [v15]
On Fri, 27 Jan 2023 22:16:18 GMT, Weijun Wang wrote: > Maybe we can rename `ZipFile::isSignatureRelated` to `ZipFile::isBlockOrSF` > as well? The term "signature related" seems to be used quite extensively around ZipFile and also in JavaUtilZipFileAccess. Semantics are very similar, but not exactly equal. Because of this, I'm a bit reluctant to rename it. > I'll approved this PR. Thanks. Thank for your help and guidance in reviewing this PR! - PR: https://git.openjdk.org/jdk/pull/11976
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter [v2]
On Fri, 27 Jan 2023 22:10:38 GMT, Roger Riggs wrote: >> The only changes I made myself in the test files are in >> Basic-X.java.template (including the copyright year). The other files were >> generated by a script, which happens to also change the copyright year for >> otherwise unmodified files. >> >> If you agree, I'll happily remove the trivial copyright year changes and >> only retain those files containing more substantial modifications. > > Fine with me, I figured it was the script that changed the copyright and > otherwise generated the same as before. tnx Otherwise untouched files are now reverted back with previous copyright year - PR: https://git.openjdk.org/jdk/pull/12259
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter [v3]
> Align `double` and `float` decimal conversions in `java.util.Formatter` with > the algorithm used in `Double.toString(double)`. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter Reverted back copyright year of otherwise untouched files. - Changes: - all: https://git.openjdk.org/jdk/pull/12259/files - new: https://git.openjdk.org/jdk/pull/12259/files/d42fbd03..6f026d45 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12259=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12259=01-02 Stats: 14 lines in 14 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/12259.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12259/head:pull/12259 PR: https://git.openjdk.org/jdk/pull/12259
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v6]
On Fri, 27 Jan 2023 21:36:29 GMT, Claes Redestad wrote: >> Scott Gibbons 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 13 additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into Base64-AVX2 >> - Merge branch 'openjdk:master' into Base64-AVX2 >> - Merge branch 'openjdk:master' into Base64-AVX2 >> - Merge branch 'Base64-AVX2' of https://github.com/asgibbons/jdk into >> Base64-AVX2 >> - Merge branch 'openjdk:master' into Base64-AVX2 >> - Address review comment >> - Remove whitespace >> - Fix wrong register usage >> - Working version of Base64 decode with AVX2 (4x perf improvement). No URL >> support >> - Merge branch 'Base64-AVX2' of https://github.com/asgibbons/jdk into >> Base64-AVX2 >> - ... and 3 more: https://git.openjdk.org/jdk/compare/50dc1196...3e66f7be > > src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2643: > >> 2641: // Handle isURL / MIME?!?! r12 is used for length calculation >> (from out) >> 2642: // >> 2643: // rbx is out, r12 is saved out, rdx is size, rsi is src > > It seems that on windows `r12` is in use, see line 2323. GHA seem to be > having some trouble finishing Windows testing on time - could there be some > issue here? Nevermind, you're not using r12 and GHA Windows testing is green now. - PR: https://git.openjdk.org/jdk/pull/12126
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v6]
On Fri, 27 Jan 2023 18:31:50 GMT, Scott Gibbons wrote:
>> Added code for Base64 acceleration (encode and decode) which will accelerate
>> ~4x for AVX2 platforms.
>>
>> Encode performance:
>> **Old:**
>>
>> Benchmark (maxNumBytes) Mode Cnt Score Error
>> Units
>> Base64Encode.testBase64Encode 1024 thrpt3 4309.439 ± 2.632
>> ops/ms
>>
>>
>> **New:**
>>
>> Benchmark (maxNumBytes) Mode Cnt Score
>> Error Units
>> Base64Encode.testBase64Encode 1024 thrpt3 24211.397 ±
>> 102.026 ops/ms
>>
>>
>> Decode performance:
>> **Old:**
>>
>> Benchmark (errorIndex) (lineSize) (maxNumBytes)
>> Mode Cnt ScoreError Units
>> Base64Decode.testBase64Decode 144 4 1024
>> thrpt3 3961.768 ± 93.409 ops/ms
>>
>> **New:**
>> Benchmark (errorIndex) (lineSize) (maxNumBytes)
>> Mode Cnt ScoreError Units
>> Base64Decode.testBase64Decode 144 4 1024
>> thrpt3 14738.051 ± 24.383 ops/ms
>
> Scott Gibbons has updated the pull request 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 13 additional
> commits since the last revision:
>
> - Merge branch 'openjdk:master' into Base64-AVX2
> - Merge branch 'openjdk:master' into Base64-AVX2
> - Merge branch 'openjdk:master' into Base64-AVX2
> - Merge branch 'Base64-AVX2' of https://github.com/asgibbons/jdk into
> Base64-AVX2
> - Merge branch 'openjdk:master' into Base64-AVX2
> - Address review comment
> - Remove whitespace
> - Fix wrong register usage
> - Working version of Base64 decode with AVX2 (4x perf improvement). No URL
> support
> - Merge branch 'Base64-AVX2' of https://github.com/asgibbons/jdk into
> Base64-AVX2
> - ... and 3 more: https://git.openjdk.org/jdk/compare/50dc1196...3e66f7be
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2159:
> 2157: }
> 2158:
> 2159: address StubGenerator::base64_AVX2_tables_addr() {
A casual reader might wonder why there's 3 other avx2-tables and then this, so
for readability it'd be nice to add "decode" to the name of this new table.
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2643:
> 2641: // Handle isURL / MIME?!?! r12 is used for length calculation
> (from out)
> 2642: //
> 2643: // rbx is out, r12 is saved out, rdx is size, rsi is src
It seems that on windows `r12` is in use, see line 2323. GHA seem to be having
some trouble finishing Windows testing on time - could there be some issue here?
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2647:
> 2645:
> 2646:
> 2647: // *** NEEDS TO BE FIXED
While it's fine to leave implementation of `getMimeDecoder` and `getUrlDecoder`
for a follow-up, I think these comments needs to be cleaned up. E.g. a
follow-up RFE filed and referenced.
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2651:
> 2649: __ jcc(Assembler::notZero, L_tailProc);
> 2650:
> 2651: __ cmpl(length, 44);
Perform `length` checks first to avoid unnecessary branches on small inputs?
Ideal might be to move this length check up just before the `_cmpl(length,
128)` in the AVX-512 block, so that if `AVX=3` short inputs branch directly to
the scalar tail procedure without jumping around. This might also apply to the
encode stub, though that's pre-existing.
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2673:
> 2671: __ vmovdqu(xmm13, Address(r13, 0xc8));
> 2672: __ vmovdqu(xmm12, Address(r13, 0x08));
> 2673: __ jmp(L_enterLoop);
This got me curious (sorry!) and maybe there's something going on here that I'm
not getting... But why have you split the loop apart and jump into the middle
of it? It'd seem not doing this would be more straightforward, with no
difference semantically and one less jump? (align32 should just translate to a
narrow nop instruction, if anything)
-
PR: https://git.openjdk.org/jdk/pull/12126
Re: RFR: 8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories [v15]
On Tue, 24 Jan 2023 12:31:30 GMT, Eirik Bjorsnos wrote: >> Some call sites of SignatureFileVerifier.isBlockOrSF fails to check that >> files reside in META-INF directly, and not in a subdirectory of META-INF. >> >> The mentioned call sites needs updates to check and ignore such files. >> >> A new test IgnoreUnrelatedSignatureFiles is added which verifies that [*.SF, >> *.RSA] files in META-INF/ subdirectories are indeed ignored. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Add whitespace after keywords if and for Marked as reviewed by weijun (Reviewer). Yes, you're right. `ZipFile:: isSignatureRelated ` collects them for verification but `SignatureFileVerifier:: isSigningRelated` is about do not re-sign them while signing. Maybe we can rename `ZipFile::isSignatureRelated` to `ZipFile::isBlockOrSF` as well? I'll approved this PR. Thanks. - PR: https://git.openjdk.org/jdk/pull/11976
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter [v2]
On Fri, 27 Jan 2023 21:27:57 GMT, Raffaello Giulietti wrote: >> test/jdk/java/util/Formatter/BasicInt.java line 1: >> >>> 1: /* >> >> It looks line the non-float/double test classes are unchanged, they could be >> dropped from the PR. > > The only changes I made myself in the test files are in Basic-X.java.template > (including the copyright year). The other files were generated by a script, > which happens to also change the copyright year for otherwise unmodified > files. > > If you agree, I'll happily remove the trivial copyright year changes and only > retain those files containing more substantial modifications. Fine with me, I figured it was the script that changed the copyright and otherwise generated the same as before. tnx - PR: https://git.openjdk.org/jdk/pull/12259
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter [v2]
On Fri, 27 Jan 2023 19:56:28 GMT, Roger Riggs wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8300869: Make use of the Double.toString(double) algorithm in >> java.util.Formatter >> >> Added tests for known differences between current and new behavior. > > test/jdk/java/util/Formatter/BasicInt.java line 1: > >> 1: /* > > It looks line the non-float/double test classes are unchanged, they could be > dropped from the PR. The only changes I made myself in the test files are in Basic-X.java.template (including the copyright year). The other files were generated by a script, which happens to also change the copyright year for otherwise unmodified files. If you agree, I'll happily remove the trivial copyright year changes and only retain those files containing more substantial modifications. - PR: https://git.openjdk.org/jdk/pull/12259
RFR: Merge jdk20
Forwardport JDK 20 -> JDK 21 - Commit messages: - Merge remote-tracking branch 'jdk20/master' into Merge_jdk20 - 8301206: Fix issue with LocaleData after JDK-8300719 - 8300953: ClassDesc::ofInternalName missing @since tag - 8300719: JDK 20 RDP2 L10n resource files update The webrevs contain the adjustments done while merging with regards to each parent branch: - master: https://webrevs.openjdk.org/?repo=jdk=12267=00.0 - jdk20: https://webrevs.openjdk.org/?repo=jdk=12267=00.1 Changes: https://git.openjdk.org/jdk/pull/12267/files Stats: 1078 lines in 85 files changed: 220 ins; 114 del; 744 mod Patch: https://git.openjdk.org/jdk/pull/12267.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12267/head:pull/12267 PR: https://git.openjdk.org/jdk/pull/12267
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]
On Fri, 27 Jan 2023 20:58:40 GMT, Rémi Forax wrote: > I think the simplest solution is to have a non public interface declared > inside java.util. Like java.util.RandomAccess, but non public. The main > advantage to use an interface is that you can document it and it's easy to > find all the implementations. I think this is feasible, but it should be placed in a sub-package in `jdk.internal`, because some trusted collections are outside `java.util`. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]
On Fri, 27 Jan 2023 13:53:45 GMT, Glavo wrote: >> I checked the `java.base` module, and all the `Collection#toArray()` method >> of collections be implemented correctly. >> >> Their return values can be trusted, so many unnecessary array duplication >> can be eliminated. > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > Collections.isTrustedCollection I think the simplest solution is to have a non public interface declared inside java.util. Like java.util.RandomAccess, but non public. The main advantage to use an interface is that you can document it and it's easy to find all the implementations. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]
On Fri, 27 Jan 2023 14:28:08 GMT, Alan Bateman wrote: > I skimmed through the latest update. I see you've considered the possibility > of a Matryoshka doll showing up but the optimization is still very very > scary. I think it would require an audit of every API in java.base that > potentially wraps a collection. It might be better to focus on a few specific > cases that can be proven to be safe. It is good to limit the trusted collection to a set of known collections. I am thinking about the best way to implement it. I hope that the impact on performance will be as small as possible. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter [v2]
On Fri, 27 Jan 2023 18:19:58 GMT, Raffaello Giulietti wrote: >> Align `double` and `float` decimal conversions in `java.util.Formatter` with >> the algorithm used in `Double.toString(double)`. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8300869: Make use of the Double.toString(double) algorithm in > java.util.Formatter > > Added tests for known differences between current and new behavior. test/jdk/java/util/Formatter/BasicInt.java line 1: > 1: /* It looks line the non-float/double test classes are unchanged, they could be dropped from the PR. - PR: https://git.openjdk.org/jdk/pull/12259
Integrated: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections
On Wed, 25 Jan 2023 21:41:37 GMT, Tagir F. Valeev wrote: > number of minor cleanups could be done in Arrays and Collections utility > classes. > In Arrays: > - Redundant import jdk.internal.misc.Unsafe; > - C-style array declaration is used in public static boolean equals(short[] > a, short a2[]) (that's the only place in the whole file) > > In Collections: > - A few obsolete "unchecked" and "rawtypes" suppressions > - Unnecessary local variable initializer in get() method > - Raw type can be avoided in a number of casts > - Explicit type parameters could be omitted or converted to diamonds > - A couple of javadoc links on private APIs are malformed This pull request has now been integrated. Changeset: ae0e76d3 Author:Tagir F. Valeev URL: https://git.openjdk.org/jdk/commit/ae0e76d3dd42de9e66196843e740e75b06894f1f Stats: 28 lines in 2 files changed: 0 ins; 6 del; 22 mod 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections Reviewed-by: smarks, darcy - PR: https://git.openjdk.org/jdk/pull/12207
Withdrawn: 8296546: Add @spec tags to API
On Thu, 10 Nov 2022 01:10:13 GMT, Jonathan Gibbons wrote: > Please review a "somewhat automated" change to insert `@spec` tags into doc > comments, as appropriate, to leverage the recent new javadoc feature to > generate a new page listing the references to all external specifications > listed in the `@spec` tags. > > "Somewhat automated" means that I wrote and used a temporary utility to scan > doc comments looking for HTML links to selected sites, such as `ietf.org`, > `unicode.org`, `w3.org`. These links may be in the main description of a doc > comment, or in `@see` tags. For each link, the URL is examined, and > "normalized", and inserted into the doc comment with a new `@spec` tag, > giving the link and tile for the spec. > > "Normalized" means... > * Use `https:` where possible (includes pretty much all cases) > * Use a single consistent host name for all URLs coming from the same spec > site (i.e. don't use different aliases for the same site) > * Point to the root page of a multi-page spec > * Use a consistent form of the spec, preferring HTML over plain text where > both are available (this mostly applies to IETF specs) > > In addition, a "standard" title is determined for all specs, determined > either from the content of the (main) spec page or from site index pages. > > The net effect is (or should be) that **all** the changes are to just **add** > new `@spec` tags, based on the links found in each doc comment. There should > be no other changes to the doc comments, or to the implementation of any > classes and interfaces. > > That being said, the utility I wrote does have additional abilities, to > update the links that it finds (e.g. changing to use `https:` etc,) but those > features are _not_ being used here, but could be used in followup PRs if > component teams so desired. I did notice while working on this overall > feature that many of our links do point to "outdated" pages, some with > eye-catching notices declaring that the spec has been superseded. Determining > how, when and where to update such links is beyond the scope of this PR. > > Going forward, it is to be hoped that component teams will maintain the > underlying links, and the URLs in `@spec` tags, such that if references to > external specifications are updated, this will include updating the `@spec` > tags. > > To see the effect of all these new `@spec` tags, see > http://cr.openjdk.java.net/~jjg/8296546/api.00/ > > In particular, see the new [External > Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) > page, which you can also find via the new link near the top of the > [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) > pages. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v6]
> Added code for Base64 acceleration (encode and decode) which will accelerate > ~4x for AVX2 platforms. > > Encode performance: > **Old:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 4309.439 ± 2.632 > ops/ms > > > **New:** > > Benchmark (maxNumBytes) Mode Cnt Score Error > Units > Base64Encode.testBase64Encode 1024 thrpt3 24211.397 ± 102.026 > ops/ms > > > Decode performance: > **Old:** > > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 3961.768 ± 93.409 ops/ms > > **New:** > Benchmark (errorIndex) (lineSize) (maxNumBytes) Mode > Cnt ScoreError Units > Base64Decode.testBase64Decode 144 4 1024 thrpt >3 14738.051 ± 24.383 ops/ms Scott Gibbons has updated the pull request 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 13 additional commits since the last revision: - Merge branch 'openjdk:master' into Base64-AVX2 - Merge branch 'openjdk:master' into Base64-AVX2 - Merge branch 'openjdk:master' into Base64-AVX2 - Merge branch 'Base64-AVX2' of https://github.com/asgibbons/jdk into Base64-AVX2 - Merge branch 'openjdk:master' into Base64-AVX2 - Address review comment - Remove whitespace - Fix wrong register usage - Working version of Base64 decode with AVX2 (4x perf improvement). No URL support - Merge branch 'Base64-AVX2' of https://github.com/asgibbons/jdk into Base64-AVX2 - ... and 3 more: https://git.openjdk.org/jdk/compare/ad44dd9d...3e66f7be - Changes: - all: https://git.openjdk.org/jdk/pull/12126/files - new: https://git.openjdk.org/jdk/pull/12126/files/98728555..3e66f7be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12126=05 - incr: https://webrevs.openjdk.org/?repo=jdk=12126=04-05 Stats: 18402 lines in 710 files changed: 4705 ins; 3233 del; 10464 mod Patch: https://git.openjdk.org/jdk/pull/12126.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12126/head:pull/12126 PR: https://git.openjdk.org/jdk/pull/12126
Integrated: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module
On Thu, 26 Jan 2023 21:03:59 GMT, Mandy Chung wrote: > Currently, a `Lookup` object with `PACKAGE` access can be used to inject a > class in the runtime package of the Lookup's lookup class via > `Lookup::defineClass`. The classes that are injected have the same access > as other members in the module and can access private members of all types in > the module via reflection. > > However, changing `Lookup.defineClass` to require full privilege access > (`PRIVATE` + `MODULE`) is an incompatible change that would break existing > frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject > auxiliary classes in a module. A module authorizes the framework by opening > a package for it to access and `Lookup::defineClass` was the supported > replacement for `setAccessible` on `ClassLoader::defineClass` hack in JDK 9. > > > This PR proposes to keep existing behavior and provide better documentation > to help developers to beware of the permissions given out when opening a > package to another module. A class injected in a module has the same > privilege as other module members. This pull request has now been integrated. Changeset: 7f05d57a Author:Mandy Chung URL: https://git.openjdk.org/jdk/commit/7f05d57a87d8b41b53194aa0dacc4057cbb58544 Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module Reviewed-by: psandoz, alanb, darcy - PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v4]
On Fri, 27 Jan 2023 18:20:42 GMT, Mandy Chung wrote: >> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a >> class in the runtime package of the Lookup's lookup class via >> `Lookup::defineClass`. The classes that are injected have the same access >> as other members in the module and can access private members of all types >> in the module via reflection. >> >> However, changing `Lookup.defineClass` to require full privilege access >> (`PRIVATE` + `MODULE`) is an incompatible change that would break existing >> frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject >> auxiliary classes in a module. A module authorizes the framework by >> opening a package for it to access and `Lookup::defineClass` was the >> supported replacement for `setAccessible` on `ClassLoader::defineClass` hack >> in JDK 9. >> >> This PR proposes to keep existing behavior and provide better documentation >> to help developers to beware of the permissions given out when opening a >> package to another module. A class injected in a module has the same >> privilege as other module members. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > fine tune wording Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v4]
On Fri, 27 Jan 2023 18:20:42 GMT, Mandy Chung wrote: >> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a >> class in the runtime package of the Lookup's lookup class via >> `Lookup::defineClass`. The classes that are injected have the same access >> as other members in the module and can access private members of all types >> in the module via reflection. >> >> However, changing `Lookup.defineClass` to require full privilege access >> (`PRIVATE` + `MODULE`) is an incompatible change that would break existing >> frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject >> auxiliary classes in a module. A module authorizes the framework by >> opening a package for it to access and `Lookup::defineClass` was the >> supported replacement for `setAccessible` on `ClassLoader::defineClass` hack >> in JDK 9. >> >> This PR proposes to keep existing behavior and provide better documentation >> to help developers to beware of the permissions given out when opening a >> package to another module. A class injected in a module has the same >> privilege as other module members. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > fine tune wording Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections [v2]
On Wed, 25 Jan 2023 22:23:26 GMT, Tagir F. Valeev wrote: >> number of minor cleanups could be done in Arrays and Collections utility >> classes. >> In Arrays: >> - Redundant import jdk.internal.misc.Unsafe; >> - C-style array declaration is used in public static boolean equals(short[] >> a, short a2[]) (that's the only place in the whole file) >> >> In Collections: >> - A few obsolete "unchecked" and "rawtypes" suppressions >> - Unnecessary local variable initializer in get() method >> - Raw type can be avoided in a number of casts >> - Explicit type parameters could be omitted or converted to diamonds >> - A couple of javadoc links on private APIs are malformed > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Revert cast removal Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12207
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter [v2]
On Fri, 27 Jan 2023 18:19:58 GMT, Raffaello Giulietti wrote: >> Align `double` and `float` decimal conversions in `java.util.Formatter` with >> the algorithm used in `Double.toString(double)`. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8300869: Make use of the Double.toString(double) algorithm in > java.util.Formatter > > Added tests for known differences between current and new behavior. Added tests for known differences. Most of the modified files are generated automatically by a script that, somewhat questionably, renews the copyright year in some of the files for no apparent reason. - PR: https://git.openjdk.org/jdk/pull/12259
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v4]
> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a > class in the runtime package of the Lookup's lookup class via > `Lookup::defineClass`. The classes that are injected have the same access > as other members in the module and can access private members of all types in > the module via reflection. > > However, changing `Lookup.defineClass` to require full privilege access > (`PRIVATE` + `MODULE`) is an incompatible change that would break existing > frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject > auxiliary classes in a module. A module authorizes the framework by opening > a package for it to access and `Lookup::defineClass` was the supported > replacement for `setAccessible` on `ClassLoader::defineClass` hack in JDK 9. > > > This PR proposes to keep existing behavior and provide better documentation > to help developers to beware of the permissions given out when opening a > package to another module. A class injected in a module has the same > privilege as other module members. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: fine tune wording - Changes: - all: https://git.openjdk.org/jdk/pull/12236/files - new: https://git.openjdk.org/jdk/pull/12236/files/487baca5..403cff53 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12236=03 - incr: https://webrevs.openjdk.org/?repo=jdk=12236=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12236.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12236/head:pull/12236 PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter [v2]
> Align `double` and `float` decimal conversions in `java.util.Formatter` with > the algorithm used in `Double.toString(double)`. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter Added tests for known differences between current and new behavior. - Changes: - all: https://git.openjdk.org/jdk/pull/12259/files - new: https://git.openjdk.org/jdk/pull/12259/files/bcbbe1e9..d42fbd03 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12259=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12259=00-01 Stats: 188 lines in 20 files changed: 168 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/12259.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12259/head:pull/12259 PR: https://git.openjdk.org/jdk/pull/12259
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]
On Fri, 27 Jan 2023 14:28:08 GMT, Alan Bateman wrote: > It might be better to focus on a few specific cases that can be proven to be > safe As of trusted collections from java.base we have - ArrayList - Arrays.asList() - HashSet - LinkedHashSet - TreeSet - EnumSet - ArrayDeque - COWArrayList - ConcurrentLinkedDeque - Key/Value/EntrySets of Maps Wouldn't it be too expensive to check whether incoming Collection's class is one of the enlisted? - PR: https://git.openjdk.org/jdk/pull/12212
Integrated: 8177418: NPE is not apparent for methods in java.util.TimeZone API docs
On Fri, 6 Jan 2023 22:38:13 GMT, Justin Lu wrote: > When their input is null, the following methods in java.util.TimeZone throw a > NullPointerException: > > _TimeZone.getTimeZone(String ID) > TimeZone.setID(String ID) > TimeZone.inDaylightTime(Date date)_ > > For example, > > > String someID = null; > TimeZone tz1 = TimeZone.getTimeZone(someID); > ``` > > throws a `NullPointerException` > > > This PR adds the missing _@throws:_ for the mentioned methods. The wording > and specification is also adjusted for the overridable methods in TZ to use > "_may throw_" over "_will throw_" because of the possibility of external > sub-classes that may override the method. This pull request has now been integrated. Changeset: 22c976a9 Author:Justin Lu Committer: Naoto Sato URL: https://git.openjdk.org/jdk/commit/22c976a9b042b2d56e849ec8f9ef1dd3d146ca78 Stats: 19 lines in 2 files changed: 16 ins; 0 del; 3 mod 8177418: NPE is not apparent for methods in java.util.TimeZone API docs Reviewed-by: lancea, naoto - PR: https://git.openjdk.org/jdk/pull/11888
Re: RFR: JDK-8301205: Port fdlibm log10 to Java [v2]
On Fri, 27 Jan 2023 13:47:07 GMT, Raffaello Giulietti wrote: >> src/java.base/share/classes/java/lang/FdLibm.java line 820: >> >>> 818: hx = (hx & 0x000f_) | ((0x3ff - i) << 20); >>> 819: y = (double)(k + i); >>> 820: x = __HI(x, hx); >> >> `x = __HI(x, hx); // replace high word of x with hx` > > Otherwise LGTM Okay; comment added back. - PR: https://git.openjdk.org/jdk/pull/12242
Re: RFR: JDK-8301205: Port fdlibm log10 to Java [v2]
> Restarting the port of FDLIBM to Java with the log10 method. > > There are two port, the first a near-transliteration from C port to use as a > test reference in > > test/jdk/java/lang/StrictMath/FdlibmTranslit.java > > and a more idiomatic Java port in > > src/java.base/share/classes/java/lang/FdLibm.java > > First I debug the FdlibmTranslit port by running it against the C version. > Once that is working, I copy the port into the java.lang.FdLibm.java and do a > series of transformation to get the code closer to usual JDK style. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Add comment. - Changes: - all: https://git.openjdk.org/jdk/pull/12242/files - new: https://git.openjdk.org/jdk/pull/12242/files/17cf431a..0f74dd5f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12242=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12242=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12242.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12242/head:pull/12242 PR: https://git.openjdk.org/jdk/pull/12242
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v3]
On Fri, 27 Jan 2023 17:41:43 GMT, Mandy Chung wrote:
>> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a
>> class in the runtime package of the Lookup's lookup class via
>> `Lookup::defineClass`. The classes that are injected have the same access
>> as other members in the module and can access private members of all types
>> in the module via reflection.
>>
>> However, changing `Lookup.defineClass` to require full privilege access
>> (`PRIVATE` + `MODULE`) is an incompatible change that would break existing
>> frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject
>> auxiliary classes in a module. A module authorizes the framework by
>> opening a package for it to access and `Lookup::defineClass` was the
>> supported replacement for `setAccessible` on `ClassLoader::defineClass` hack
>> in JDK 9.
>>
>> This PR proposes to keep existing behavior and provide better documentation
>> to help developers to beware of the permissions given out when opening a
>> package to another module. A class injected in a module has the same
>> privilege as other module members.
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review feedback and add @apiNote
The update looks good.
src/java.base/share/classes/java/lang/Module.java line 606:
> 604: * {@link java.lang.invoke.MethodHandles.Lookup Lookup} object that
> is allowed to
> 605: * {@link java.lang.invoke.MethodHandles.Lookup#defineClass(byte[])
> define classes}
> 606: * in package {@code p}.
A small suggestion here is to change "means that code in M" to "allows code in
M".
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v2]
On Fri, 27 Jan 2023 17:07:08 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/Module.java line 605:
>>
>>> 603: * {@link java.lang.invoke.MethodHandles.Lookup Lookup} object
>>> that is allowed to
>>> 604: * {@link
>>> java.lang.invoke.MethodHandles.Lookup#defineClass(byte[]) define classes}
>>> 605: * in package {@code p}.
>>
>> I wonder if this should be an apiNote rather method description. There is
>> also the no-arg isOpen method and maybe we should add a note there too. What
>> would you think about linking "deep reflection" to
>> AccessibleObject.setAccesssible(boolean) ?
>
>> I wonder if this should be an apiNote rather method description.
>
> I considered this and no clear cut on this. I don't have a strong opinion on
> this. What do you think?
I updated with apiNote, which is clearer.
-
PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v3]
> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a > class in the runtime package of the Lookup's lookup class via > `Lookup::defineClass`. The classes that are injected have the same access > as other members in the module and can access private members of all types in > the module via reflection. > > However, changing `Lookup.defineClass` to require full privilege access > (`PRIVATE` + `MODULE`) is an incompatible change that would break existing > frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject > auxiliary classes in a module. A module authorizes the framework by opening > a package for it to access and `Lookup::defineClass` was the supported > replacement for `setAccessible` on `ClassLoader::defineClass` hack in JDK 9. > > > This PR proposes to keep existing behavior and provide better documentation > to help developers to beware of the permissions given out when opening a > package to another module. A class injected in a module has the same > privilege as other module members. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: review feedback and add @apiNote - Changes: - all: https://git.openjdk.org/jdk/pull/12236/files - new: https://git.openjdk.org/jdk/pull/12236/files/4d3e879c..487baca5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12236=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12236=01-02 Stats: 17 lines in 2 files changed: 11 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12236.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12236/head:pull/12236 PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8177418: NPE is not apparent for methods in java.util.TimeZone API docs [v8]
On Wed, 25 Jan 2023 20:21:11 GMT, Justin Lu wrote: >> When their input is null, the following methods in java.util.TimeZone throw >> a NullPointerException: >> >> _TimeZone.getTimeZone(String ID) >> TimeZone.setID(String ID) >> TimeZone.inDaylightTime(Date date)_ >> >> For example, >> >> >> String someID = null; >> TimeZone tz1 = TimeZone.getTimeZone(someID); >> ``` >> >> throws a `NullPointerException` >> >> >> This PR adds the missing _@throws:_ for the mentioned methods. The wording >> and specification is also adjusted for the overridable methods in TZ to use >> "_may throw_" over "_will throw_" because of the possibility of external >> sub-classes that may override the method. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > STZ.inDT can also be overriden Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11888
[jdk20] Integrated: 8301206: Fix issue with LocaleData after JDK-8300719
On Fri, 27 Jan 2023 01:43:11 GMT, Damon Nguyen wrote: > Localization update didn't include an update to LocaleData file. This PR > addresses this by updating the file with the newly translated values. The > LocaleDataTest now passes. This should address the recent related failures to > LocaleData. This pull request has now been integrated. Changeset: e5860ef6 Author:Damon Nguyen Committer: Naoto Sato URL: https://git.openjdk.org/jdk20/commit/e5860ef60a9353508afb09716158baf8bfb35559 Stats: 54 lines in 2 files changed: 1 ins; 0 del; 53 mod 8301206: Fix issue with LocaleData after JDK-8300719 Reviewed-by: naoto - PR: https://git.openjdk.org/jdk20/pull/118
Re: [Proposal] Make toLowerCase and toUpperCase based on Locale.ROOT by default
+1 Unfortunately, it would be too disruptive to change the decades old behavior. Naoto On 1/27/23 3:53 AM, Alan Bateman wrote: On 27/01/2023 05:42, Glavo wrote: I analyzed the usage of toLowerCase and toUpperCase in OpenJDK more carefully, and found that none of the use cases really expected locale-sensitive behavior I expected that as there were a number of passes over these use-sites over the years. That said, if some of the remaining usages can be changed to specify the locale then it would avoid others puzzling over them too. Changing them probably needs careful review so changing them in specific areas or in smaller batches would make it easier for potential Reviewers. In any case, I agree with Rémi that changing the existing methods is too subtle and maybe it's time to look at no-arg replacements. -Alan
Integrated: 8300924: Method::invoke throws wrong exception type when passing wrong number of arguments to method with 4 or more parameters
On Tue, 24 Jan 2023 18:19:22 GMT, Mandy Chung wrote: > A simple fix in core reflection to check if the number of actual and formal > parameters differ before invoking the method or the constructor regardless of > whether it's a specialized case or not. This pull request has now been integrated. Changeset: 7aaf76c5 Author:Mandy Chung URL: https://git.openjdk.org/jdk/commit/7aaf76c5290a1688f9450a357aaae964615c29d0 Stats: 85 lines in 3 files changed: 67 ins; 10 del; 8 mod 8300924: Method::invoke throws wrong exception type when passing wrong number of arguments to method with 4 or more parameters Reviewed-by: rriggs - PR: https://git.openjdk.org/jdk/pull/12170
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v2]
On Fri, 27 Jan 2023 11:44:46 GMT, Alan Bateman wrote: > I wonder if this should be an apiNote rather method description. I considered this and no clear cut on this. I don't have a strong opinion on this. What do you think? - PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections [v2]
On Wed, 25 Jan 2023 22:23:26 GMT, Tagir F. Valeev wrote: >> number of minor cleanups could be done in Arrays and Collections utility >> classes. >> In Arrays: >> - Redundant import jdk.internal.misc.Unsafe; >> - C-style array declaration is used in public static boolean equals(short[] >> a, short a2[]) (that's the only place in the whole file) >> >> In Collections: >> - A few obsolete "unchecked" and "rawtypes" suppressions >> - Unnecessary local variable initializer in get() method >> - Raw type can be avoided in a number of casts >> - Explicit type parameters could be omitted or converted to diamonds >> - A couple of javadoc links on private APIs are malformed > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Revert cast removal Test run looks good, ok to integrate. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/12207
Re: RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter
On Fri, 27 Jan 2023 16:02:38 GMT, Raffaello Giulietti wrote: > Align `double` and `float` decimal conversions in `java.util.Formatter` with > the algorithm used in `Double.toString(double)`. The specification in `Formatter` explicitly refers to the outcome of `Double.toString(double)`. However, it currently uses another implementation for the floating-point to decimal conversion, which might end up in rare, slightly different outcomes than the ones described in the specification. This fixes the discrepancy between specification and implementation in `Formatter`. Class `jdk.internal.math.FormattedFPDecimal` replaces `jdk.internal.math.FormattedFloatingDecimal`, which is no longer needed. - PR: https://git.openjdk.org/jdk/pull/12259
RFR: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter
Align `double` and `float` decimal conversions in `java.util.Formatter` with the algorithm used in `Double.toString(double)`. - Commit messages: - 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter Changes: https://git.openjdk.org/jdk/pull/12259/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12259=00 Issue: https://bugs.openjdk.org/browse/JDK-8300869 Stats: 720 lines in 4 files changed: 319 ins; 372 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/12259.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12259/head:pull/12259 PR: https://git.openjdk.org/jdk/pull/12259
[jdk20] Integrated: 8300953: ClassDesc::ofInternalName missing @since tag
On Tue, 24 Jan 2023 13:35:14 GMT, Adam Sotona wrote: > ClassDesc::ofInternalName was added in JDK 20, however @since tag is missing. > This patch fixes the javadoc. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: b22e5216 Author:Adam Sotona URL: https://git.openjdk.org/jdk20/commit/b22e5216c4ead4621f137086db6f5b6a0c3982c7 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8300953: ClassDesc::ofInternalName missing @since tag Reviewed-by: darcy, mchung, jjg - PR: https://git.openjdk.org/jdk20/pull/115
Re: [jdk20] RFR: 8300953: ClassDesc::ofInternalName missing @since tag
On Tue, 24 Jan 2023 13:35:14 GMT, Adam Sotona wrote: > ClassDesc::ofInternalName was added in JDK 20, however @since tag is missing. > This patch fixes the javadoc. > > Please review. > > Thanks, > Adam Marked as reviewed by jjg (Reviewer). - PR: https://git.openjdk.org/jdk20/pull/115
Re: RFR: 8294982: Implementation of Classfile API [v11]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 106 commits: - Merge branch 'master' into JDK-8294982 - update of Java version constants - unification of sealed class modifiers order - merged JAVAC_FLAGS and EXCLUDES in Java.gmk - removal of Preview.java and TransPatterns.java patch and enabled preview for java.base - jdk.compiler ClassWriter patch to avoid writing PREVIEW_MINOR_VERSION to classes participating in preview - Merge branch 'master' into JDK-8294982 - Merge branch 'master' into JDK-8294982 - javadoc cleanup - removal of AccessController::doPriviledged from ClassHierarchyResolver - ... and 96 more: https://git.openjdk.org/jdk/compare/e4252bb9...80213e61 - Changes: https://git.openjdk.org/jdk/pull/10982/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10982=10 Stats: 53409 lines in 323 files changed: 53405 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]
On Fri, 27 Jan 2023 13:53:45 GMT, Glavo wrote: >> I checked the `java.base` module, and all the `Collection#toArray()` method >> of collections be implemented correctly. >> >> Their return values can be trusted, so many unnecessary array duplication >> can be eliminated. > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > Collections.isTrustedCollection I skimmed through the latest update. I see you've considered the possibility of a Matryoshka doll showing up but the optimization is still very very scary. I think it would require an audit of every API in java.base that potentially wraps a collection. It might be better to focus on a few specific cases that can be proven to be safe. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]
> I checked the `java.base` module, and all the `Collection#toArray()` method > of collections be implemented correctly. > > Their return values can be trusted, so many unnecessary array duplication can > be eliminated. Glavo has updated the pull request incrementally with one additional commit since the last revision: Collections.isTrustedCollection - Changes: - all: https://git.openjdk.org/jdk/pull/12212/files - new: https://git.openjdk.org/jdk/pull/12212/files/e0966d49..aecd79ff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12212=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12212=00-01 Stats: 51 lines in 8 files changed: 36 ins; 2 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/12212.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12212/head:pull/12212 PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: JDK-8301205: Port fdlibm log10 to Java
On Fri, 27 Jan 2023 06:52:31 GMT, Joe Darcy wrote: > Restarting the port of FDLIBM to Java with the log10 method. > > There are two port, the first a near-transliteration from C port to use as a > test reference in > > test/jdk/java/lang/StrictMath/FdlibmTranslit.java > > and a more idiomatic Java port in > > src/java.base/share/classes/java/lang/FdLibm.java > > First I debug the FdlibmTranslit port by running it against the C version. > Once that is working, I copy the port into the java.lang.FdLibm.java and do a > series of transformation to get the code closer to usual JDK style. src/java.base/share/classes/java/lang/FdLibm.java line 820: > 818: hx = (hx & 0x000f_) | ((0x3ff - i) << 20); > 819: y = (double)(k + i); > 820: x = __HI(x, hx); `x = __HI(x, hx); // replace high word of x with hx` - PR: https://git.openjdk.org/jdk/pull/12242
Re: RFR: JDK-8301205: Port fdlibm log10 to Java
On Fri, 27 Jan 2023 13:45:18 GMT, Raffaello Giulietti wrote: >> Restarting the port of FDLIBM to Java with the log10 method. >> >> There are two port, the first a near-transliteration from C port to use as a >> test reference in >> >> test/jdk/java/lang/StrictMath/FdlibmTranslit.java >> >> and a more idiomatic Java port in >> >> src/java.base/share/classes/java/lang/FdLibm.java >> >> First I debug the FdlibmTranslit port by running it against the C version. >> Once that is working, I copy the port into the java.lang.FdLibm.java and do >> a series of transformation to get the code closer to usual JDK style. > > src/java.base/share/classes/java/lang/FdLibm.java line 820: > >> 818: hx = (hx & 0x000f_) | ((0x3ff - i) << 20); >> 819: y = (double)(k + i); >> 820: x = __HI(x, hx); > > `x = __HI(x, hx); // replace high word of x with hx` Otherwise LGTM - PR: https://git.openjdk.org/jdk/pull/12242
Re: RFR: 8294982: Implementation of Classfile API [v10]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: update of Java version constants - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/931ca1c5..806b9025 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=09 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=08-09 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
On Fri, 27 Jan 2023 12:15:34 GMT, Glavo wrote: > > What about Collections.synchronizedCollection and other methods that may > > wrap/delagete to collections from other modules? > > Good question, I missed this point. I think I should provide a helper method > to check whether the collection is trusted. Maybe you can add an internal annotation and annotate all trusted collection classes with it. - PR: https://git.openjdk.org/jdk/pull/12212
Re: [jdk20] RFR: 8301206: Fix issue with LocaleData after JDK-8300719 [v2]
On Fri, 27 Jan 2023 02:34:37 GMT, Damon Nguyen wrote: >> Localization update didn't include an update to LocaleData file. This PR >> addresses this by updating the file with the newly translated values. The >> LocaleDataTest now passes. This should address the recent related failures >> to LocaleData. > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Add bugid. Update year Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.org/jdk20/pull/118
Re: Is ReDos seen as bug/vulnerability?
On 27/01/2023 11:50, David Schumann wrote: Hello, during a PenTest we found a ReDos issue in the JRE which causes Matcher.matches() to go into an endless loop. Is such an issue considered a bug for the JDK team (aka should I file a bug report)? Or is such an issue considered "by design"? The issue appears in current JRE versions (tested with 17 and 21) We can't discuss such matters here. If you think there is a security issue then please report it to OpenJDK vulnerability group [1]. -Alan. [1] https://openjdk.org/groups/vulnerability/report
Re: Is ReDos seen as bug/vulnerability?
Please file a bug report with the relevant (and disclosable) details. From: core-libs-dev on behalf of David Schumann Date: Friday, 27 January 2023 at 12:50 To: core-libs-dev@openjdk.org Subject: Is ReDos seen as bug/vulnerability? Hello, during a PenTest we found a ReDos issue in the JRE which causes Matcher.matches() to go into an endless loop. Is such an issue considered a bug for the JDK team (aka should I file a bug report)? Or is such an issue considered "by design"? The issue appears in current JRE versions (tested with 17 and 21) Best Regards, David Schumann
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
On Fri, 27 Jan 2023 11:35:54 GMT, Alan Bateman wrote: > What about Collections.synchronizedCollection and other methods that may > wrap/delagete to collections from other modules? Good question, I missed this point. I think I should provide a helper method to check whether the collection is trusted. - PR: https://git.openjdk.org/jdk/pull/12212
Re: [Proposal] Make toLowerCase and toUpperCase based on Locale.ROOT by default
On 27/01/2023 05:42, Glavo wrote: I analyzed the usage of toLowerCase and toUpperCase in OpenJDK more carefully, and found that none of the use cases really expected locale-sensitive behavior I expected that as there were a number of passes over these use-sites over the years. That said, if some of the remaining usages can be changed to specify the locale then it would avoid others puzzling over them too. Changing them probably needs careful review so changing them in specific areas or in smaller batches would make it easier for potential Reviewers. In any case, I agree with Rémi that changing the existing methods is too subtle and maybe it's time to look at no-arg replacements. -Alan
Is ReDos seen as bug/vulnerability?
Hello, during a PenTest we found a ReDos issue in the JRE which causes Matcher.matches() to go into an endless loop. Is such an issue considered a bug for the JDK team (aka should I file a bug report)? Or is such an issue considered "by design"? The issue appears in current JRE versions (tested with 17 and 21) Best Regards, David Schumann
Re: RFR: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module [v2]
On Thu, 26 Jan 2023 22:27:36 GMT, Mandy Chung wrote:
>> Currently, a `Lookup` object with `PACKAGE` access can be used to inject a
>> class in the runtime package of the Lookup's lookup class via
>> `Lookup::defineClass`. The classes that are injected have the same access
>> as other members in the module and can access private members of all types
>> in the module via reflection.
>>
>> However, changing `Lookup.defineClass` to require full privilege access
>> (`PRIVATE` + `MODULE`) is an incompatible change that would break existing
>> frameworks which use `privateLookupIn` and `Lookup::defineClass` to inject
>> auxiliary classes in a module. A module authorizes the framework by
>> opening a package for it to access and `Lookup::defineClass` was the
>> supported replacement for `setAccessible` on `ClassLoader::defineClass` hack
>> in JDK 9.
>>
>> This PR proposes to keep existing behavior and provide better documentation
>> to help developers to beware of the permissions given out when opening a
>> package to another module. A class injected in a module has the same
>> privilege as other module members.
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review feedback
src/java.base/share/classes/java/lang/Module.java line 605:
> 603: * {@link java.lang.invoke.MethodHandles.Lookup Lookup} object that
> is allowed to
> 604: * {@link java.lang.invoke.MethodHandles.Lookup#defineClass(byte[])
> define classes}
> 605: * in package {@code p}.
I wonder if this should be an apiNote rather method description. There is also
the no-arg isOpen method and maybe we should add a note there too. What would
you think about linking "deep reflection" to
AccessibleObject.setAccesssible(boolean) ?
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 240:
> 238: * of {@code targetClass}. Extreme caution should be taken when
> opening a package
> 239: * to another module as such defined classes have the same full
> privilege
> 240: * access as other members in the target module.
"define classes" instead of inject classes is good.
-
PR: https://git.openjdk.org/jdk/pull/12236
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
On Thu, 26 Jan 2023 06:46:16 GMT, Glavo wrote: > I checked the `java.base` module, and all the `Collection#toArray()` method > of collections be implemented correctly. > > Their return values can be trusted, so many unnecessary array duplication can > be eliminated. What about Collections.synchronizedCollection and other methods that may wrap/delagete to collections from other modules? - PR: https://git.openjdk.org/jdk/pull/12212
Re: Math.clamp method?
Hello, Remi! > Given that the semantics of NaN is clearly defined for Math.max/min (if one > of the values is NaN the result is NaN), > I don't believe we need a special case here for NaN. > > The semantics should be, this is equivalent to execute > Math.max(min, Math.min(max, value)) > > So clamp(double) can be implemented using minsd and maxsd on x64, which is > already what the VM does. I agree that for NaN value, it's reasonable to return NaN. However, I think it's better to throw if min or max is NaN, as we should check the invariant that min <= max. We can do this in a single check: if (!(min <= max)) throw new IllegalArgumentException(...); With best regards, Tagir Valeev.
Re: Math.clamp method?
Hello, all! Thank you for an interesting discussion. As I see some interest towards this feature, I've created a new issue to track this: https://bugs.openjdk.org/browse/JDK-8301226 I haven't found an exact duplicate, only an issue with related discussion JDK-4466549. Feel free to link duplicates if you can search JIRA better than me :-) On Thu, Jan 26, 2023 at 12:47 AM John Rose wrote: > > Dealing with numeric ranges is error-prone, so I welcome these. > > Another operation on ranges is membership. This is easy to derive > from the clamp methods, I think: > >checkRange(x,min,max) := clamp(x,min,max) == x checkRange relates somehow to the methods we have in java.util.Objects, namely checkFromIndexSize, checkIndex, etc. Though these methods are throwing an exception, rather than returning false. This could be considered as a separate enhancement. > But I think there is no harm and real benefit from widening > the value parameters, so value is always either long or double. > That will save casts (which also cause bugs) for some use > cases, such as doing 32x32-to-64-bit arithmetic followed > by overflow checks and/or saturation. That’s a common use > case for clamping. > > public static int clamp(long value, int min, int max) > public static long clamp(long value, long min, long max) > public static double clamp(double value, double min, double max) > public static float clamp(double value, float min, float max) I really like the idea of int clamp(long value, int min, int max), as this makes the method much more useful. On the other hand, I have doubles about float clamp(double value, float min, float max), as it could be called accidentally and we will have accidental precision loss. E.g., imagine the following code: // bounds are declared as floats for some reason // this usually doesn't matter, as these numbers are exact in float and in double float lower = 0.0f; float upper = 10.0f; double v = 1/3.; System.out.println(v); v = Math.clamp(v, lower, upper); // oops! System.out.println(v); Having float clamp(double, float, float), this method will be linked here, resulting in accidental precision loss. The output is: 0. 0.333432674408 Given that float type is rarely used, I'd stick to the following signatures: public static int clamp(long value, int min, int max) public static long clamp(long value, long min, long max) public static double clamp(double value, double min, double max) public static float clamp(float value, float min, float max) > I thought about cross-domain clamps, which preserve 64 bit > precision and double range at the same time in different places: > > public static long clamp(long value, double min, double max) > public static long clamp(double value, long min, long max) This is even more dangerous, as simply calling double value = ... value = clamp(value, 0, 1); // not 0.0 and 1.0 you will get 0 instead of the original value that fits the range. I imagine that many people will step on this. If these methods are desired, it would be better to name them differently, to avoid overload ambiguity. For now, I would avoid this. > The subexpression max-min never overflows, when it > is regarded as an unsigned number, and the “>>>1” > immediately converts that unsigned number into the > correctly signed positive number. That’s the only > way to avoid overflow for all inputs. The third > idiom above (the “smartest” of the three) will fail, > for example, if you feed it negative values. > For some uses that is OK, but for others it isn’t. > > public static int midpoint(int min, int max) > public static long midpoint(long min, long max) > public static double midpoint(double min, double max) > public static float midpoint(float min, float max) midpoint is another great idea, I like it! Could also be considered separately. With best regards, Tagir Valeev.
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le [v2]
On Fri, 27 Jan 2023 10:16:56 GMT, Matthias Baesken wrote:
>> test/jdk/jdk/internal/vm/Continuation/Fuzz.java line 95:
>>
>>> 93: }
>>> 94: if (Platform.isPPC()) {
>>> 95: COMPILATION_TIMEOUT = COMPILATION_TIMEOUT * 2;
>>
>> I guess this won't compile. COMPILATION_TIMEOUT is final.
>
> Yes correct, it cannot be final any more.
Well it could be final if you would add `(Platform.isPPC() ? 2 : 1) * ...` to
the initialization. But I'm indifferent.
-
PR: https://git.openjdk.org/jdk/pull/12244
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le [v3]
On Fri, 27 Jan 2023 10:20:26 GMT, Matthias Baesken wrote: >> On our Linux ppc64le test machines we quite often see compilation timeouts >> in the test jdk/internal/vm/Continuation/Fuzz.java. >> (especially when running with fastdebug binaries) >> So it probably makes sense to use a higher compilation timeout (maybe factor >> 2) on this platform. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > COMPILATION_TIMEOUT cannot be final Marked as reviewed by rrich (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12244
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le [v2]
On Fri, 27 Jan 2023 10:07:31 GMT, Richard Reingruber wrote:
>> Matthias Baesken has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Do not limit to Linux, handle all PPC platforms
>
> test/jdk/jdk/internal/vm/Continuation/Fuzz.java line 95:
>
>> 93: }
>> 94: if (Platform.isPPC()) {
>> 95: COMPILATION_TIMEOUT = COMPILATION_TIMEOUT * 2;
>
> I guess this won't compile. COMPILATION_TIMEOUT is final.
Yes correct, it cannot be final any more.
-
PR: https://git.openjdk.org/jdk/pull/12244
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le [v3]
> On our Linux ppc64le test machines we quite often see compilation timeouts in > the test jdk/internal/vm/Continuation/Fuzz.java. > (especially when running with fastdebug binaries) > So it probably makes sense to use a higher compilation timeout (maybe factor > 2) on this platform. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: COMPILATION_TIMEOUT cannot be final - Changes: - all: https://git.openjdk.org/jdk/pull/12244/files - new: https://git.openjdk.org/jdk/pull/12244/files/74f2b80b..e8e3c3c5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12244=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12244=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12244.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12244/head:pull/12244 PR: https://git.openjdk.org/jdk/pull/12244
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le [v2]
On Fri, 27 Jan 2023 08:52:21 GMT, Matthias Baesken wrote:
>> On our Linux ppc64le test machines we quite often see compilation timeouts
>> in the test jdk/internal/vm/Continuation/Fuzz.java.
>> (especially when running with fastdebug binaries)
>> So it probably makes sense to use a higher compilation timeout (maybe factor
>> 2) on this platform.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Do not limit to Linux, handle all PPC platforms
test/jdk/jdk/internal/vm/Continuation/Fuzz.java line 95:
> 93: }
> 94: if (Platform.isPPC()) {
> 95: COMPILATION_TIMEOUT = COMPILATION_TIMEOUT * 2;
I guess this won't compile. COMPILATION_TIMEOUT is final.
-
PR: https://git.openjdk.org/jdk/pull/12244
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
On Thu, 26 Jan 2023 10:05:28 GMT, Sergey Tsypanov wrote: > > it is necessary to copy the elements to a larger new array > > Right, I missed this is addAll() method. Btw, in this class you could do > similar optimization in constructor as well. Just now, I have pushed some updates and used similar optimizations in some places that were missed before. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
On Thu, 26 Jan 2023 10:01:21 GMT, Glavo wrote: > it is necessary to copy the elements to a larger new array Right, I missed this is addAll() method. Btw, in this class you could do similar optimization in constructor as well. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
On Fri, 27 Jan 2023 09:35:00 GMT, Sergey Tsypanov wrote: > @Glavo I've filed https://bugs.openjdk.org/browse/JDK-8301220 so you can use > it for this PR. Thank you! > I think as soon as we anyway assign the array from trusted collection, we > don't need `len == 0` check here any more I think it is necessary to check the length here. If the length of the current COWArrayList is not zero, it is necessary to copy the elements to a larger new array. - PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
On Thu, 26 Jan 2023 06:46:16 GMT, Glavo wrote:
> I checked the `java.base` module, and all the `Collection#toArray()` method
> of collections be implemented correctly.
>
> Their return values can be trusted, so many unnecessary array duplication can
> be eliminated.
You could also have a look into `PriorityQueue.initElementsFromCollection()`
@Glavo I've filed https://bugs.openjdk.org/browse/JDK-8301220 so you can use it
for this PR.
src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java line
746:
> 744: int len = es.length;
> 745: Object[] newElements;
> 746: if (len == 0 && c.getClass().getModule() ==
> Object.class.getModule()) {
I think as soon as we anyway assign the array from trusted collection, we don't
need `len == 0` check here any more
-
PR: https://git.openjdk.org/jdk/pull/12212
RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted
I checked the `java.base` module, and all the `Collection#toArray()` method of collections be implemented correctly. Their return values can be trusted, so many unnecessary array duplication can be eliminated. - Commit messages: - fix checkstyle - Update ImmutableCollections - Merge branch 'master' into array-copy - Rollback Collectors - fix checkstyle - update - Update List.copyOf - Return value of toArray of collection types in java.base should be trusted Changes: https://git.openjdk.org/jdk/pull/12212/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12212=00 Issue: https://bugs.openjdk.org/browse/JDK-8301220 Stats: 14 lines in 6 files changed: 2 ins; 2 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/12212.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12212/head:pull/12212 PR: https://git.openjdk.org/jdk/pull/12212
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le [v2]
On Fri, 27 Jan 2023 08:41:12 GMT, Richard Reingruber wrote:
>> Matthias Baesken has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Do not limit to Linux, handle all PPC platforms
>
> test/jdk/jdk/internal/vm/Continuation/Fuzz.java line 94:
>
>> 92:+ "on macosx-aarch64");
>> 93: }
>> 94: if (Platform.isLinux() && Platform.isPPC()) {
>
> I wouldn't limit to Linux.
I changed it, removed the Linux related check.
-
PR: https://git.openjdk.org/jdk/pull/12244
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le [v2]
> On our Linux ppc64le test machines we quite often see compilation timeouts in > the test jdk/internal/vm/Continuation/Fuzz.java. > (especially when running with fastdebug binaries) > So it probably makes sense to use a higher compilation timeout (maybe factor > 2) on this platform. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Do not limit to Linux, handle all PPC platforms - Changes: - all: https://git.openjdk.org/jdk/pull/12244/files - new: https://git.openjdk.org/jdk/pull/12244/files/856c99c7..74f2b80b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12244=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12244=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12244.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12244/head:pull/12244 PR: https://git.openjdk.org/jdk/pull/12244
Re: RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le
On Fri, 27 Jan 2023 08:33:32 GMT, Matthias Baesken wrote:
> On our Linux ppc64le test machines we quite often see compilation timeouts in
> the test jdk/internal/vm/Continuation/Fuzz.java.
> (especially when running with fastdebug binaries)
> So it probably makes sense to use a higher compilation timeout (maybe factor
> 2) on this platform.
test/jdk/jdk/internal/vm/Continuation/Fuzz.java line 94:
> 92:+ "on macosx-aarch64");
> 93: }
> 94: if (Platform.isLinux() && Platform.isPPC()) {
I wouldn't limit to Linux.
-
PR: https://git.openjdk.org/jdk/pull/12244
RFR: JDK-8301163: jdk/internal/vm/Continuation/Fuzz.java increase COMPILATION_TIMEOUT for Linux ppc64le
On our Linux ppc64le test machines we quite often see compilation timeouts in the test jdk/internal/vm/Continuation/Fuzz.java. (especially when running with fastdebug binaries) So it probably makes sense to use a higher compilation timeout (maybe factor 2) on this platform. - Commit messages: - JDK-8301163 Changes: https://git.openjdk.org/jdk/pull/12244/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12244=00 Issue: https://bugs.openjdk.org/browse/JDK-8301163 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12244.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12244/head:pull/12244 PR: https://git.openjdk.org/jdk/pull/12244
ConcurrentModificationException has links to Vector, Hashtable, LinkedList
Hello. I've noticed that ConcurrentModificationException javadoc has a "See Also" section which contains links to legacy collections: Vector, Hashtable, LinkedList. https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/util/ConcurrentModificationException.html I wonder if it makes sense to replace/update this linkes to ArrayList,HashMap instead? I think it's better to "advertise" more modern collections. Andrey Turbanov
