Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 05:10:44 GMT, Ioi Lam wrote:
>> This patch makes the result of "java -Xshare:dump" deterministic:
>> - Disabled new Java threads from launching. This is harmless. See comments
>> in jvm.cpp
>> - Fixed a problem in hashtable ordering in heapShared.cpp
>> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random
>> bits. Added code to zero it.
>> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in
>> make/scripts/compare.sh
>>
>> Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic.
>> This will be fixed in
>> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
>>
>> Testing under way:
>> - tier1~tier5
>> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline,
>> windows-x86-cmp-baseline, etc).
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Fixed zero build
Hi Ioi,
some questions, comments inline.
Like David in the comments, I am also a bit vague on the usefulness, but I may
not know the whole story. Is it to enable repackagers like Debian to check the
"reproducable" tickbox on their OpenJDK package? Or is there a practical need
for this?
Thanks, Thomas
src/hotspot/share/prims/jvm.cpp line 2887:
> 2885: return;
> 2886: }
> 2887: #endif
Should we do this for jni_AttachCurrentThread too?
src/hotspot/share/utilities/hashtable.hpp line 42:
> 40:
> 41: LP64_ONLY(unsigned int _gap;)
> 42:
For 64-bit, you now lose packing potential in the theoretical case the
following payload does not have to be aligned to 64 bit. E.g. for T=char, where
the whole entry would fit into 8 bytes. Probably does not matter as long as
entries are allocated individually from C-heap which is a lot more wasteful
anyway.
For 32-bit, I think you may have the same problem if the payload starts with a
uint64_t. Would that not be aligned to a 64-bit boundary too? Whether or not
you build on 64-bit?
I think setting the memory, or at least the first 8..16 bytes, of the entry to
zero in BasicHashtable::new_entry could be more robust:
(16 bytes in case the payload starts with a long double but that may be
overthinking it :)
template BasicHashtableEntry*
BasicHashtable::new_entry(unsigned int hashValue) {
char* p = :new (NEW_C_HEAP_ARRAY(char, this->entry_size(), F);
::memset(p, 0, MIN2(this->entry_size(), 16)); // needs reproducable
BasicHashtableEntry* entry = ::new (p) BasicHashtableEntry(hashValue);
return entry;
}
If you are worried about performance, this may also be controlled by a template
parameter, and then you do it just for the system dictionary.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings [v3]
> 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 > TEST="micro:org.openjdk.be
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8282662: Revert dubious changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7729/files - new: https://git.openjdk.java.net/jdk/pull/7729/files/b31edfcd..5bbe8c4e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7729&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7729&range=00-01 Stats: 6 lines in 2 files changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7729.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7729/head:pull/7729 PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Tue, 8 Mar 2022 14:28:00 GMT, liach wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8282662: Revert dubious changes > > src/java.base/share/classes/sun/reflect/annotation/AnnotationSupport.java > line 79: > >> 77: >> containerBeforeContainee(annotations, annoClass); >> 78: >> 79: result.addAll((indirectFirst ? 0 : 1), List.of(indirect)); > > This `indirect` is most likely to be of size > 2 I've reverted this along with the rest of dubious changes - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 15:54:45 GMT, Zhengyu Gu wrote: >> Please review this small patch that fixes a potential memory leak that >> exception return fails to release allocated `cacheDirs` >> >> Test: >> >> - [x] jdk_desktop on Linux x86_64 > > Zhengyu Gu has updated the pull request incrementally with one additional > commit since the last revision: > > mrserb and aivanov-jdk's comments Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Tue, 8 Mar 2022 14:27:23 GMT, liach wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8282662: Revert dubious changes
>
> src/java.base/share/classes/java/nio/file/FileTreeIterator.java line 70:
>
>> 68: throws IOException
>> 69: {
>> 70: this.walker = new FileTreeWalker(List.of(options), maxDepth);
>
> Relates to https://bugs.openjdk.java.net/browse/JDK-8145048
Should I keep it as is or revert along with the rest of dubious changes?
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 20:47:45 GMT, Alexey Ivanov wrote: > You're right. The old icon handles in `hOldIcon` and `hOldIconSm` will be > leaked here if `CreateIconFromRaster` throws an exception. I've submitted [JDK-8282862](https://bugs.openjdk.java.net/browse/JDK-8282862): AwtWindow::SetIconData leaks old icon handles if an exception is detected - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282429: StringBuilder/StringBuffer.toString() skip compressing for UTF16 strings [v3]
On Wed, 9 Mar 2022 08:33:36 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: 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS [v4]
On Wed, 9 Mar 2022 01:33:43 GMT, Ian Graves wrote: >> Proposed change in behavior to correct inconsistencies between `\w` and `\b` >> metacharacters > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Updating with additional descriptors. Removing DataProvider import Thank you for the cleanup. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7539
Re: RFR: 8279508: Auto-vectorize Math.round API [v12]
> 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 15 commits: - 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. - 8279508: Review comments resolved.` - 8279508: Adding descriptive comments. - 8279508: Review comments resolved. - 8279508: Review comments resolved. - 8279508: Fixing for windows failure. - 8279508: Adding few descriptive comments. - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8279508 - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d07f7c76...547f4e31 - Changes: https://git.openjdk.java.net/jdk/pull/7094/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7094&range=11 Stats: 752 lines in 24 files changed: 660 ins; 30 del; 62 mod Patch: https://git.openjdk.java.net/jdk/pull/7094.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7094/head:pull/7094 PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 05:10:44 GMT, Ioi Lam wrote: >> This patch makes the result of "java -Xshare:dump" deterministic: >> - Disabled new Java threads from launching. This is harmless. See comments >> in jvm.cpp >> - Fixed a problem in hashtable ordering in heapShared.cpp >> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random >> bits. Added code to zero it. >> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in >> make/scripts/compare.sh >> >> Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. >> This will be fixed in >> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828). >> >> Testing under way: >> - tier1~tier5 >> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, >> windows-x86-cmp-baseline, etc). > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > Fixed zero build The "heap dump" aspect of this is not something I'm familiar with, but if the threads don't affect the list of classes dumped, they surely must affect what is in the heap dump otherwise their execution would not be an issue. So you must be sacrificing something by not having these threads start. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: JDK-8282798 java.lang.runtime.Carrier
On Tue, 8 Mar 2022 14:02:39 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. Note that the Object[] + int/long[] trick is pretty close to how we implemented objects in Nashorn and we got decent performance there. I'll run some jmh tests and compare against the anon class. As Brian states, it's not the implementation, it's the API. - PR: https://git.openjdk.java.net/jdk/pull/7744
[jdk18] RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
Hi all, This pull request contains a backport of commit [8eb453ba](https://github.com/openjdk/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775) from the [openjdk/jdk](https://git.openjdk.java.net/jdk) repository. The commit being backported was authored by Roman Kennke on 10 Dec 2021 and was reviewed by Roger Riggs and Peter Levart. It fixes a memory leak in ObjectStreamClass under certain conditions. See bug for details. Thanks! - Commit messages: - Backport 8eb453baebe377697286f7eb32280ca9f1fd7775 Changes: https://git.openjdk.java.net/jdk18/pull/117/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk18&pr=117&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277072 Stats: 496 lines in 4 files changed: 284 ins; 186 del; 26 mod Patch: https://git.openjdk.java.net/jdk18/pull/117.diff Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/117/head:pull/117 PR: https://git.openjdk.java.net/jdk18/pull/117
Re: Please help backport the fix with the XSLT compiler in JDK18 to JDK11 & JDK17
Note that management of the update releases are are discussed on different aliases and follow their own processes: http://mail.openjdk.java.net/mailman/listinfo/jdk-updates-dev http://openjdk.java.net/projects/jdk-updates/ -Joe On 3/7/2022 6:59 PM, Cheng Jin wrote: Hi there, I notice the issue with the XSLT compiler (https://github.com/openjdk/jdk/blob/master/src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/compiler/XSLTC.java) I raised later last year at https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/083135.html has been fixed in JDK18 via https://bugs.openjdk.java.net/browse/JDK-8276657. Could you help backport the fix to JDK11 and JDK17 give both of them share the same issue in code? Many thanks. Best Regards Cheng Jin
Re: [jdk18] RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Wed, 9 Mar 2022 13:11:27 GMT, Roman Kennke wrote: > Hi all, > > This pull request contains a backport of commit > [8eb453ba](https://github.com/openjdk/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775) > from the [openjdk/jdk](https://git.openjdk.java.net/jdk) repository. > > The commit being backported was authored by Roman Kennke on 10 Dec 2021 and > was reviewed by Roger Riggs and Peter Levart. > > It fixes a memory leak in ObjectStreamClass under certain conditions. See bug > for details. > > Thanks! Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk18/pull/117
Re: [jdk18] RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Wed, 9 Mar 2022 13:11:27 GMT, Roman Kennke wrote: > Hi all, > > This pull request contains a backport of commit > [8eb453ba](https://github.com/openjdk/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775) > from the [openjdk/jdk](https://git.openjdk.java.net/jdk) repository. > > The commit being backported was authored by Roman Kennke on 10 Dec 2021 and > was reviewed by Roger Riggs and Peter Levart. > > It fixes a memory leak in ObjectStreamClass under certain conditions. See bug > for details. > > Thanks! Oops, I believe this should have been done vs jdk18u (not jdk18). Trying again. - PR: https://git.openjdk.java.net/jdk18/pull/117
[jdk18] Withdrawn: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Wed, 9 Mar 2022 13:11:27 GMT, Roman Kennke wrote: > Hi all, > > This pull request contains a backport of commit > [8eb453ba](https://github.com/openjdk/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775) > from the [openjdk/jdk](https://git.openjdk.java.net/jdk) repository. > > The commit being backported was authored by Roman Kennke on 10 Dec 2021 and > was reviewed by Roger Riggs and Peter Levart. > > It fixes a memory leak in ObjectStreamClass under certain conditions. See bug > for details. > > Thanks! This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk18/pull/117
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Wed, 9 Mar 2022 09:37:30 GMT, Сергей Цыпанов wrote:
>> src/java.base/share/classes/java/nio/file/FileTreeIterator.java line 70:
>>
>>> 68: throws IOException
>>> 69: {
>>> 70: this.walker = new FileTreeWalker(List.of(options), maxDepth);
>>
>> Relates to https://bugs.openjdk.java.net/browse/JDK-8145048
>
> Should I keep it as is or revert along with the rest of dubious changes?
probably keep it. this can be updated in the patch for that bug.
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Wed, 9 Mar 2022 08:35:45 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282662: Revert dubious changes src/java.base/share/classes/java/lang/invoke/MethodType.java line 910: > 908: if (skipPos > myLen || myLen - skipPos > fullLen) > 909: return false; > 910: List> myList = List.of(ptypes); imo should revert this one together with that proxy parameter one - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS [v4]
On Wed, 9 Mar 2022 01:33:43 GMT, Ian Graves wrote:
>> Proposed change in behavior to correct inconsistencies between `\w` and `\b`
>> metacharacters
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updating with additional descriptors. Removing DataProvider import
src/java.base/share/classes/java/util/regex/Pattern.java line 161:
> 159: * Any character (may or may not
> match line terminators)
> 160: * id="digit">{@code \d}
> 161: * A digit: {@code [0-9]} if if
>
Looks like there is a superfluous `if` here.
-
PR: https://git.openjdk.java.net/jdk/pull/7539
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v2]
On Wed, 9 Mar 2022 02:02:51 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: > > Updated to use SHA-384 as long as the keysize permits. The `JarSigner` API is not updated. - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v12]
> I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Restructure encodeUTF8 to reduce code gen issues - Changes: - all: https://git.openjdk.java.net/jdk/pull/7231/files - new: https://git.openjdk.java.net/jdk/pull/7231/files/934b5b8a..3d155c87 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=10-11 Stats: 21 lines in 1 file changed: 8 ins; 5 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7231/head:pull/7231 PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v2]
> 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 two additional commits since the last revision: - More comments. - Allocate long fields first (alignment) - Changes: - all: https://git.openjdk.java.net/jdk/pull/7744/files - new: https://git.openjdk.java.net/jdk/pull/7744/files/ac2b6eb2..246450ca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7744&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7744&range=00-01 Stats: 95 lines in 1 file changed: 26 ins; 4 del; 65 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: 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS [v4]
On Wed, 9 Mar 2022 16:30:24 GMT, Brian Burkhalter wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updating with additional descriptors. Removing DataProvider import
>
> src/java.base/share/classes/java/util/regex/Pattern.java line 161:
>
>> 159: * Any character (may or may not
>> match line terminators)
>> 160: * > id="digit">{@code \d}
>> 161: * A digit: {@code [0-9]} if if
>>
>
> Looks like there is a superfluous `if` here.
Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/7539
Re: RFR: 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS [v5]
> Proposed change in behavior to correct inconsistencies between `\w` and `\b` > metacharacters Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Removing superfluous 'if' - Changes: - all: https://git.openjdk.java.net/jdk/pull/7539/files - new: https://git.openjdk.java.net/jdk/pull/7539/files/98fcaa9a..57fef39c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7539&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7539&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7539.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7539/head:pull/7539 PR: https://git.openjdk.java.net/jdk/pull/7539
Re: RFR: 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS [v5]
On Wed, 9 Mar 2022 17:49:11 GMT, Ian Graves wrote: >> Proposed change in behavior to correct inconsistencies between `\w` and `\b` >> metacharacters > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Removing superfluous 'if' Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7539
Re: RFR: 8264160: Regex \b is not consistent with \w without UNICODE_CHARACTER_CLASS [v5]
On Wed, 9 Mar 2022 17:55:47 GMT, Ian Graves wrote: >> Proposed change in behavior to correct inconsistencies between `\w` and `\b` >> metacharacters > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Removing superfluous 'if' LGTM. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7539
Re: RFR: 8254574: PrintWriter handling of InterruptedIOException PrintWriter handling of InterruptedIOException should be removed [v2]
> Remove reference to `java.io.InterruptedIOException` from > `java.io.PrintStream`, and make the specifications of `checkError()`, > `setError()`, and `clearError()` consistent between `java.io.PrintStream` and > `java.io.PrintWriter`. Brian Burkhalter has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8254574: PrintWriter handling of InterruptedIOException should be removed - Changes: - all: https://git.openjdk.java.net/jdk/pull/7507/files - new: https://git.openjdk.java.net/jdk/pull/7507/files/4015d9c0..95f15465 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7507&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7507&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7507/head:pull/7507 PR: https://git.openjdk.java.net/jdk/pull/7507
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v3]
> 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: Update JarSigner javadoc to make it consistent with previous update - Changes: - all: https://git.openjdk.java.net/jdk/pull/7652/files - new: https://git.openjdk.java.net/jdk/pull/7652/files/7f6fe4b5..099a6d92 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7652&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7652&range=01-02 Stats: 16 lines in 2 files changed: 0 ins; 3 del; 13 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: 8256018: Adler32/CRC32/CRC32C missing reachabilityFence [v2]
On Wed, 11 Nov 2020 11:58:08 GMT, Lance Andersen wrote:
>> Hi,
>>
>> Please review the fix for JDK-8256018 which addresses the issue that the
>> update(ByteBuffer) methods of Adler32, CRC32, and CRC32C should use
>> Reference.reachabilityFence to ensure that direct byte buffer are kept kept
>> alive when they are accessed directly.
>>
>> The Mach5 jdk-tier1, jdk-tier2, jdk-tier3 as well as the
>> jck:api/java_util/zip,jck:api/java_util/jar continue to run clean.
>>
>> Best
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove extra blank line
I wonder why this change was necessary? Did you see any concrete problems or
crashes? If yes, could you please share any details?
The code you've fixed with a `reachabilityFence` looks as follows:
if (buffer.isDirect()) {
try {
crc = updateByteBuffer(crc, ((DirectBuffer)buffer).address(),
pos, rem);
} finally {
Reference.reachabilityFence(buffer);
}
} else if (buffer.hasArray()) {
...
} else {
...
}
buffer.position(limit); // <-- 'buffer' is still alive here (at least
in the bytecode)
But as you can see, `buffer` is still alive at the end of the method. According
to @iwanowww (see [this
mail](http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051312.html)
during the ["Reduce Chance Of Mistakenly Early Backing Memory Cleanup"
discussion](http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/thread.html#51221))
this should suffice because HotSpot computes reachability based on bytecode
analysis (rather than analysis of optimized IR).
Shouldn't this be enough to keep `buffer` alive until `updateByteBuffer()` has
finished working on the buffer?
-
PR: https://git.openjdk.java.net/jdk/pull/1149
Re: RFC: JDK-8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..)
@Alan, @Lance, Sorry for my obtrusiveness, but what's your opinion on this issue? Thank you and best regards, Volker On Fri, Mar 4, 2022 at 11: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 native zlib `inflate(..)` method which > is free to change the bytes beyond `b[off+`*k*`]` (where *k* is the > number of inflated bytes). > > From a practical point of view, I don't see this as a big problem, > because callers of `InflaterInputStream::read(byte[] b, int off, int > len)` can never know how many bytes will be written into the output > buffer `b` (and in fact its content can always be completely > overwritten). It therefore makes no sense to depend on any data there > being untouched after the call. Also, having used zlib-cloudflare > productively for about two years, we haven't seen real-world issues > because of this behavior yet. However, from a specification point of > view it is easy to artificially construct a program which violates > `InflaterInputStream::read(..)`'s postcondition if using one of the > alterantive zlib implementations. A recently integrated JTreg test > (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" > fails with zlib-chromium but could be easily fixed to run with > alternative implementations as well. > > What should/can be done to solve this problem? I think we have three options: > > 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*". > > 2. Tighten the specification of `Inflater::read(byte[] b, int off, int > len)` to explicitly forbid any writes into the output array `b` beyond > the inflated bytes. > > 3. Change the implementation of `InflaterInputStream::read(..)` to > call `Inflater::read(..)` with a temporary buffer and only copy the > nu,ber of inflated bytes into `InflaterInputStream::read(..)`'s output > buffer. I think we all agree that this is only a theoretic option > because of its unacceptable performance regression. > > I personally favor option 1 but I'm interested in your opinions? > > Thank you and best regards, > Volker
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v3]
On Wed, 9 Mar 2022 19:15:36 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: > > Update JarSigner javadoc to make it consistent with previous update Sorry if my previous comment confused you, the code and javadoc are not consistent now. src/java.base/share/classes/sun/security/util/SignatureUtil.java line 561: > 559: return (isDSA || bitLength >= 624 ? "SHA384" : "SHA256"); > 560: } > 561: } In this method, "SHA-384" for 7680-bit key (7680 > 7680 is false). src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java line 439: > 437: * Specifically, if a DSA or RSA key with a key size no less > than 7680 > 438: * bits, or an EC key with a key size no less than 512 bits, > 439: * SHA-512 will be used as the hash function for the signature. In this javadoc, SHA-512 for 7680-bit key (7680 is no less than 7680). - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v12]
On Wed, 9 Mar 2022 16:52:54 GMT, Claes Redestad wrote: >> I'm requesting comments and, hopefully, some help with this patch to replace >> `StringCoding.hasNegatives` with `countPositives`. The new method does a >> very similar pass, but alters the intrinsic to return the number of leading >> bytes in the `byte[]` range which only has positive bytes. This allows for >> dealing much more efficiently with those `byte[]`s that has a ASCII prefix, >> with no measurable cost on ASCII-only or latin1/UTF16-mostly input. >> >> Microbenchmark results: >> https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 >> >> - Only implemented on x86 for now, but I want to verify that implementations >> of `countPositives` can be implemented with similar efficiency on all >> platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) >> before moving ahead. This pretty much means holding up this until it's >> implemented on all platforms, which can either contributed to this PR or as >> dependent follow-ups. >> >> - An alternative to holding up until all platforms are on board is to allow >> the implementation of `StringCoding.hasNegatives` and `countPositives` to be >> implemented so that the non-intrinsified method calls into the intrinsified. >> This requires structuring the implementations differently based on which >> intrinsic - if any - is actually implemented. One way to do this could be to >> mimic how `java.nio` handles unaligned accesses and expose which intrinsic >> is available via `Unsafe` into a `static final` field. >> >> - There are a few minor regressions (~5%) in the x86 implementation on >> `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, >> for example `encode-/decodeShortMixed` even see a minor improvement, which >> makes me consider those corner case regressions with little real world >> implications (if you have latin1 Strings, you're likely to also have >> ASCII-only strings in your mix). > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Restructure encodeUTF8 to reduce code gen issues The regressions I observe on aarch64 in `encodeLatin1Short` and a few others are not in the intrinsic itself but due changes to the surrounding code. Reverting the changes to `String.encodeUTF8` removes the regressions (but also the improvements). Seems some loop optimization is not taking place like it should - or just differently. Going back to check I see that x64 is also affected, meaning this is something that has come in when syncing up with master. I've experimented with adjusting the code to try and workaround and improve code gen, but with only partial success. I'll back out the changes to `String.encodeUTF8`, see if we can deal with the loop opt regression (separately) and return to do the `encodeUTF8` optimization later on. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v13]
> I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). Claes Redestad has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 40 commits: - Merge branch 'master' into count_positives - Restructure encodeUTF8 to reduce code gen issues - use 32-bit mask to calculate correct remainder value - ary1 not required to have USE_KILL effect - Better implementation for aarch64 returning roughly the count of positive bytes - Document that it's allowed for implementations to return values less than the exact count (iff there are negative bytes) - Clean out and remove vmIntrinsics::_hasNegatives and all related code - s390 impl provided by @RealLucy - PPC impl provided by @TheRealMDoerr - Narrow the bottom_type of CountPositivesNode (always results in a positive int value) - ... and 30 more: https://git.openjdk.java.net/jdk/compare/ff766204...30739e15 - Changes: https://git.openjdk.java.net/jdk/pull/7231/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=12 Stats: 638 lines in 36 files changed: 288 ins; 62 del; 288 mod Patch: https://git.openjdk.java.net/jdk/pull/7231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7231/head:pull/7231 PR: https://git.openjdk.java.net/jdk/pull/7231
RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10
msg drop for jdk19, Mar 9, 2022 - Commit messages: - open jdk19 l10n msg drop Changes: https://git.openjdk.java.net/jdk/pull/7765/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7765&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280400 Stats: 13775 lines in 142 files changed: 12170 ins; 1249 del; 356 mod Patch: https://git.openjdk.java.net/jdk/pull/7765.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7765/head:pull/7765 PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v3]
On Wed, 9 Mar 2022 19:44:39 GMT, Weijun Wang wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update JarSigner javadoc to make it consistent with previous update > > src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java line 439: > >> 437: * Specifically, if a DSA or RSA key with a key size no less >> than 7680 >> 438: * bits, or an EC key with a key size no less than 512 bits, >> 439: * SHA-512 will be used as the hash function for the signature. > > In this javadoc, SHA-512 for 7680-bit key (7680 is no less than 7680). Right, there are a few places which this is documented. Code and doc aren't closely coupled together plus changed course a few times... I will fix this and double check other files. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10
On Wed, 9 Mar 2022 21:09:30 GMT, Alisen Chung wrote: > msg drop for jdk19, Mar 9, 2022 `src/java.base/share/classes/sun/util/resources/CurrencyNames_de.properties` `src/java.base/share/classes/sun/util/resources/CurrencyNames_ja.properties` `src/java.base/share/classes/sun/util/resources/CurrencyNames_zh_CN.properties` These are part of `jdk.localedata` module. Should be excluded from `java.base` module and apply the diffs to `src/jdk.localedata/share/classes/sun/util/resources/ext/CurrencyNames_xx.properties` manually. (Note that the first half of it is not necessary when merging). - PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v6]
On Tue, 8 Feb 2022 22:11:34 GMT, Raffaello Giulietti wrote: >> Hello, >> >> here's a PR for a patch submitted on March 2020 >> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was >> a thing. >> >> The patch has been edited to adhere to OpenJDK code conventions about >> multi-line (block) comments. Nothing in the code proper has changed, except >> for the addition of redundant but clarifying parentheses in some expressions. >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains 12 commits: > > - 4511638: Double.toString(double) sometimes produces incorrect results > >Adapted hashes in ElementStructureTest.java > - 4511638: Double.toString(double) sometimes produces incorrect results > >Merge branch 'master' into JDK-4511638 > - 4511638: Double.toString(double) sometimes produces incorrect results > >Enhanced intervals in MathUtils. >Updated references to Schubfach v4. > - 4511638: Double.toString(double) sometimes produces incorrect results > >Merge branch 'master' into JDK-4511638 > - 4511638: Double.toString(double) sometimes produces incorrect results > >Slight adjustments to Javadoc as suggested in the JDK-8202555 (CSR) > comments. > - 4511638: Double.toString(double) sometimes produces incorrect results > >Adjusted hashes in test/langtools/tools/javac/sym/ElementStructureTest.java > - 4511638: Double.toString(double) sometimes produces incorrect results > >Merge branch 'master' into JDK-4511638 > - 4511638: Double.toString(double) sometimes produces incorrect results > - 4511638: Double.toString(double) sometimes produces incorrect results > >Refactored test classes to better match OpenJDK conventions. >Added tests recommended by Guy Steele and Paul Zimmermann. > - Changed MAX_CHARS to static > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/92f4f40d...c29dff76 Keep PR open. - PR: https://git.openjdk.java.net/jdk/pull/3402
RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver
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. - Commit messages: - JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver Changes: https://git.openjdk.java.net/jdk/pull/7766/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7766&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282776 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 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: 8281146: Replace StringCoding.hasNegatives with countPositives [v14]
> I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Revert encodeUTF8 for this PR due issues with fragile optimization - Changes: - all: https://git.openjdk.java.net/jdk/pull/7231/files - new: https://git.openjdk.java.net/jdk/pull/7231/files/30739e15..58ee73bb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=12-13 Stats: 18 lines in 1 file changed: 0 ins; 9 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/7231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7231/head:pull/7231 PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v14]
On Wed, 9 Mar 2022 23:44:22 GMT, Claes Redestad wrote: >> I'm requesting comments and, hopefully, some help with this patch to replace >> `StringCoding.hasNegatives` with `countPositives`. The new method does a >> very similar pass, but alters the intrinsic to return the number of leading >> bytes in the `byte[]` range which only has positive bytes. This allows for >> dealing much more efficiently with those `byte[]`s that has a ASCII prefix, >> with no measurable cost on ASCII-only or latin1/UTF16-mostly input. >> >> Microbenchmark results: >> https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 >> >> - Only implemented on x86 for now, but I want to verify that implementations >> of `countPositives` can be implemented with similar efficiency on all >> platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) >> before moving ahead. This pretty much means holding up this until it's >> implemented on all platforms, which can either contributed to this PR or as >> dependent follow-ups. >> >> - An alternative to holding up until all platforms are on board is to allow >> the implementation of `StringCoding.hasNegatives` and `countPositives` to be >> implemented so that the non-intrinsified method calls into the intrinsified. >> This requires structuring the implementations differently based on which >> intrinsic - if any - is actually implemented. One way to do this could be to >> mimic how `java.nio` handles unaligned accesses and expose which intrinsic >> is available via `Unsafe` into a `static final` field. >> >> - There are a few minor regressions (~5%) in the x86 implementation on >> `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, >> for example `encode-/decodeShortMixed` even see a minor improvement, which >> makes me consider those corner case regressions with little real world >> implications (if you have latin1 Strings, you're likely to also have >> ASCII-only strings in your mix). > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert encodeUTF8 for this PR due issues with fragile optimization Reverting changes to `String.encodeUTF8` brought all `encode`-micros down to effectively no change: https://jmh.morethan.io/?gist=b957cb9457c31141ac71d47f2e10486a (which proves implementing `hasNegatives` using `countPositives != len` has no measurable cost) I consider this the final version for this PR (assuming tests pass). I need someone to review the aarch64 changes in particular, and perhaps someone from the core library team should sign off on the String changes (less of those now). - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v15]
> I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Fix copyright year in new test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7231/files - new: https://git.openjdk.java.net/jdk/pull/7231/files/58ee73bb..bc5a8c80 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=13-14 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7231/head:pull/7231 PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v4]
> 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: Updated to match the latest SignatureUtil.ifcFfcStrength() impl - Changes: - all: https://git.openjdk.java.net/jdk/pull/7652/files - new: https://git.openjdk.java.net/jdk/pull/7652/files/099a6d92..f728aa7d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7652&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7652&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 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: 8280400: JDK 19 L10n resource files update - msgdrop 10
On Wed, 9 Mar 2022 21:09:30 GMT, Alisen Chung wrote: > msg drop for jdk19, Mar 9, 2022 For the bundles in java.xml: For files with Oracle copyright, update the year to 2022 and @LastModified Mar 2022. Take XPATHErrorResources_ja.java as an example, the copyright year was updated to 2021 and @LastModified: Nov 2021. That's probably the date it was last modified, but as we updating them now along with a number of other files, it would be good to be consistent and change all of them to the current date. For files with the reserved comment block such as SerializerMessages_de.java, do the same as did in XPATHErrorResources_de.java, add the copyright header and LastModified tag with the current date. For files with the Oracle GPL license such as message_zh_CN.properties, do not delete the license header. Instead, update the copyright year to 2022 if there are changes. I don't see any changes were made for many of these files, for example from msg/XMLSchemaMessages_ja.properties to regex/message_zh_CN.properties, the only change was the removal of the header. In the webrev view, some files have the word "undefined" right under the license header, hopefully once the license header is fixed, that would go away as well. - PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v27]
On Sat, 5 Mar 2022 19:06:03 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > refactor tests Sorry, the test changes look like they're heading in the wrong direction. I tried to provide some hints for what I was looking for in my previous comments. At this point, I felt it would have been too time-consuming to provide a bunch of review comments to try to get your proposed tests closer to what I was looking for, so instead I've extended/rewritten the existing `WhiteBoxResizeTest` file: https://github.com/stuart-marks/jdk/blob/JDK-8281631-smarks/test/jdk/java/util/HashMap/WhiteBoxResizeTest.java I hope you're ok with this. Here are some issues I have with the multiple-file approach: * The separate classes tends to obscure things, as the separate files are actually tightly coupled; infrastructure (reflection stuff) and test case setup (Util, TestSuite) are separated from the test, making it hard to see what's going on. * Each test method still does too much work. In general, test methods should do one thing in three phases: setup, perform action, assert results -- the [GivenWhenThen](https://martinfowler.com/bliki/GivenWhenThen.html) pattern. (Yes, I know a lot of tests currently in the regression suite violate this, but I think it's reasonable for new fine-grained combo tests like this one to be held to this standard.) Several of the test methods here do setup, perform an op, make assertions, perform another op, make assertions, perform another op, make more assertions, etc. The reason this is bad is that it's hard to separate out special cases. Consider `WhiteBoxHashMapInitialCapacityTest`. Since this test method tests both lazy allocation and initial capacity allocation, it can't handle the different behavior of `WeakHashMap`. * The need for separate classes is possibly driven by JUnit 4, where the test parameterization mechanism seems to only be available on the granularity of an entire class. (I'm not a JUnit expert, so I might have missed something.) This makes it difficult to structure parameterized tests on a per-method basis, as can be done in TestNG. The existing `WhiteBoxResizeTest` is TestNG, so it seems sensible to extend that, and add several parameterized test methods. I believe the new `WhiteBoxResizeTest` covers all the existing cases as well as the new ones in your tests, plus some additional ones that we've uncovered during this discussion. It covers a reasonable set of cases in the following areas: * combo of various constructors plus populating the map in different ways all get the same expected table size * lazy allocation of table * correct default allocation of table * verification of fix for `WeakHashMap` premature resizing * verification of fix using double instead of float calculation for certain large sizes Here are some notes about what I did. Unfortunately all that's here is the end result. I tried a bunch of experiments and different alternatives, which aren't present here, but I'll try to document some of the processes I went through to get to this result. * Rearranged the infrastructure to accommodate `WeakHashMap`. The `table()` and `capacity()` methods now work for all map classes under test. Really, there are only three classes, and two (`HashMap` and `LinkedHashMap`) have the table in the `HashMap` class, so a simple conditional will work. If additional classes are added, the conditional might turn into a case expression. * Simplified VarHandle setup. The old code had to load some private classes in order to get the exact types to provide to `findVarHandle()`. It's easier to find the field using reflection and then to "unreflect" it into a VarHandle. * Rearranged test methods into pairs of data provider and data-provider-consuming test methods, possibly with some helper methods, demarcated into different sections in the test class. Using TestNG data providers is a bit clunky with `Object[][]` or `Iterator` but it ends up working reasonably well. * The `tableSizeFor()` test is also now a data provider. It probably would have been ok to leave it as a single method with a bunch of asserts, but using a data provider enables using a tabular form for the test data, which I think makes it easier to manage the test cases. * Most test data is tabular. It's fairly straightforward but for some providers it's bit a repetitive to add more test cases. However, I didn't go the next step of compressing out the repetition, because doing so would have made it difficult to exclude special cases. For example, the `WeakHashMap` growth policy for `putAll` differs from the others. * Of note is that some of the larger sizes/capacities are tested using a "fake" map, which reports some size but whose `entrySet` iterator returns only one entry. This works for some of the im
RFR: 8058924: FileReader(String) documentation is insufficient
Add a statement to the `java.io` package documentation clarifying how a `String` representing a _pathname string_ is interpreted in the package. - Commit messages: - 8058924: FileReader(String) documentation is insufficient Changes: https://git.openjdk.java.net/jdk/pull/7767/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7767&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8058924 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7767.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7767/head:pull/7767 PR: https://git.openjdk.java.net/jdk/pull/7767
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v14]
On Sat, 5 Mar 2022 21:27:47 GMT, Marcono1234 wrote: > Would it make sense to override next(int) to always throw an exception? Even > though it should not be possible for a user to call the method on the > wrapper, it might be better to be on the safe side; for example in case a new > Random method which depends on next(int) is added in the future (even though > this is unlikely). Heh, interesting. On the one hand, it can only be called reflectively, and doing so would be an odd thing to do, so who cares. I initially thought it would NPE immediately on `seed`, but it turns out that `seed` is actually initialized by the wrapper's call to the superclass constructor. So calling `next(int)` will actually advance the random state in the superclass, quite independently of the wrapped `RandomGenerator`. So yeah maybe having `next(int)` throw UOE might be reasonable. It's good documentation for the wrapper as well. As an aside it might be reasonable to initialize `seed` to `null` in that constructor, since the seed should never be used. The NPE will fail-fast in the unlikely event that something ends up trying to use it. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v15]
On Sun, 6 Mar 2022 01:39:02 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > Update javadoc I've created a draft CSR for this change: https://bugs.openjdk.java.net/browse/JDK-8282928 Please review. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver
On Wed, 9 Mar 2022 22:52:41 GMT, Mandy Chung wrote: > A simple patch to call `Objects.requireNonNull(recv)` for an explicit null > receiver check rather than NPE thrown by `Object::getClass`. The message of > NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be > helpful but not in this case. Hi Mandy, This is an enhancement request not a bug and I don't think adding a performance penalty to all correct code just to produce a slightly clearer NPE message for broken code, is the right trade-off here. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/7766
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v6]
On Tue, 8 Feb 2022 22:11:34 GMT, Raffaello Giulietti wrote: >> Hello, >> >> here's a PR for a patch submitted on March 2020 >> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was >> a thing. >> >> The patch has been edited to adhere to OpenJDK code conventions about >> multi-line (block) comments. Nothing in the code proper has changed, except >> for the addition of redundant but clarifying parentheses in some expressions. >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains 12 commits: > > - 4511638: Double.toString(double) sometimes produces incorrect results > >Adapted hashes in ElementStructureTest.java > - 4511638: Double.toString(double) sometimes produces incorrect results > >Merge branch 'master' into JDK-4511638 > - 4511638: Double.toString(double) sometimes produces incorrect results > >Enhanced intervals in MathUtils. >Updated references to Schubfach v4. > - 4511638: Double.toString(double) sometimes produces incorrect results > >Merge branch 'master' into JDK-4511638 > - 4511638: Double.toString(double) sometimes produces incorrect results > >Slight adjustments to Javadoc as suggested in the JDK-8202555 (CSR) > comments. > - 4511638: Double.toString(double) sometimes produces incorrect results > >Adjusted hashes in test/langtools/tools/javac/sym/ElementStructureTest.java > - 4511638: Double.toString(double) sometimes produces incorrect results > >Merge branch 'master' into JDK-4511638 > - 4511638: Double.toString(double) sometimes produces incorrect results > - 4511638: Double.toString(double) sometimes produces incorrect results > >Refactored test classes to better match OpenJDK conventions. >Added tests recommended by Guy Steele and Paul Zimmermann. > - Changed MAX_CHARS to static > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/92f4f40d...c29dff76 Why is this still not merged? - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Tue, 8 Mar 2022 00:19:58 GMT, Joe Darcy wrote: > > The mapping from Location to AccessFlag(s) could be implemented event > > without using a Map. You just have to be careful not to use EnumSet for > > that (i.e. before AccessFlag enum constants are fully initialized) - an > > ArrayList is better for this case anyway. Also, conversion from > > ModuleDescriptor modifier(s) to AccessFlag(s) could be done without first > > creating a bitmask and then re-constructing a set of AccessFlag(s) from it. > > Reflective object modifiers can only be translated via bitmask, but various > > ModuleDescriptor Modifier(s) may have a direct mapping to corresponding > > AccessFlag(s). For example: > > [plevart@5a3cd8f](https://github.com/plevart/jdk/commit/5a3cd8f6851a0deae7e3e5028bba9a51d7863929) > > Yes, I made the same observation for the module-related modifiers when coding > this up; I'll look to add some API support to avoid the need to detour > through bitmasks. > > To get the public API worked out, I wanted to avoid complications in > inter-dependent class initialization. Thanks for suggesting an alternative; > I'll consider that when I resume work on this PR (need to start writing some > tests...) Thanks. On a second thought, maybe the common bitmask representation is the way to go. One one hand it must be computed from the set of module-related Modifier set(s), but then again such bitmask is very easy to cache economically (a single int field, no synchronization). When you have a bitmask at hand and a list of all possible elements (the universe), a very efficient implementation of a Set is possible (similar to EnumSet). Such AccessFlagSet instances do not need to be cached since they are just thin views over existing "permanent" structures and will, as such, typically be optimized away by JIT. Note the particularly efficient .contains() implementation which just checks two bitmasks (the flag mask and the location mask). https://github.com/plevart/jdk/commit/de9f2614da56775be4ae07c6781cbec9fbd39930 - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy 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 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/b7d02b1c...14980605 > Such AccessFlagSet ... > > [plevart@de9f261](https://github.com/plevart/jdk/commit/de9f2614da56775be4ae07c6781cbec9fbd39930) ...also needs proper hashCode/equals/toString implementations of course: https://github.com/plevart/jdk/commit/1ad5e1f925029908ecf8b21d793c25aec34a80cb - PR: https://git.openjdk.java.net/jdk/pull/7445
