Re: 8276207: Properties.loadFromXML/storeToXML works incorrectly for supplementary characters
Hi Anirvan,
The change looks good to me. Please go ahead with creating a pull
request. It would be easier for reviewers to pull the patch and look at
it. On first glance, test "testStoreWithSupplementaryCharacters" may be
a bit sensitive to file format. It may be solved with a roundtrip
(store/read) and comparing key/value of the entry, or test that the
output contains then entry element.
Thanks,
Joe
On 10/31/21 3:56 AM, Anirvan Sarkar wrote:
Hi,
Since it seems that the mailing list is scrubbing attachments, patch is
mentioned inline below.
diff a/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
b/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
--- a/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
+++ b/src/java.base/share/classes/jdk/internal/util/xml/impl/Parser.java
@@ -1,7 +1,7 @@
/*
- * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
@@ -1991,24 +1991,22 @@
case ';':
// Convert the character entity to a
character
try {
int i = Integer.parseInt(
new String(mBuff, idx + 1,
mBuffIdx - idx), 10);
-if (i >= 0x) {
-panic(FAULT);
+// Restore the buffer offset
+mBuffIdx = idx - 1;
+for(char character : Character.toChars(i))
{
+if (character == ' ' || mInp.next !=
null) {
+bappend(character, flag);
+} else {
+bappend(character);
+}
}
-ch = (char) i;
} catch (NumberFormatException nfe) {
panic(FAULT);
}
-// Restore the buffer offset
-mBuffIdx = idx - 1;
-if (ch == ' ' || mInp.next != null) {
-bappend(ch, flag);
-} else {
-bappend(ch);
-}
st = -1;
break;
case 'a':
// If the entity buffer is empty and
ch == 'x'
@@ -2032,24 +2030,22 @@
case ';':
// Convert the character entity to a
character
try {
int i = Integer.parseInt(
new String(mBuff, idx + 1,
mBuffIdx - idx), 16);
-if (i >= 0x) {
-panic(FAULT);
+// Restore the buffer offset
+mBuffIdx = idx - 1;
+for(char character : Character.toChars(i))
{
+if (character == ' ' || mInp.next !=
null) {
+bappend(character, flag);
+} else {
+bappend(character);
+}
}
-ch = (char) i;
} catch (NumberFormatException nfe) {
panic(FAULT);
}
-// Restore the buffer offset
-mBuffIdx = idx - 1;
-if (ch == ' ' || mInp.next != null) {
-bappend(ch, flag);
-} else {
-bappend(ch);
-}
st = -1;
break;
default:
panic(FAULT);
diff
a/src/java.base/share/classes/jdk/internal/util/xml/impl/XMLStreamWriterImpl.java
b/src/java.base/share/classes/jdk/internal/util/xml/impl/XMLStreamWriterImpl.java
---
a/src/java.base/share/classes/jdk/internal/util/xml/impl/XMLStreamWriterImpl.java
+++
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
Hello Alan, On 01/11/21 9:22 pm, Alan Bateman wrote: On Mon, 1 Nov 2021 03:10:45 GMT, Jaikiran Pai wrote: Can I please get a review for this change which fixes the issue reported in https://bugs.openjdk.java.net/browse/JDK-8275509? The `ModuleDescriptor.hashCode()` method uses the hash code of its various components to compute the final hash code. While doing so it ends up calling hashCode() on (collection of) various `enum` types. Since the hashCode() of enum types is generated from a call to `java.lang.Object.hashCode()`, their value isn't guaranteed to stay the same across multiple JVM runs. The commit here proposes to use the ordinal of the enum as the hash code. As Alan notes in the mailing list discussion, any changes to the ordinal of the enum (either due to addition of new value, removal of a value or just reordering existing values, all of which I think will be rare in these specific enum types) isn't expected to produce the same hash code across those changed runtimes and that should be okay. The rest of the ModuleDescriptor.hashCode() has been reviewed too and apart from calls to the enum types hashCode(), rest of the calls in that implementation, either directly or indirectly end up as calls on `java.lang.String.hashCode()` and as such aren't expected to cause non-deterministic values. A new jtreg test has been added to reproduce this issue and verify the fix. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: better method name in test class Thanks for the update to the test. Instead of launching 50 VMs, what would you think about tossing the use of ProcessTools and just changing the test description so that the test runs twice, once with the default, the second with CDS disabled, as, in: @run ModuleDescriptorHashCodeTest @run main/othervm -Xshare:off ModuleDescriptorHashCodeTest When I started off with this test, I had thought of a similar construct. However, in order to compare the hash code value generated across JVM runs (i.e. the one generated in @run 1 against the one generated in @run 2), I would need to somehow hold on to that dynamic value/result for comparison. Using the @run construct wouldn't allow me to do that. So I decided to use the ProcessTools library to have more control over it. If I missed some way to still use @run for such a test, do let me know, I'll change it accordingly. The tests are run by many people every day, they are run continuously on all platforms in several CI systems. So I think you'll get much more variability that way rather than launching 50 VMs. I have updated the PR to reduce the runs from 50 to just 2. Once with default CDS and once with CDS disabled. I also feel like the test could be re-written to test the module descriptors of all modules in the boot layer. That would avoid having a dependency on the java.sql module as it would be testing every module. I think that's a good point and will make this test case cover more modules. I've updated the PR to now cover all these boot layer modules. -Jaikiran
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v10]
> Can I please get a review for this change which fixes the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8275509? > > The `ModuleDescriptor.hashCode()` method uses the hash code of its various > components to compute the final hash code. While doing so it ends up calling > hashCode() on (collection of) various `enum` types. Since the hashCode() of > enum types is generated from a call to `java.lang.Object.hashCode()`, their > value isn't guaranteed to stay the same across multiple JVM runs. > > The commit here proposes to use the ordinal of the enum as the hash code. As > Alan notes in the mailing list discussion, any changes to the ordinal of the > enum (either due to addition of new value, removal of a value or just > reordering existing values, all of which I think will be rare in these > specific enum types) isn't expected to produce the same hash code across > those changed runtimes and that should be okay. > > The rest of the ModuleDescriptor.hashCode() has been reviewed too and apart > from calls to the enum types hashCode(), rest of the calls in that > implementation, either directly or indirectly end up as calls on > `java.lang.String.hashCode()` and as such aren't expected to cause > non-deterministic values. > > A new jtreg test has been added to reproduce this issue and verify the fix. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Test case changes: - Test all boot layer modules instead of just the java.sql module - Reduce the number of processes launched for reproducibility testing from 50 to 2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/dee4197f..3552edf0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=08-09 Stats: 74 lines in 1 file changed: 16 ins; 35 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Tue, 19 Oct 2021 20:34:55 GMT, Vamsi Parasa wrote: >> Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. >> This change show 1.87X improvement on a micro benchmark. > > Vamsi Parasa has updated the pull request incrementally with one additional > commit since the last revision: > > refactoring to remove code duplication by using a common routine for > UMulHiLNode and MulHiLNode Thank you for spotting the stale comment. It will removed in another related commit that will be pushed soon... - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275766: (tz) Update Timezone Data to 2021e
On Thu, 28 Oct 2021 01:02:27 GMT, Yoshiki Sato wrote: > Please review the integration of tzdata2021e (including tzdata2021d) to the > JDK. > The fix has passed all relevant JTREG regression tests and JCK tests. > > 8275754: (tz) Update Timezone Data to 2021d > 8275849: TestZoneInfo310.java fails with tzdata2021e @coffeys please complete your review on this PR. - PR: https://git.openjdk.java.net/jdk/pull/6144
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Mon, 1 Nov 2021 12:05:32 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 17 commits:
>
> - Add cache for memory address var handles
> - Merge branch 'master' into JEP-419
> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
> - Merge branch 'master' into JEP-419
> - Fix copyright header in TestArrayCopy
> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
> - * use `invokeWithArguments` to simplify new test
> - Add test for liveness check with high-aririty downcalls
>(make sure that if an exception occurs in a downcall because of liveness,
>ref count of other resources are left intact).
> - * Fix javadoc issue in VaList
>* Fix bug in concurrent logic for shared scope acquire
> - Address review comments
> - ... and 7 more:
> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line
111:
> 109: class VarHandleCache {
> 110: private static final Map handleMap =
> new ConcurrentHashMap<>();
> 111: private static final Map
> handleMapNoAlignCheck = new ConcurrentHashMap<>();
Something to consider later if this is an issue. Since the number of
`ValueLayout` instances is fixed, carrier x order = 18, we can use stable
arrays with ordinals on the instances.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
On Mon, 1 Nov 2021 16:10:26 GMT, Ichiroh Takiguchi wrote: >> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13. >> After JDK18-b13, javac and some other langtool command's usage were garbled >> on Japanese Windows. >> These commands use PrintWriter instead of standard out/err with PrintStream. > > Ichiroh Takiguchi has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - Langtools command's usage were grabled on Japanese Windows I needed to rebase my fixed code since JavapTask.java and JdepsTask.java were updated. Hello @jonathan-gibbons . Could you check new fixes ? I changed following parts: * Native charset is generated on com/sun/tools/javac/util/Log.java. * module-info.java was updated Hello @naotoj . Could you check new fixes again ? - PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v2]
On Mon, 1 Nov 2021 15:35:36 GMT, Aleksey Shipilev wrote: >> This blocks JDK-8276215: `StrictMath` intrinsics are handled peculiarly by >> giving failing intrinsics a second chance to match against the similar >> `Math` intrinsics. This has interesting consequence for matchers: we can >> match the native `StrictMath.sqrt` to non-native intrinsic for `Math.sqrt`. >> Interpreter would then have to disambiguate the two. It could be made >> simpler and more consistent. >> >> For `min`/`max` methods, `StrictMath` already delegates to `Math` methods, >> so we can just drop the intrinsics for them. `sqrt` is harder to delegate, >> because it is `native` and a part of public API, so we can instead do the >> proper special intrinsic for it. >> >> There seem to be no performance regressions with this patch at least on >> Linux x86_64: >> >> >> $ CONF=linux-x86_64-server-release make test TEST="micro:StrictMathBench" >> >> Benchmark Mode Cnt Score Error Units >> >> ### Before >> >> StrictMathBench.minDouble thrpt4 230921.558 ± 234.238 ops/ms >> StrictMathBench.minFloat thrpt4 230932.303 ± 126.721 ops/ms >> StrictMathBench.minInt thrpt4 230917.256 ± 73.008 ops/ms >> StrictMathBench.minLongthrpt4 194460.828 ± 178.079 ops/ms >> >> >> StrictMathBench.maxDouble thrpt4 230983.180 ± 161.211 ops/ms >> StrictMathBench.maxFloat thrpt4 230969.290 ± 277.500 ops/ms >> StrictMathBench.maxInt thrpt4 231033.581 ± 200.015 ops/ms >> StrictMathBench.maxLongthrpt4 194590.744 ± 114.295 ops/ms >> >> >> StrictMathBench.sqrtDouble thrpt4 230722.037 ± .080 ops/ms >> >> ### After >> >> StrictMathBench.minDouble thrpt4 230976.625 ± 67.338 ops/ms >> StrictMathBench.minFloat thrpt4 230896.021 ± 270.434 ops/ms >> StrictMathBench.minInt thrpt4 230859.741 ± 403.147 ops/ms >> StrictMathBench.minLongthrpt4 194456.673 ± 111.557 ops/ms >> >> StrictMathBench.maxDouble thrpt4 230890.776 ± 89.924 ops/ms >> StrictMathBench.maxFloat thrpt4 230918.334 ± 63.160 ops/ms >> StrictMathBench.maxInt thrpt4 231059.128 ± 51.224 ops/ms >> StrictMathBench.maxLongthrpt4 194488.210 ± 495.224 ops/ms >> >> StrictMathBench.sqrtDouble thrpt4 231023.703 ± 247.330 ops/ms >> >> >> Additional testing: >> - [x] `StrictMath` benchmarks >> - [x] Linux x86_64 fastdebug `tier1` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Touchups Yes, I am fine with new intrinsics for them. - PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:51:36 GMT, Pavel Rappo wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Use the blessed modifiers order The test job that has been scheduled previously has completed successfully. - PR: https://git.openjdk.java.net/jdk/pull/6191
Integrated: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote: > Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. This pull request has now been integrated. Changeset: 2eafa036 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/2eafa036c03d3e8f3dba8f67dd37b484874dc3d3 Stats: 13 lines in 3 files changed: 0 ins; 1 del; 12 mod 8276234: Trivially clean up locale-related code Reviewed-by: redestad, naoto, iris - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]
On Mon, 1 Nov 2021 22:27:08 GMT, Claes Redestad wrote: >> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java >> line 3269: >> >>> 3267: return false; >>> 3268: } >>> 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in >>> an int and can't be negative >> >> Unfortunately, this is not a valid assumption, and it affects the logic of >> the optimization more generally (methods where non-negative is assumed). >> >> Basically, NANO_OF_SECOND (and all other fields in the formatter) can have >> any `long` value. Despite immediate appearances, the value is not limited to >> 0 to 999,999,999. This is because you are allowed to create an >> implementation of `Temporal` that returns values outside that range. No such >> class exists in the JDK, but such a class would be perfectly legal. As a >> related example, it would be perfectly value to write a time class that ran >> from 03:00 to 26:59 each day, thus HOUROF_DAY cannot be assumed by the >> formatter to be between 0 and 23. > > The commentary on this line could probably be improved, but this is in a > private printer-parser that will only be used for NANO_OF_SECOND and not any > arbitrary `TemporalField` (see line 704), thus I fail to see how this > assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to > 999,999,999). > > I considered writing a more generic integral-fraction printer parser that > would optimize for any value-range that fits in an int, but seeing how > NANO_OF_SECOND is likely the only one used in practice and with a high demand > for better efficiency I opted to specialize for it more directly. I see what you're saying that an arbitrary `Temporal` could define its own fields with its own ranges, but I would consider it a design bug if such an implementation at a whim redefines the value ranges of well-defined constants such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a `Temporal` would have to define its own enumeration of allowed `TemporalField`s. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]
> Prompted by a request from Volkan Yazıcı I took a look at why the java.time > formatters are less efficient for some common patterns than custom formatters > in apache-commons and log4j. This patch reduces the gap, without having > looked at the third party implementations. > > When printing times: > - Avoid turning integral values into `String`s before appending them to the > buffer > - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of > `BigDecimal` > > This means a speed-up and reduction in allocations when formatting almost any > date or time pattern, and especially so when including sub-second parts > (`S-S`). > > Much of the remaining overhead can be traced to the need to create a > `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` > internally. We could likely also win performance by specializing some common > patterns. > > Testing: tier1-3 Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove stray method - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/9e97b4dc..ee13139a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=00-01 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak javadoc of loaderLookup - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/17f45861..7cf4fcd9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=10-11 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8276188: Clarify "default charset" descriptions in String class
On Mon, 1 Nov 2021 18:31:17 GMT, Naoto Sato wrote: > This is a leftover document fix to `String` class for the JEP 400. > Corresponding CSR has also been drafted: > https://bugs.openjdk.java.net/browse/JDK-8276238 Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6198
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter
On Mon, 1 Nov 2021 22:18:52 GMT, Stephen Colebourne wrote: >> Prompted by a request from Volkan Yazıcı I took a look at why the java.time >> formatters are less efficient for some common patterns than custom >> formatters in apache-commons and log4j. This patch reduces the gap, without >> having looked at the third party implementations. >> >> When printing times: >> - Avoid turning integral values into `String`s before appending them to the >> buffer >> - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of >> `BigDecimal` >> >> This means a speed-up and reduction in allocations when formatting almost >> any date or time pattern, and especially so when including sub-second parts >> (`S-S`). >> >> Much of the remaining overhead can be traced to the need to create a >> `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` >> internally. We could likely also win performance by specializing some common >> patterns. >> >> Testing: tier1-3 > > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > line 3269: > >> 3267: return false; >> 3268: } >> 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in >> an int and can't be negative > > Unfortunately, this is not a valid assumption, and it affects the logic of > the optimization more generally (methods where non-negative is assumed). > > Basically, NANO_OF_SECOND (and all other fields in the formatter) can have > any `long` value. Despite immediate appearances, the value is not limited to > 0 to 999,999,999. This is because you are allowed to create an implementation > of `Temporal` that returns values outside that range. No such class exists in > the JDK, but such a class would be perfectly legal. As a related example, it > would be perfectly value to write a time class that ran from 03:00 to 26:59 > each day, thus HOUROF_DAY cannot be assumed by the formatter to be between 0 > and 23. The commentary on this line could probably be improved, but this is in a private printer-parser that will only be used for NANO_OF_SECOND and not any arbitrary `Temporal` (see line 704), thus I fail to see how this assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to 999,999,999). I considered writing a more generic integral-fraction printer parser that would optimize for any value-range that fits in an int, but seeing how NANO_OF_SECOND is likely the only one used in practice and with a high demand for better efficiency I opted to specialize for it more directly. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter
On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad wrote: > Prompted by a request from Volkan Yazıcı I took a look at why the java.time > formatters are less efficient for some common patterns than custom formatters > in apache-commons and log4j. This patch reduces the gap, without having > looked at the third party implementations. > > When printing times: > - Avoid turning integral values into `String`s before appending them to the > buffer > - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of > `BigDecimal` > > This means a speed-up and reduction in allocations when formatting almost any > date or time pattern, and especially so when including sub-second parts > (`S-S`). > > Much of the remaining overhead can be traced to the need to create a > `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` > internally. We could likely also win performance by specializing some common > patterns. > > Testing: tier1-3 Changes requested by scolebourne (Author). src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3269: > 3267: return false; > 3268: } > 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in > an int and can't be negative Unfortunately, this is not a valid assumption, and it affects the logic of the optimization more generally (methods where non-negative is assumed). Basically, NANO_OF_SECOND (and all other fields in the formatter) can have any `long` value. Despite immediate appearances, the value is not limited to 0 to 999,999,999. This is because you are allowed to create an implementation of `Temporal` that returns values outside that range. No such class exists in the JDK, but such a class would be perfectly legal. As a related example, it would be perfectly value to write a time class that ran from 03:00 to 26:59 each day, thus HOUROF_DAY cannot be assumed by the formatter to be between 0 and 23. - PR: https://git.openjdk.java.net/jdk/pull/6188
New candidate JEP: 421: Deprecate Finalization for Removal
https://openjdk.java.net/jeps/421 Summary: Deprecate finalization for removal in a future release. Finalization remains enabled by default for now, but can be disabled to facilitate early testing. In a future release it will be disabled by default, and in a later release it will be removed. Maintainers of libraries and applications that rely upon finalization should consider migrating to other resource management techniques such as the try-with-resources statement and cleaners. - Mark
Re: RFR: 8276188: Clarify "default charset" descriptions in String class
On Mon, 1 Nov 2021 18:31:17 GMT, Naoto Sato wrote: > This is a leftover document fix to `String` class for the JEP 400. > Corresponding CSR has also been drafted: > https://bugs.openjdk.java.net/browse/JDK-8276238 Diffs align with the corresponding CSR, which I have also reviewed. - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6198
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v2]
On Mon, 1 Nov 2021 18:44:53 GMT, Vladimir Kozlov wrote: > Removing intrinsics for StrictMatch `min/max` methods may prevent them from > inlining if they are not hot when caller is compiled. Would you like me to leave them instead? That would mean we introduce these new intrinsic definitions: /* StrictMath intrinsics, similar to what we have in Math. */ \ do_intrinsic(_min_strict, java_lang_StrictMath, min_name, int2_int_signature,F_S) \ do_intrinsic(_max_strict, java_lang_StrictMath, max_name, int2_int_signature,F_S) \ do_intrinsic(_minF_strict, java_lang_StrictMath, min_name, float2_float_signature,F_S) \ do_intrinsic(_maxF_strict, java_lang_StrictMath, max_name, float2_float_signature,F_S) \ do_intrinsic(_minD_strict, java_lang_StrictMath, min_name, double2_double_signature, F_S) \ do_intrinsic(_maxD_strict, java_lang_StrictMath, max_name, double2_double_signature, F_S) \ /* Special flavor of dsqrt intrinsic to handle the "native" method in StrictMath. Otherwise the same as in Math. */ \ do_intrinsic(_dsqrt_strict, java_lang_StrictMath, sqrt_name, double_double_signature, F_SN) \ - PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v2]
On Mon, 1 Nov 2021 15:35:36 GMT, Aleksey Shipilev wrote: >> This blocks JDK-8276215: `StrictMath` intrinsics are handled peculiarly by >> giving failing intrinsics a second chance to match against the similar >> `Math` intrinsics. This has interesting consequence for matchers: we can >> match the native `StrictMath.sqrt` to non-native intrinsic for `Math.sqrt`. >> Interpreter would then have to disambiguate the two. It could be made >> simpler and more consistent. >> >> For `min`/`max` methods, `StrictMath` already delegates to `Math` methods, >> so we can just drop the intrinsics for them. `sqrt` is harder to delegate, >> because it is `native` and a part of public API, so we can instead do the >> proper special intrinsic for it. >> >> There seem to be no performance regressions with this patch at least on >> Linux x86_64: >> >> >> $ CONF=linux-x86_64-server-release make test TEST="micro:StrictMathBench" >> >> Benchmark Mode Cnt Score Error Units >> >> ### Before >> >> StrictMathBench.minDouble thrpt4 230921.558 ± 234.238 ops/ms >> StrictMathBench.minFloat thrpt4 230932.303 ± 126.721 ops/ms >> StrictMathBench.minInt thrpt4 230917.256 ± 73.008 ops/ms >> StrictMathBench.minLongthrpt4 194460.828 ± 178.079 ops/ms >> >> >> StrictMathBench.maxDouble thrpt4 230983.180 ± 161.211 ops/ms >> StrictMathBench.maxFloat thrpt4 230969.290 ± 277.500 ops/ms >> StrictMathBench.maxInt thrpt4 231033.581 ± 200.015 ops/ms >> StrictMathBench.maxLongthrpt4 194590.744 ± 114.295 ops/ms >> >> >> StrictMathBench.sqrtDouble thrpt4 230722.037 ± .080 ops/ms >> >> ### After >> >> StrictMathBench.minDouble thrpt4 230976.625 ± 67.338 ops/ms >> StrictMathBench.minFloat thrpt4 230896.021 ± 270.434 ops/ms >> StrictMathBench.minInt thrpt4 230859.741 ± 403.147 ops/ms >> StrictMathBench.minLongthrpt4 194456.673 ± 111.557 ops/ms >> >> StrictMathBench.maxDouble thrpt4 230890.776 ± 89.924 ops/ms >> StrictMathBench.maxFloat thrpt4 230918.334 ± 63.160 ops/ms >> StrictMathBench.maxInt thrpt4 231059.128 ± 51.224 ops/ms >> StrictMathBench.maxLongthrpt4 194488.210 ± 495.224 ops/ms >> >> StrictMathBench.sqrtDouble thrpt4 231023.703 ± 247.330 ops/ms >> >> >> Additional testing: >> - [x] `StrictMath` benchmarks >> - [x] Linux x86_64 fastdebug `tier1` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Touchups Removing intrinsics for StrictMatch `min/max` methods may prevent them from inlining if they are not hot when caller is compiled. - PR: https://git.openjdk.java.net/jdk/pull/6184
Re: [External] : Re: JDK-8275509: (jlink) SystemModulesPlugin generates a jdk.internal.module.SystemModules$all.class which isn't reproducible
Hi Jaikiran,
Thanks for writing the test case to explore the problems in this area.
Please see my comments below:
On 10/29/21 5:55 AM, Jaikiran Pai wrote:
Hello Ioi (and others),
On 22/10/21 1:39 pm, Jaikiran Pai wrote:
Hello Ioi,
On 22/10/21 12:29 pm, Ioi Lam wrote:
On 10/21/21 9:09 PM, Jaikiran Pai wrote:
Hello Ioi,
This is my initial analysis of the problem.
>>> Can anyone think of similar problems that may happen
elsewhere?
The static constructors of enum classes are executed at both CDS
dump time and run time. E.g.,
public enum Modifier {
OPEN
}
The method essentially does this:
public static final Modifier OPEN = new Modifier("OPEN");
If a reference of Modifier.OPEN is stored inside the CDS archived
heap during dump time, it will be different than the value of
Modifier.OPEN that is re-created at runtime by the execution of
Modifier.
I have almost next to nothing knowledge about CDS internals. My
only understanding of it is based on some documentation that I have
read. One of them being this one
https://docs.oracle.com/en/java/javase/17/vm/class-data-sharing.html#GUID-7EAA3411-8CF0-4D19-BD05-DF5E1780AA91.
Based on that documentation (and other similar ones), it was my
understanding that CDS was meant to store/share class "metadata"
like it states in that doc:
"When the JVM starts, the shared archive is memory-mapped to allow
sharing of read-only JVM metadata for these classes among multiple
JVM processes."
But from what you explain in that enum example, it looks like it
also stores class instance data that is computed at build time on
the host where the JDK image was generated? Did I understand it
correctly? Is this only for enums or does it also store the static
initialization data of "class" types too? If it does store the
static init data of class types too, then wouldn't such data be
host/build time specific and as such the classes that need to be
enrolled into the default CDS archive of the JDK should be very
selective (by reviewing what they do in their static init)? Like I
said, I haven't looked into this in detail so perhaps it already is
selective in the JDK build?
Hi Jaikiran,
Thank you very much for the detailed response.
CDS also has the ability to archive Java heap object. Since
https://bugs.openjdk.java.net/browse/JDK-8244778 , we have archived
the entire module graph to improve start-up time. At run time, the
module graph (as well as other archived heap objects) are loaded
from the CDS archive and put into the Java heap (either through
memory mapping or copying).
That is interesting and something that I hadn't known.
You can see the related code in
jdk.internal.module.ModuleBootstrap::boot()
I just had a look at it and it's quite elaborate and it'll take a me
while to fully grasp it (if at all) given its understandable complexity.
When the module system has started up, the module graph will
reference a copy of the OPEN enum object that was created as part of
the archive. However, the Modifier. will also be executed at
VM start-up, causing a second copy of the OPEN enum object to be
stored into the static field Modified::OPEN.
Thank you for that detail. That helps me understand this a bit more
(and opens a few questions). To be clear - the VM startup code which
creates that other copy, ends up creating that copy because that
piece of initialization happens even before the module system has
fully started up and created those references from the archived
state? Otherwise, the classloaders I believe would be smart enough to
not run that static init again, had the module system with that graph
from the archived state been fully "up"?
So would this mean that this not just impacts enums but essentially
every class referenced within the module system (of just boot layer?)
that has state which is initialized during static init? To be more
precise, consider the very common example of loggers which are
typically static initialized and stored in a static (final) field:
private static final java.util.logger.Logger logger =
Logger.getLogger(SomeClass.class);
If the boot layer module graph has any classes which has state like
this, it would then mean that if such classes do get initialized very
early on during VM startup, then they too are impacted and the module
graph holding instances of such classes will end up using a different
instance for such fields as compared to the rest of the application
code?
In essence, such classes which get accessed early (before module
system with the archived graph is "up") during VM startup can end up
_virtually_ having their static initialization run twice (I
understand it won't be run twice, but that's the end result, isn't it)?
I was really curious why this was only applicable to enums and why
other static initialization (like the one I explained above) wasn't
impacted. So I decided to do a small PoC. To come up with an
RFR: 8276188: Clarify "default charset" descriptions in String class
This is a leftover document fix to `String` class for the JEP 400. Corresponding CSR has also been drafted: https://bugs.openjdk.java.net/browse/JDK-8276238 - Commit messages: - 8276188: Clarify "default charset" descriptions in String class Changes: https://git.openjdk.java.net/jdk/pull/6198/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6198=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276188 Stats: 16 lines in 1 file changed: 2 ins; 2 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/6198.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6198/head:pull/6198 PR: https://git.openjdk.java.net/jdk/pull/6198
Re: RFR: 8252990: Intrinsify Unsafe.storeStoreFence [v2]
On Thu, 28 Oct 2021 08:58:48 GMT, Aleksey Shipilev wrote: >> `Unsafe.storeStoreFence` currently delegates to stronger >> `Unsafe.storeFence`. We can teach compilers to map this directly to already >> existing rules that handle `MemBarStoreStore`. Like explicit >> `LoadFence`/`StoreFence`, we introduce the special node to differentiate >> explicit fence and implicit store-store barriers. `storeStoreFence` is >> usually used to simulate safe `final`-field like constructions in special >> JDK classes, like `ConstantCallSite` and friends. >> >> Motivational performance difference on benchmarks from JDK-8276054 on ARM32 >> (Raspberry Pi 4): >> >> >> Benchmark Mode Cnt ScoreError Units >> Multiple.plain avgt3 2.669 ± 0.004 ns/op >> Multiple.release avgt3 16.688 ± 0.057 ns/op >> Multiple.storeStoreavgt3 14.021 ± 0.144 ns/op // Better >> >> MultipleWithLoads.plainavgt3 4.672 ± 0.053 ns/op >> MultipleWithLoads.release avgt3 16.689 ± 0.044 ns/op >> MultipleWithLoads.storeStore avgt3 14.012 ± 0.010 ns/op // Better >> >> MultipleWithStores.plain avgt3 14.687 ± 0.009 ns/op >> MultipleWithStores.release avgt3 45.393 ± 0.192 ns/op >> MultipleWithStores.storeStore avgt3 38.048 ± 0.033 ns/op // Better >> >> Publishing.plain avgt3 27.079 ± 0.201 ns/op >> Publishing.release avgt3 27.088 ± 0.241 ns/op >> Publishing.storeStore avgt3 27.009 ± 0.259 ns/op // Within >> error, hidden by allocation >> >> Single.plain avgt3 2.670 ± 0.002 ns/op >> Single.releaseFenceavgt3 6.675 ± 0.001 ns/op >> Single.storeStoreFence avgt3 8.012 ± 0.027 ns/op // Worse, >> seems to be ARM32 implementation artifact >> >> >> The same thing on AArch64 (Raspberry Pi 3): >> >> >> Benchmark Mode Cnt Score Error Units >> >> Multiple.plain avgt3 5.914 ± 0.115 ns/op >> Multiple.release avgt3 10.149 ± 0.059 ns/op >> Multiple.storeStoreavgt3 6.757 ± 0.138 ns/op // Better >> >> MultipleWithLoads.plainavgt3 11.849 ± 0.331 ns/op >> MultipleWithLoads.release avgt3 35.565 ± 1.144 ns/op >> MultipleWithLoads.storeStore avgt3 19.441 ± 0.471 ns/op // Better >> >> MultipleWithStores.plain avgt3 5.920 ± 0.213 ns/op >> MultipleWithStores.release avgt3 20.286 ± 0.347 ns/op >> MultipleWithStores.storeStore avgt3 12.686 ± 0.230 ns/op // Better >> >> Publishing.plain avgt3 22.261 ± 1.630 ns/op >> Publishing.release avgt3 22.269 ± 0.576 ns/op >> Publishing.storeStore avgt3 17.464 ± 0.397 ns/op // Better >> >> Single.plain avgt3 5.916 ± 0.063 ns/op >> Single.release avgt3 10.148 ± 0.401 ns/op >> Single.storeStore avgt3 6.767 ± 0.164 ns/op // Better >> >> >> As expected, this does not affect x86_64 at all, because both `release` and >> `storeStore` are effectively no-ops, only affecting compiler optimizations: >> >> >> Benchmark Mode Cnt Score Error Units >> >> Multiple.plain avgt3 0.406 ± 0.002 ns/op >> Multiple.release avgt3 0.409 ± 0.018 ns/op >> Multiple.storeStoreavgt3 0.406 ± 0.001 ns/op >> >> MultipleWithLoads.plainavgt3 4.328 ± 0.006 ns/op >> MultipleWithLoads.release avgt3 4.600 ± 0.014 ns/op >> MultipleWithLoads.storeStore avgt3 4.602 ± 0.006 ns/op >> >> MultipleWithStores.plain avgt3 0.812 ± 0.001 ns/op >> MultipleWithStores.release avgt3 0.812 ± 0.002 ns/op >> MultipleWithStores.storeStore avgt3 0.812 ± 0.002 ns/op >> >> Publishing.plain avgt3 6.370 ± 0.059 ns/op >> Publishing.release avgt3 6.358 ± 0.436 ns/op >> Publishing.storeStore avgt3 6.367 ± 0.054 ns/op >> >> Single.plain avgt3 0.407 ± 0.039 ns/op >> Single.releaseFenceavgt3 0.406 ± 0.001 ns/op >> Single.storeStoreFence avgt3 0.406 ± 0.001 ns/op >> >> >> Additional testing: >> - [x] Linux x86_64 fastdebug `tier1` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Fix the comment to match JDK-8276096 Finally revived my quiet AArch64 dev board, added AArch64 results, which are even better than ARM32. Updated PR with perf results. - PR: https://git.openjdk.java.net/jdk/pull/6136
Re: RFR: JDK-8276236: Table headers missing in Formatter api docs
On Mon, 1 Nov 2021 15:59:22 GMT, Ludvig Janiuk wrote: > Adds table headers to all tables in Formatter api docs. I inferred the header > "Unicode" for the columns that contained unicode > codepoints. > > Formatter api docs: > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Formatter.html Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6195
RFR: JDK-8276239: Better tables in java.util.random package summary
The tables are now striped, and they use row headers (which is a nice-to-have for accessibility). - Commit messages: - Striped tables and table row headers Changes: https://git.openjdk.java.net/jdk/pull/6196/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6196=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276239 Stats: 32 lines in 1 file changed: 0 ins; 0 del; 32 mod Patch: https://git.openjdk.java.net/jdk/pull/6196.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6196/head:pull/6196 PR: https://git.openjdk.java.net/jdk/pull/6196
Re: RFR: JDK-8276239: Better tables in java.util.random package summary
On Mon, 1 Nov 2021 17:10:56 GMT, Ludvig Janiuk wrote: > The tables are now striped, and they use row headers (which is a nice-to-have > for accessibility). Marked as reviewed by jlaskey (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6196
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:51:36 GMT, Pavel Rappo wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Use the blessed modifiers order Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: JDK-8276236: Table headers missing in Formatter api docs
On Mon, 1 Nov 2021 15:59:22 GMT, Ludvig Janiuk wrote: > Adds table headers to all tables in Formatter api docs. I inferred the header > "Unicode" for the columns that contained unicode > codepoints. > > Formatter api docs: > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Formatter.html LGTM - Marked as reviewed by coffeys (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6195
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v11]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix liveness issue with loader lookups - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/9b519343..17f45861 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=09-10 Stats: 191 lines in 6 files changed: 187 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:25:26 GMT, Pavel Rappo wrote: >> src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: >> >>> 246: private static final LocaleDataStrategy INSTANCE = new >>> LocaleDataStrategy(); >>> 247: // TODO: avoid hard-coded Locales >>> 248: private final static Set JAVA_BASE_LOCALES >> >> Canonical modifier order is `static final` > > It turns out that my IDE was configured NOT to highlight missorted modifiers. > Once I reconfigured it, I saw these cases and some other in that same file. > I'll fix them all if that's okay. > > My recollection is that there have been campaigns on using "the blessed > modifiers order". It might be that since the last such crusade the modifiers > have gone out of hand, and we might need to re-bless them :-) Fixed in 2b7e0c6. - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
> Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Use the blessed modifiers order - Changes: - all: https://git.openjdk.java.net/jdk/pull/6191/files - new: https://git.openjdk.java.net/jdk/pull/6191/files/8e2815b8..2b7e0c68 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6191=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6191=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6191/head:pull/6191 PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:23:49 GMT, Claes Redestad wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: > >> 246: private static final LocaleDataStrategy INSTANCE = new >> LocaleDataStrategy(); >> 247: // TODO: avoid hard-coded Locales >> 248: private final static Set JAVA_BASE_LOCALES > > Canonical modifier order is `static final` It turns out that my IDE was configured NOT to highlight missorted modifiers. Once I reconfigured it, I saw these cases and some other in that same file. I'll fix them all if that's okay. My recollection is that there have been campaigns on using "the blessed modifiers order". It might be that since the last such crusade the modifiers have gone out of hand, and we might need to re-bless them :-) - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:52:51 GMT, Daniel Fuchs wrote:
>> Please review this PR. A comprehensive test job has been scheduled; I'll
>> notify this thread once that job has completed.
>
> src/java.base/share/classes/sun/util/resources/LocaleData.java line 336:
>
>> 334: public List getCandidateLocales(String baseName, Locale
>> locale) {
>> 335: // Specify only the given locale
>> 336: return List.of(locale);
>
> Is it guaranteed that `locale` is not `null` here?
This is a good question. As far I can see, the only call site checks for null
explicitly:
https://github.com/openjdk/jdk/blob/a4c46e1e4f4f2f05c8002b2af683a390fc46b424/src/java.base/share/classes/sun/util/resources/Bundles.java#L113
So the answer is yes.
-
PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote: > Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13. > After JDK18-b13, javac and some other langtool command's usage were garbled > on Japanese Windows. > These commands use PrintWriter instead of standard out/err with PrintStream. Ichiroh Takiguchi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - 8274544: Langtools command's usage were garbled on Japanese Windows - 8274544: Langtools command's usage were garbled on Japanese Windows - 8274544: Langtools command's usage were garbled on Japanese Windows - 8274544: Langtools command's usage were garbled on Japanese Windows - Langtools command's usage were grabled on Japanese Windows - Changes: https://git.openjdk.java.net/jdk/pull/5771/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5771=04 Stats: 104 lines in 17 files changed: 74 ins; 0 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/5771.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5771/head:pull/5771 PR: https://git.openjdk.java.net/jdk/pull/5771
RFR: JDK-8276236: Table headers missing in Formatter api docs
Adds table headers to all tables in Formatter api docs. I inferred the header "Unicode" for the columns that contained unicode codepoints. Formatter api docs: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Formatter.html - Commit messages: - Fixes JDK-8276236 Changes: https://git.openjdk.java.net/jdk/pull/6195/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6195=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276236 Stats: 80 lines in 1 file changed: 80 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6195.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6195/head:pull/6195 PR: https://git.openjdk.java.net/jdk/pull/6195
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote:
> Please review this PR. A comprehensive test job has been scheduled; I'll
> notify this thread once that job has completed.
src/java.base/share/classes/sun/util/resources/LocaleData.java line 336:
> 334: public List getCandidateLocales(String baseName, Locale
> locale) {
> 335: // Specify only the given locale
> 336: return List.of(locale);
Is it guaranteed that `locale` is not `null` here?
-
PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
On Mon, 1 Nov 2021 03:10:45 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change which fixes the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8275509? >> >> The `ModuleDescriptor.hashCode()` method uses the hash code of its various >> components to compute the final hash code. While doing so it ends up calling >> hashCode() on (collection of) various `enum` types. Since the hashCode() of >> enum types is generated from a call to `java.lang.Object.hashCode()`, their >> value isn't guaranteed to stay the same across multiple JVM runs. >> >> The commit here proposes to use the ordinal of the enum as the hash code. As >> Alan notes in the mailing list discussion, any changes to the ordinal of the >> enum (either due to addition of new value, removal of a value or just >> reordering existing values, all of which I think will be rare in these >> specific enum types) isn't expected to produce the same hash code across >> those changed runtimes and that should be okay. >> >> The rest of the ModuleDescriptor.hashCode() has been reviewed too and apart >> from calls to the enum types hashCode(), rest of the calls in that >> implementation, either directly or indirectly end up as calls on >> `java.lang.String.hashCode()` and as such aren't expected to cause >> non-deterministic values. >> >> A new jtreg test has been added to reproduce this issue and verify the fix. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > better method name in test class Thanks for the update to the test. Instead of launching 50 VMs, what would you think about tossing the use of ProcessTools and just changing the test description so that the test runs twice, once with the default, the second with CDS disabled, as, in: @run ModuleDescriptorHashCodeTest @run main/othervm -Xshare:off ModuleDescriptorHashCodeTest The tests are run by many people every day, they are run continuously on all platforms in several CI systems. So I think you'll get much more variability that way rather than launching 50 VMs. I also feel like the test could be re-written to test the module descriptors of all modules in the boot layer. That would avoid having a dependency on the java.sql module as it would be testing every module. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v2]
On Mon, 1 Nov 2021 13:08:05 GMT, Andrew Haley wrote: > So we have _dsqrt and_dsqrt_strict, which must be functionally identical, but > we provide both names because they're part of a public API. I think this > deserves an explanatory comment in the code. Yes, no problem, added comment near intrinsic definition. - PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v2]
> This blocks JDK-8276215: `StrictMath` intrinsics are handled peculiarly by > giving failing intrinsics a second chance to match against the similar `Math` > intrinsics. This has interesting consequence for matchers: we can match the > native `StrictMath.sqrt` to non-native intrinsic for `Math.sqrt`. Interpreter > would then have to disambiguate the two. It could be made simpler and more > consistent. > > For `min`/`max` methods, `StrictMath` already delegates to `Math` methods, so > we can just drop the intrinsics for them. `sqrt` is harder to delegate, > because it is `native` and a part of public API, so we can instead do the > proper special intrinsic for it. > > There seem to be no performance regressions with this patch at least on Linux > x86_64: > > > $ CONF=linux-x86_64-server-release make test TEST="micro:StrictMathBench" > > Benchmark Mode Cnt Score Error Units > > ### Before > > StrictMathBench.minDouble thrpt4 230921.558 ± 234.238 ops/ms > StrictMathBench.minFloat thrpt4 230932.303 ± 126.721 ops/ms > StrictMathBench.minInt thrpt4 230917.256 ± 73.008 ops/ms > StrictMathBench.minLongthrpt4 194460.828 ± 178.079 ops/ms > > > StrictMathBench.maxDouble thrpt4 230983.180 ± 161.211 ops/ms > StrictMathBench.maxFloat thrpt4 230969.290 ± 277.500 ops/ms > StrictMathBench.maxInt thrpt4 231033.581 ± 200.015 ops/ms > StrictMathBench.maxLongthrpt4 194590.744 ± 114.295 ops/ms > > > StrictMathBench.sqrtDouble thrpt4 230722.037 ± .080 ops/ms > > ### After > > StrictMathBench.minDouble thrpt4 230976.625 ± 67.338 ops/ms > StrictMathBench.minFloat thrpt4 230896.021 ± 270.434 ops/ms > StrictMathBench.minInt thrpt4 230859.741 ± 403.147 ops/ms > StrictMathBench.minLongthrpt4 194456.673 ± 111.557 ops/ms > > StrictMathBench.maxDouble thrpt4 230890.776 ± 89.924 ops/ms > StrictMathBench.maxFloat thrpt4 230918.334 ± 63.160 ops/ms > StrictMathBench.maxInt thrpt4 231059.128 ± 51.224 ops/ms > StrictMathBench.maxLongthrpt4 194488.210 ± 495.224 ops/ms > > StrictMathBench.sqrtDouble thrpt4 231023.703 ± 247.330 ops/ms > > > Additional testing: > - [x] `StrictMath` benchmarks > - [x] Linux x86_64 fastdebug `tier1` Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Touchups - Changes: - all: https://git.openjdk.java.net/jdk/pull/6184/files - new: https://git.openjdk.java.net/jdk/pull/6184/files/4cd966dc..27202fa4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6184=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6184=00-01 Stats: 8 lines in 3 files changed: 4 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6184.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6184/head:pull/6184 PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote: > Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. LGTM, but please use `static final` src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: > 246: private static final LocaleDataStrategy INSTANCE = new > LocaleDataStrategy(); > 247: // TODO: avoid hard-coded Locales > 248: private final static Set JAVA_BASE_LOCALES Canonical modifier order is `static final` src/java.base/share/classes/sun/util/resources/LocaleData.java line 327: > 325: = new SupplementaryStrategy(); > 326: // TODO: avoid hard-coded Locales > 327: private final static Set JAVA_BASE_LOCALES Canonical modifier order is `static final` - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6191
RFR: 8276220: Reduce excessive allocations in DateTimeFormatter
Prompted by a request from Volkan Yazıcı I took a look at why DataTimeFormatters are much less efficient for some common patterns than custom formatters in apache-commons and log4j. This patch address some of that gap, without having looked at the third party implementations. When printing times: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal` This means a speed-up and reduction in allocations when formatting almost any date or time pattern, and especially so when including sub-second parts (`S-S`). Much of the remaining overhead can be traced to the need to create a `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns. Testing: tier1-3 - Commit messages: - 8276220: Reduce excessive allocations in DateTimeFormatter Changes: https://git.openjdk.java.net/jdk/pull/6188/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6188=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276220 Stats: 429 lines in 4 files changed: 407 ins; 10 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter
On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad wrote: > Prompted by a request from Volkan Yazıcı I took a look at why > DataTimeFormatters are much less efficient for some common patterns than > custom formatters in apache-commons and log4j. This patch address some of > that gap, without having looked at the third party implementations. > > When printing times: > - Avoid turning integral values into `String`s before appending them to the > buffer > - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of > `BigDecimal` > > This means a speed-up and reduction in allocations when formatting almost any > date or time pattern, and especially so when including sub-second parts > (`S-S`). > > Much of the remaining overhead can be traced to the need to create a > `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` > internally. We could likely also win performance by specializing some common > patterns. > > Testing: tier1-3 Microbenchmark numbers for the supplied `DateTimeFormatterBench`. Baseline: Benchmark (pattern) Mode Cnt Score Error Units formatInstants HH:mm:ss thrpt 15 6.558 ± 0.125 ops/ms ·gc.alloc.rate.normHH:mm:ss thrpt 15 192015.139 ± 0.352B/op formatInstants HH:mm:ss.SSS thrpt 15 2.772 ± 0.036 ops/ms ·gc.alloc.rate.normHH:mm:ss.SSS thrpt 15 696805.289 ± 11280.987B/op formatInstants -MM-dd'T'HH:mm:ss thrpt 15 4.025 ± 0.056 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss thrpt 15 264020.926 ± 0.555B/op formatInstants -MM-dd'T'HH:mm:ss.SSS thrpt 15 2.121 ± 0.026 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss.SSS thrpt 15 768811.221 ± 11281.118B/op formatZonedDateTime HH:mm:ss thrpt 15 8.797 ± 0.254 ops/ms ·gc.alloc.rate.normHH:mm:ss thrpt 15 96007.787 ± 0.331B/op formatZonedDateTime HH:mm:ss.SSS thrpt 15 3.109 ± 0.024 ops/ms ·gc.alloc.rate.normHH:mm:ss.SSS thrpt 15 600798.055 ± 11280.992B/op formatZonedDateTime -MM-dd'T'HH:mm:ss thrpt 15 4.814 ± 0.037 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss thrpt 15 168013.636 ± 0.389B/op formatZonedDateTime -MM-dd'T'HH:mm:ss.SSS thrpt 15 2.299 ± 0.059 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss.SSS thrpt 15 680012.566 ± 11281.140B/op Patched: Benchmark (pattern) Mode Cnt Score Error Units formatInstants HH:mm:ss thrpt 15 7.721 ± 0.114 ops/ms ·gc.alloc.rate.normHH:mm:ss thrpt 15 120010.043 ± 0.934B/op formatInstants HH:mm:ss.SSS thrpt 15 5.684 ± 0.083 ops/ms ·gc.alloc.rate.normHH:mm:ss.SSS thrpt 15 120009.973 ± 0.608B/op formatInstants -MM-dd'T'HH:mm:ss thrpt 15 4.962 ± 0.058 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss thrpt 15 120010.027 ± 0.703B/op formatInstants -MM-dd'T'HH:mm:ss.SSS thrpt 15 3.889 ± 0.068 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss.SSS thrpt 15 120010.284 ± 1.003B/op formatZonedDateTime HH:mm:ss thrpt 15 11.087 ± 0.404 ops/ms ·gc.alloc.rate.normHH:mm:ss thrpt 15 24002.072 ± 0.219B/op formatZonedDateTime HH:mm:ss.SSS thrpt 15 7.325 ± 0.139 ops/ms ·gc.alloc.rate.normHH:mm:ss.SSS thrpt 15 24002.080 ± 0.267B/op formatZonedDateTime -MM-dd'T'HH:mm:ss thrpt 15 6.127 ± 0.040 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss thrpt 15 24002.084 ± 0.459B/op formatZonedDateTime -MM-dd'T'HH:mm:ss.SSS thrpt 15 4.576 ± 0.049 ops/ms ·gc.alloc.rate.norm -MM-dd'T'HH:mm:ss.SSS thrpt 15 24002.102 ± 0.511B/op The most dramatic improvement is seen for `formatZonedDateTime` using the format `-MM-dd'T'HH:mm:ss.SSS`, which sees a 2x speed-up and almost 97% reduction in allocations. The same variant using `Instant`s see a 1.8x speed-up and almost 85% reduction in allocations. - PR: https://git.openjdk.java.net/jdk/pull/6188
RFR: 8276234: Trivially clean up locale-related code
Please review this PR. A comprehensive test job has been scheduled; I'll notify this thread once that job has completed. - Commit messages: - Fix *code* typo - Be more immutable - Fix typos Changes: https://git.openjdk.java.net/jdk/pull/6191/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6191=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276234 Stats: 10 lines in 3 files changed: 0 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/6191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6191/head:pull/6191 PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling
On Mon, 1 Nov 2021 11:23:16 GMT, Aleksey Shipilev wrote: > This blocks JDK-8276215: `StrictMath` intrinsics are handled peculiarly by > giving failing intrinsics a second chance to match against the similar `Math` > intrinsics. This has interesting consequence for matchers: we can match the > native `StrictMath.sqrt` to non-native intrinsic for `Math.sqrt`. Interpreter > would then have to disambiguate the two. It could be made simpler and more > consistent. > > For `min`/`max` methods, `StrictMath` already delegates to `Math` methods, so > we can just drop the intrinsics for them. `sqrt` is harder to delegate, > because it is `native` and a part of public API, so we can instead do the > proper special intrinsic for it. > > There seem to be no performance regressions with this patch at least on Linux > x86_64: > > > $ CONF=linux-x86_64-server-release make test TEST="micro:StrictMathBench" > > Benchmark Mode Cnt Score Error Units > > ### Before > > StrictMathBench.minDouble thrpt4 230921.558 ± 234.238 ops/ms > StrictMathBench.minFloat thrpt4 230932.303 ± 126.721 ops/ms > StrictMathBench.minInt thrpt4 230917.256 ± 73.008 ops/ms > StrictMathBench.minLongthrpt4 194460.828 ± 178.079 ops/ms > > > StrictMathBench.maxDouble thrpt4 230983.180 ± 161.211 ops/ms > StrictMathBench.maxFloat thrpt4 230969.290 ± 277.500 ops/ms > StrictMathBench.maxInt thrpt4 231033.581 ± 200.015 ops/ms > StrictMathBench.maxLongthrpt4 194590.744 ± 114.295 ops/ms > > > StrictMathBench.sqrtDouble thrpt4 230722.037 ± .080 ops/ms > > ### After > > StrictMathBench.minDouble thrpt4 230976.625 ± 67.338 ops/ms > StrictMathBench.minFloat thrpt4 230896.021 ± 270.434 ops/ms > StrictMathBench.minInt thrpt4 230859.741 ± 403.147 ops/ms > StrictMathBench.minLongthrpt4 194456.673 ± 111.557 ops/ms > > StrictMathBench.maxDouble thrpt4 230890.776 ± 89.924 ops/ms > StrictMathBench.maxFloat thrpt4 230918.334 ± 63.160 ops/ms > StrictMathBench.maxInt thrpt4 231059.128 ± 51.224 ops/ms > StrictMathBench.maxLongthrpt4 194488.210 ± 495.224 ops/ms > > StrictMathBench.sqrtDouble thrpt4 231023.703 ± 247.330 ops/ms > > > Additional testing: > - [x] `StrictMath` benchmarks > - [x] Linux x86_64 fastdebug `tier1` So we have _dsqrt and_dsqrt_strict, which must be functionally identical, but we provide both names because they're part of a public API. I think this deserves an explanatory comment in the code. - PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8276096: Simplify Unsafe.{load|store}Fence fallbacks by delegating to fullFence [v2]
On Mon, 1 Nov 2021 07:32:18 GMT, Aleksey Shipilev wrote: >> src/hotspot/share/classfile/vmIntrinsics.hpp line 526: >> >>> 524:do_name( storeFence_name, >>> "storeFence")\ >>> 525:do_alias(storeFence_signature, >>> void_method_signature) \ >>> 526: do_intrinsic(_fullFence,jdk_internal_misc_Unsafe, >>> fullFence_name, fullFence_signature, F_R) \ >> >> Why did you drop the N from F_RN? AFAICS the fullFence method is still >> native. > > Good spot! That's indeed incorrect, fixed in new commit. I am surprised > `CheckIntrinsics` did not found this discrepancy. I believe "native" flags > are not checked at all? For example, existing `_hashCode` intrinsic is also > `F_R`, while it covers the native `java.lang.Object::hashCode`. I try to beef > up those checks separately. This `CheckIntrinsics` oddity is handled by #6187. - PR: https://git.openjdk.java.net/jdk/pull/6149
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - Add cache for memory address var handles - Merge branch 'master' into JEP-419 - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm) - Merge branch 'master' into JEP-419 - Fix copyright header in TestArrayCopy - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!) - * use `invokeWithArguments` to simplify new test - Add test for liveness check with high-aririty downcalls (make sure that if an exception occurs in a downcall because of liveness, ref count of other resources are left intact). - * Fix javadoc issue in VaList * Fix bug in concurrent logic for shared scope acquire - Address review comments - ... and 7 more: https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343 - Changes: https://git.openjdk.java.net/jdk/pull/5907/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5907=09 Stats: 14497 lines in 189 files changed: 6773 ins; 5149 del; 2575 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
RFR: 8276217: Harmonize StrictMath intrinsics handling
This blocks JDK-8276215: `StrictMath` intrinsics are handled peculiarly by giving failing intrinsics a second chance to match against the similar `Math` intrinsics. This has interesting consequence for matchers: we can match the native `StrictMath.sqrt` to non-native intrinsic for `Math.sqrt`. Interpreter would then have to disambiguate the two. It could be made simpler and more consistent. For `min`/`max` methods, `StrictMath` already delegates to `Math` methods, so we can just drop the intrinsics for them. `sqrt` is harder to delegate, because it is `native` and a part of public API, so we can instead do the proper special intrinsic for it. There seem to be no performance regressions with this patch at least on Linux x86_64: $ CONF=linux-x86_64-server-release make test TEST="micro:StrictMathBench" Benchmark Mode Cnt Score Error Units ### Before StrictMathBench.minDouble thrpt4 230921.558 ± 234.238 ops/ms StrictMathBench.minFloat thrpt4 230932.303 ± 126.721 ops/ms StrictMathBench.minInt thrpt4 230917.256 ± 73.008 ops/ms StrictMathBench.minLongthrpt4 194460.828 ± 178.079 ops/ms StrictMathBench.maxDouble thrpt4 230983.180 ± 161.211 ops/ms StrictMathBench.maxFloat thrpt4 230969.290 ± 277.500 ops/ms StrictMathBench.maxInt thrpt4 231033.581 ± 200.015 ops/ms StrictMathBench.maxLongthrpt4 194590.744 ± 114.295 ops/ms StrictMathBench.sqrtDouble thrpt4 230722.037 ± .080 ops/ms ### After StrictMathBench.minDouble thrpt4 230976.625 ± 67.338 ops/ms StrictMathBench.minFloat thrpt4 230896.021 ± 270.434 ops/ms StrictMathBench.minInt thrpt4 230859.741 ± 403.147 ops/ms StrictMathBench.minLongthrpt4 194456.673 ± 111.557 ops/ms StrictMathBench.maxDouble thrpt4 230890.776 ± 89.924 ops/ms StrictMathBench.maxFloat thrpt4 230918.334 ± 63.160 ops/ms StrictMathBench.maxInt thrpt4 231059.128 ± 51.224 ops/ms StrictMathBench.maxLongthrpt4 194488.210 ± 495.224 ops/ms StrictMathBench.sqrtDouble thrpt4 231023.703 ± 247.330 ops/ms Additional testing: - [x] `StrictMath` benchmarks - [x] Linux x86_64 fastdebug `tier1` - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/6184/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6184=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276217 Stats: 66 lines in 16 files changed: 27 ins; 26 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/6184.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6184/head:pull/6184 PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v10]
On Fri, 29 Oct 2021 16:17:46 GMT, Aleksei Efimov wrote: >> This change implements a new service provider interface for host name and >> address resolution, so that java.net.InetAddress API can make use of >> resolvers other than the platform's built-in resolver. >> >> The following API classes are added to `java.net.spi` package to facilitate >> this: >> - `InetAddressResolverProvider` - abstract class defining a service, and >> is, essentially, a factory for `InetAddressResolver` resolvers. >> - `InetAddressResolverProvider.Configuration ` - an interface describing the >> platform's built-in configuration for resolution operations that could be >> used to bootstrap a resolver construction, or to implement partial >> delegation of lookup operations. >> - `InetAddressResolver` - an interface that defines methods for the >> fundamental forward and reverse lookup operations. >> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the >> characteristics of one forward lookup operation. >> >> More details in [JEP-418](https://openjdk.java.net/jeps/418). >> >> Testing: new and existing `tier1:tier3` tests > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Replace 'system' with 'the system-wide', move comment section Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
On Wed, 6 Oct 2021 18:40:36 GMT, Mark Reinhold wrote: >> @mbreinhold Could you comment on this pull request? > >> @mbreinhold Could you comment on this pull request? > > This is somewhat tricky code. I’ll take a look, but give me a few days. @mbreinhold @AlanBateman Are you aware of my comment? Could you please reply? - PR: https://git.openjdk.java.net/jdk/pull/5679
Re: RFR: 8276164: RandomAccessFile#write method could throw IndexOutOfBoundsException that is not described in javadoc [v2]
On Fri, 29 Oct 2021 12:51:46 GMT, Masanori Yano wrote: >> Could you please review the 8276164 bug fixes? >> >> I suggest adding the missing javadoc of `@throws IndexOutOfBoundsException`. > > Masanori Yano has updated the pull request incrementally with two additional > commits since the last revision: > > - 8276164: RandomAccessFile#write method could throw > IndexOutOfBoundsException that is not described in javadoc > - 8276164: RandomAccessFile#write method could throw > IndexOutOfBoundsException that is not described in javadoc This looks okay. I just wonder if we should take a wider pass over the classes in java.io as @throws IOOBE is missing or covered by method descriptions in several places. Also many of the input/output stream classes don't inheritDoc this exception so it doesn't appear in the javadoc of the sub-classes. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6168
Re: RFR: 8276096: Simplify Unsafe.{load|store}Fence fallbacks by delegating to fullFence
On Mon, 1 Nov 2021 02:15:04 GMT, David Holmes wrote: > I'm not quite seeing the motivation here. Your claim is that the > non-intrinsic implementations involve a native call and so that is too > expensive; yet the new code still relies on the fullFence being intrinsified > else it is still a native call and a heavier barrier. If these fences were > intrinisified piecemeal then perhaps this is an issue on some platform, but > is that really the case? If you intrinsified one wouldn't you intrinsify all? Yes, that was not clear, sorry. For current platforms, it is mostly a maintenance cleanup to shrink the unnecessary Unsafe interfaces: if we disable the `acquireFence` intrinsic, we don't need to call into native fallback (which would be excessive), instead we can just go to Java-level fallback (which would also be faster). I am looking at the cases where we would like to only intrinsify `fullFence`, for example for Zero interpreter. Instead of handling all three flavors of fences, we can get the majority of performance win by only drilling the interpreter-entry-intrinsic hole for `fullFence`, and let everything else handled at Java level. - PR: https://git.openjdk.java.net/jdk/pull/6149
Re: RFR: 8276096: Simplify Unsafe.{load|store}Fence fallbacks by delegating to fullFence [v2]
On Mon, 1 Nov 2021 02:09:19 GMT, David Holmes wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Restore RN for fullFence > > src/hotspot/share/classfile/vmIntrinsics.hpp line 526: > >> 524:do_name( storeFence_name, >> "storeFence")\ >> 525:do_alias(storeFence_signature, >> void_method_signature) \ >> 526: do_intrinsic(_fullFence,jdk_internal_misc_Unsafe, >> fullFence_name, fullFence_signature, F_R) \ > > Why did you drop the N from F_RN? AFAICS the fullFence method is still native. Good spot! That's indeed incorrect, fixed in new commit. I am surprised `CheckIntrinsics` did not found this discrepancy. I believe "native" flags are not checked at all? For example, existing `_hashCode` intrinsic is also `F_R`, while it covers the native `java.lang.Object::hashCode`. I try to beef up those checks separately. - PR: https://git.openjdk.java.net/jdk/pull/6149
Re: RFR: 8276096: Simplify Unsafe.{load|store}Fence fallbacks by delegating to fullFence [v2]
> `Unsafe.{load|store}Fence` falls back to `unsafe.cpp` for
> `OrderAccess::{acquire|release}Fence()`. It seems too heavy-handed (useless?)
> to call to runtime for a single memory barrier. We can simplify the native
> `Unsafe` interface by falling back to `fullFence` when `{load|store}Fence`
> intrinsics are not available. This would be similar to what
> `Unsafe.{loadLoad|storeStore}Fences` do.
>
> This is the behavior of these intrinsics now, on x86_64, using benchmarks
> from JDK-8276054:
>
>
> Benchmark Mode Cnt Score Error Units
>
> # Default
> Single.acquire avgt3 0.407 ± 0.060 ns/op
> Single.fullavgt3 4.693 ± 0.005 ns/op
> Single.loadLoadavgt3 0.415 ± 0.095 ns/op
> Single.plain avgt3 0.406 ± 0.002 ns/op
> Single.release avgt3 0.408 ± 0.047 ns/op
> Single.storeStore avgt3 0.408 ± 0.043 ns/op
>
> # -XX:DisableIntrinsic=_storeFence
> Single.acquire avgt3 0.408 ± 0.016 ns/op
> Single.fullavgt3 4.694 ± 0.002 ns/op
> Single.loadLoadavgt3 0.406 ± 0.002 ns/op
> Single.plain avgt3 0.406 ± 0.001 ns/op
> Single.release avgt3 4.694 ± 0.003 ns/op <--- upgraded to full
> Single.storeStore avgt3 4.690 ± 0.005 ns/op <--- upgraded to full
>
> # -XX:DisableIntrinsic=_loadFence
> Single.acquire avgt3 4.691 ± 0.001 ns/op <--- upgraded to full
> Single.fullavgt3 4.693 ± 0.009 ns/op
> Single.loadLoadavgt3 4.693 ± 0.013 ns/op <--- upgraded to full
> Single.plain avgt3 0.408 ± 0.072 ns/op
> Single.release avgt3 0.415 ± 0.016 ns/op
> Single.storeStore avgt3 0.416 ± 0.041 ns/op
>
> # -XX:DisableIntrinsic=_fullFence
> Single.acquire avgt3 0.406 ± 0.014 ns/op
> Single.fullavgt3 15.836 ± 0.151 ns/op <--- calls runtime
> Single.loadLoadavgt3 0.406 ± 0.001 ns/op
> Single.plain avgt3 0.426 ± 0.361 ns/op
> Single.release avgt3 0.407 ± 0.021 ns/op
> Single.storeStore avgt3 0.410 ± 0.061 ns/op
>
> # -XX:DisableIntrinsic=_fullFence,_loadFence
> Single.acquire avgt3 15.822 ± 0.282 ns/op <--- upgraded, calls
> runtime
> Single.fullavgt3 15.851 ± 0.127 ns/op <--- calls runtime
> Single.loadLoadavgt3 15.829 ± 0.045 ns/op <--- upgraded, calls
> runtime
> Single.plain avgt3 0.406 ± 0.001 ns/op
> Single.release avgt3 0.414 ± 0.156 ns/op
> Single.storeStore avgt3 0.422 ± 0.452 ns/op
>
> # -XX:DisableIntrinsic=_fullFence,_storeFence
> Single.acquire avgt3 0.407 ± 0.016 ns/op
> Single.fullavgt3 15.347 ± 6.783 ns/op <--- calls runtime
> Single.loadLoadavgt3 0.406 ± 0.001 ns/op
> Single.plain avgt3 0.406 ± 0.002 ns/op
> Single.release avgt3 15.828 ± 0.019 ns/op <--- upgraded, calls
> runtime
> Single.storeStore avgt3 15.834 ± 0.045 ns/op <--- upgraded, calls
> runtime
>
> # -XX:DisableIntrinsic=_fullFence,_loadFence,_storeFence
> Single.acquire avgt3 15.838 ± 0.030 ns/op <--- upgraded, calls
> runtime
> Single.fullavgt3 15.854 ± 0.277 ns/op <--- calls runtime
> Single.loadLoadavgt3 15.826 ± 0.160 ns/op <--- upgraded, calls
> runtime
> Single.plain avgt3 0.406 ± 0.003 ns/op
> Single.release avgt3 15.838 ± 0.019 ns/op <--- upgraded, calls
> runtime
> Single.storeStore avgt3 15.844 ± 0.104 ns/op <--- upgraded, calls
> runtime
>
>
> Additional testing:
> - [x] Linux x86_64 fastdebug `tier1`
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
Restore RN for fullFence
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/6149/files
- new: https://git.openjdk.java.net/jdk/pull/6149/files/e2c623be..a0fd03ee
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=6149=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=6149=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/6149.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6149/head:pull/6149
PR: https://git.openjdk.java.net/jdk/pull/6149
