Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
On Thu, 4 Nov 2021 03:18:40 GMT, Jaikiran Pai wrote:
> > So I decided to stick with using the ordinal modifiers in the hashCode()
> > computation. However, I'm not an expert on the performance aspects of the
> > Collections and how much using the modifiers for hashCode() will improve
> > the performance in this case. Perhaps that's what you meant by it not
> > giving a good (enough) spread? If so, do let me know and I'll go ahead and
> > update the PR to remove using the modifiers in the hashCode() computation
> > and also update the javadoc of each of those hashCode() methods which state
> > that the modifiers are used in the hashCode() computation.
>
> Okay, but we may have to re-visit this some time because summing the ordinals
> won't give a good spread, e.g. STATIC+SYNTHETIC will give the same value as
> TRANSITIVE. This is why I was saying it would be nearly the same to just
> leave them out.
That's a good point and thank you for explaining that part. So to address that,
how about not leaving them out, but instead of using `Enum#ordinal()` for the
hashCode generation, we use `Enum#name()` which would translate to a
`String#hashCode()`. Something like:
private static int modsHashCode(Iterable> enums) {
int h = 0;
for (Enum e : enums) {
h = h * 43 + Objects.hashCode(e.name());
}
return h;
}
That way the hashCode isn't dependent on the Object identity (which is what the
original issue was), is still reproducible across JVM runs and at the same tie
should provide a good spread for examples like the one you noted. Furthermore,
unlike the usage of `ordinal` where any reordering of the enum values would
have changed the hashCode value (which was OK), this usage of `name()` doesn't
exhibit that nature (which again should be OK).
I've updated the PR to use this construct. The test continues to pass. Let me
know if this looks fine or if you prefer doing it differently.
-
PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v12]
> 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: use Enum#name() for hashCode() generation instead of Enum#ordinal() - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/9dd2774c..a0c4f847 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=10-11 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 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
RFR: JDK-8276572: Fake libsyslookup.so library causes tooling issues
The lack of anything to compile in `syslookup.c` is leading to a number of issues in our tooling, on some architectures more than others (s390x seems to be the most problematic). They expect to be able to retrieve debuginfo and compiler flags from generated .so files and fail with libsyslookup.so This simple patch adds a dummy function to `syslookup.c` so it appears more like a regular file to be compiled. I can't see this causing a problem with the symbol lookup, but we could filter it out on the Java side if need be. - Commit messages: - JDK-8276572: Fake libsyslookup.so library causes tooling issues Changes: https://git.openjdk.java.net/jdk/pull/6245/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6245=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276572 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6245/head:pull/6245 PR: https://git.openjdk.java.net/jdk/pull/6245
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
Hello Alan, On 04/11/21 1:21 am, Alan Bateman wrote: On Wed, 3 Nov 2021 03:22:40 GMT, Jaikiran Pai wrote: So I decided to stick with using the ordinal modifiers in the hashCode() computation. However, I'm not an expert on the performance aspects of the Collections and how much using the modifiers for hashCode() will improve the performance in this case. Perhaps that's what you meant by it not giving a good (enough) spread? If so, do let me know and I'll go ahead and update the PR to remove using the modifiers in the hashCode() computation and also update the javadoc of each of those hashCode() methods which state that the modifiers are used in the hashCode() computation. Okay, but we may have to re-visit this some time because summing the ordinals won't give a good spread, e.g. STATIC+SYNTHETIC will give the same value as TRANSITIVE. This is why I was saying it would be nearly the same to just leave them out. So let's go with it for now. Thanks for the update to the test. The assertNotSame to check that they aren't the same instance looks a bit odd but at least you have a comment to explain it. I guess we should add an assertEquals to check that the 2 descriptors are equals too. Then I think we are done. I intentionally didn't add the assertEquals for the 2 module descriptors because that check will fail (with CDS enabled) until the other issue https://bugs.openjdk.java.net/browse/JDK-8275731 is fixed. -Jaikiran
Re: RFR: 8276348: Use blessed modifier order in java.base
On 4/11/2021 12:14 am, Pavel Rappo wrote:
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote:
This PR follows up one of the recent PRs, where I used a non-canonical modifier
order. Since the problem was noticed [^1], why not to address it en masse?
As far as I remember, the first mass-canonicalization of modifiers took place
in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453
files. Since then modifiers have become a bit loose, and it makes sense to
re-bless (using the JDK-8136583 terminology) them.
This change was produced by running the below command followed by updating the
copyright years on the affected files where necessary:
$ sh ./bin/blessed-modifier-order.sh src/java.base
The resulting change is much smaller than that of 2015: 39 lines spanning 21
files.
[^1]:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
(or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
[^2]:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
_Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on
[core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_
On 3/11/2021 9:26 pm, Pavel Rappo wrote:
On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz wrote:
Pragmatically, fix the script to ignore those keywords on comment lines. Learn
Perl, its just a regular expression pattern match and replace expression.
I understand in principle how to modify that script to ignore doc comments. The thing I
was referring to when said "btw, how would we do that?" was this: not all
comment lines are prose. Some of those lines belong to snippets of code, which I guess
you would also like to be properly formatted.
But having seen several reviewers be unmoved by the difference, the real
pragmatic view is to ignore the English.
I'm sorry you feel that way. Would it be okay if I made it clear that those two
words are not English adjectives but are special symbols that happen to use
Latin script and originate from the English words they resemble? If so, I could
enclose each of them in `{@code ... }`. If not, I could drop that particular
change from this PR.
The blessed-modifier-order.sh script intentionally modifies comments, with the
hope of finding code snippets (it did!)
Probably I manually deleted the change to Object.java back in 2015, to avoid
the sort of controversy we're seeing now.
I don't have a strong feeling either way on changing that file.
I agree with @pavelrappo that script-generated changes should not be mixed
with manual changes.
I would also not update copyright years for such changes.
It's a feature of blessed-modifier-order.sh that all existing formatting is
perfectly preserved.
One more thing. Please have a look at this other line in the same file; this
line was there before the change
https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
So before the change, the file was somewhat inconsistent. The change made it
consistent. **If one is going to ever revert that controversial part of the
change, please update both lines so that the file remains consistent.**
Line 281 is (was!) consistent with line 277 because it is distinguishing a synchronized "static method" from
a synchronized "instance method". There was no need to make any change to line 281 because of the
blessed-order of modifiers as defined for method declarations! This text is just prose. Now for consistency you should
change line 277 to refer to a "non-static synchronized method" (as "instance synchronized method"
would be truly awful).
Thanks, David. You've provided a clear and convincing argument, and I can see
the inconsistency I introduced. I can revert that particular piece back if you
think that it would be appropriate.
That said, this line will have to be filtered out every time the script is run.
I could probably provide a more involved script that harnesses the power of AST
(com.sun.source.doctree) to try to filter out prose, but it would be impossible
to beat the simplicity of the current script; and simplicity is also important.
Given this is prose, the adjectives should be separated by commas: "a
synchronized, static method", and "a synchronized, instance method".
Does that avoid the problem with the script?
Line 49 places "static synchronized" in code font, implying that it is referring to the actual method
modifiers and as such using the blessed order is appropriate. Line 49 does not need to be "consistent" with
line 281. If line 49 used a normal font so the words "static" and "synchronized" were just prose
then either order would be perfectly fine in terms of English language, but then you could make a case for having it be
consistent with line 281.
I've been always having hard time with modifiers being not enclosed in `@code`
in the first place; they are barely
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Fri, 29 Oct 2021 10:38:41 GMT, Ivan Šipka wrote:
>> cc @ctornqvi
>
> Ivan Šipka has updated the pull request incrementally with one additional
> commit since the last revision:
>
> removed file added by accident
Thanks for the clarification, David. I guess my recollection of jtreg code
isn’t as good as I thought, and `-` isn’t mandatory, though this means there is
a (theoretically possible) ambiguity, e.g. `{os=windows; version=11}` vs
`{os=windows1; version=1}`
-
PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Thu, 4 Nov 2021 01:34:01 GMT, Igor Ignatyev wrote: >>> That doesn’t seem right as jtreg expects `-` not >>> `` >> >> It has been tested, details in ticket comment. > > I’m sorry @frkator but there is nothing in the ticket. @iignatev the comment is confidential as it refers to internal testing infrastructure. @frkator be mindful of what parts of the JBS issue are public and which are not. - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8276096: Simplify Unsafe.{load|store}Fence fallbacks by delegating to fullFence [v2]
On Mon, 1 Nov 2021 07:36:53 GMT, Aleksey Shipilev wrote:
>> `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
Marked as reviewed by dholmes (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6149
Re: RFR: 8276048: Error in javadoc regarding Long method public static Long valueOf(String s)
On Wed, 27 Oct 2021 09:57:43 GMT, swati sharma wrote:
> 8276048: Error in javadoc regarding Long method public static Long
> valueOf(String s)
> Fix: Changing integer to {@code Long}
I agree with Joe there is nothing to fix here - "integer" is general
mathematical term. If it were "int" or "Integer" then that would be different.
-
PR: https://git.openjdk.java.net/jdk/pull/6135
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Wed, 3 Nov 2021 23:11:36 GMT, Ivan Šipka wrote: >> That doesn’t seem right as jtreg expects `-` not `` > >> That doesn’t seem right as jtreg expects `-` not `` > > It has been tested, details in ticket comment. I’m sorry @frkator but there is nothing in the ticket. - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Wed, 3 Nov 2021 22:12:29 GMT, Igor Ignatyev wrote: > That doesn’t seem right as jtreg expects `-` not `` It has been tested, details in ticket comment. - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]
On Wed, 3 Nov 2021 21:57:44 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 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidentally committed experimental @Stable (no effect on micros) Thanks, Naoto! - PR: https://git.openjdk.java.net/jdk/pull/6188
Integrated: 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 This pull request has now been integrated. Changeset: ce8c7670 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/ce8c76700ba854f43c48d32b068b30e7d78d9d1e Stats: 564 lines in 4 files changed: 533 ins; 12 del; 19 mod 8276220: Reduce excessive allocations in DateTimeFormatter Reviewed-by: scolebourne, naoto - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Fri, 29 Oct 2021 10:38:41 GMT, Ivan Šipka wrote: >> cc @ctornqvi > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > removed file added by accident That doesn’t seem right as jtreg expects `-` not `` - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]
On Wed, 3 Nov 2021 21:57:44 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 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove accidentally committed experimental @Stable (no effect on micros) Looks good. Thank you for the fix! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]
> 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 accidentally committed experimental @Stable (no effect on micros) - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/f6adb5b5..b663fe63 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=08-09 Stats: 2 lines in 1 file changed: 0 ins; 2 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
RFR: 8231490: Ugly racy writes to ZipUtils.defaultBuf
This change applies the minimal fix suggested in https://bugs.openjdk.java.net/browse/JDK-8231490. The bug text suggests possibilities for reworking, but notes that this change is enough to fix the data race. Adding a regression test is probaby not feasible but we do observe that Java TSAN no longer reports a race after the change. - Commit messages: - 8231490: Fix a data race in java.util.zip.Inflater Changes: https://git.openjdk.java.net/jdk/pull/6242/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6242=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8231490 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6242.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6242/head:pull/6242 PR: https://git.openjdk.java.net/jdk/pull/6242
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v9]
> 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: Copyrights - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/01fce436..f6adb5b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=07-08 Stats: 25 lines in 4 files changed: 22 ins; 0 del; 3 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: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
On Tue, 2 Nov 2021 20:09:21 GMT, Mandy Chung wrote: > I reviewed the change. It is reasonable to fix the parsing of the pre-release > version and the build version be consistent with parsing of the version > number which currently skips consecutive delimiters. > > The spec is unclear on whether an empty token separated by the delimiters is > ignored. The spec may needs some clarification. Yes, I think we should fix the consistency issues although the cases where the inconsistency show up seem unusual. In two minds on whether to should do the spec clarification as part of this PR or create a separate issue. - PR: https://git.openjdk.java.net/jdk/pull/5679
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
On Wed, 3 Nov 2021 03:22:40 GMT, Jaikiran Pai wrote: > So I decided to stick with using the ordinal modifiers in the hashCode() > computation. However, I'm not an expert on the performance aspects of the > Collections and how much using the modifiers for hashCode() will improve the > performance in this case. Perhaps that's what you meant by it not giving a > good (enough) spread? If so, do let me know and I'll go ahead and update the > PR to remove using the modifiers in the hashCode() computation and also > update the javadoc of each of those hashCode() methods which state that the > modifiers are used in the hashCode() computation. Okay, but we may have to re-visit this some time because summing the ordinals won't give a good spread, e.g. STATIC+SYNTHETIC will give the same value as TRANSITIVE. This is why I was saying it would be nearly the same to just leave them out. So let's go with it for now. Thanks for the update to the test. The assertNotSame to check that they aren't the same instance looks a bit odd but at least you have a comment to explain it. I guess we should add an assertEquals to check that the 2 descriptors are equals too. Then I think we are done. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v8]
On Wed, 3 Nov 2021 19:44:35 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 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add MICRO_OF_SECOND tests to retain proper coverage of FractionPrinterParser Oh, just noticed the copyright year->2021 in modified files. Should have noticed before. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]
On Wed, 3 Nov 2021 18:17:38 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Minor cleanup > > test/jdk/java/time/test/java/time/format/TestFractionPrinterParser.java line > 80: > >> 78: >> 79: /** >> 80: * Test FractionPrinterParser. > > OK, then I'd add some comments that the test covers `NanoPrinterParser` as > well. Ok, done. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v8]
> 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: Add MICRO_OF_SECOND tests to retain proper coverage of FractionPrinterParser - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/3ce6d977..01fce436 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=06-07 Stats: 107 lines in 2 files changed: 103 ins; 2 del; 2 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: 8276048: Error in javadoc regarding Long method public static Long valueOf(String s)
On Wed, 27 Oct 2021 09:57:43 GMT, swati sharma wrote:
> 8276048: Error in javadoc regarding Long method public static Long
> valueOf(String s)
> Fix: Changing integer to {@code Long}
Strictly speaking I do not view the current text as erroneous. At time
"integer" is used an adjective; for example, 1 is an integer number while 1.5
is not. I've sometimes used "integral" as an adjective to avoid any confusion
with the int(eger) type.
-
PR: https://git.openjdk.java.net/jdk/pull/6135
Integrated: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
On Wed, 20 Oct 2021 00:04:08 GMT, Ivan Šipka wrote: > cc @ctornqvi This pull request has now been integrated. Changeset: 32895ac6 Author:Ivan Šipka Committer: Mark Sheppard URL: https://git.openjdk.java.net/jdk/commit/32895ac60949ccceb0a3d25c73ec5e3a00c29593 Stats: 1 line in 2 files changed: 1 ins; 0 del; 0 mod 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 Reviewed-by: bpb, msheppar - PR: https://git.openjdk.java.net/jdk/pull/6025
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 Firstly, please do NOT rebase your fix, as it will clobber the review history. Use merge instead. As to the fix, I am not sure consolidating the code into Log.java would be OK as it would introduce module dependency. - PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8276207: Properties.loadFromXML/storeToXML works incorrectly for supplementary characters
On Tue, 2 Nov 2021 19:06:09 GMT, Anirvan Sarkar wrote: > This fixes Properties.loadFromXML/storeToXML so that it works correctly for > supplementary characters. Looks good to me. Thanks Anirvan for fixing the issue. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6216
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]
On Wed, 3 Nov 2021 17:23:51 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 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Minor cleanup test/jdk/java/time/test/java/time/format/TestFractionPrinterParser.java line 80: > 78: > 79: /** > 80: * Test FractionPrinterParser. OK, then I'd add some comments that the test covers `NanoPrinterParser` as well. - PR: https://git.openjdk.java.net/jdk/pull/6188
RE: Incorrect assumption on the class name in compile(InputSource input, String name) at XSLTC.java
The issue for the XSLTC was raised at
https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-October/082767.html
:
The code in compile(InputSource input, String name) at
https://github.com/openjdk/jdk/blob/master/src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/compiler/XSLTC.java
seems incorrect as follows:
public boolean compile(InputSource input, String name) {
try {
// Reset globals in case we're called by compile(ArrayList v);
reset();
// The systemId may not be set, so we'll have to check the URL
String systemId = null;
if (input != null) {
systemId = input.getSystemId();
}
// Set the translet class name if not already set
if (_className == null) {
if (name != null) {
setClassName(name);
}
else if (systemId != null && !systemId.equals("")) {
setClassName(Util.baseName(systemId)); <---
incorrect here
}
// Ensure we have a non-empty class name at this point
if (_className == null || _className.length() == 0) {
setClassName("GregorSamsa"); // default translet name
}
}
...
Specifically, the code above assumes that systemId is something like
"xxx:yyy" in which case the class name (_className) is
"die.verwandlung.yyy" ("die.verwandlung." is the default package name since
Java11) However,Util.baseName(systemId) returns null when systemId is
something like "xxx:" (empty after ":"), in which case the class name
(_className) ends up with "die.verwandlung."(an invalid class name inserted
in the constant pool of the generated bytecode).
>From the perspective of robustness, the code above should be updated to
handle the situation. e.g.
else if (systemId != null && !systemId.equals("")) {
String baseName = Util.baseName(systemId);
if ((baseName != null) && (baseName.length() > 0))
{ <--
setClassName(baseName);
}
which means it should check whether the returned base name from
Util.baseName(systemId) is empty before setting the class name; otherwise,
it should use the default class name ("GregorSamsa").
Thanks and Best Regards
Cheng Jin
J9 VM, IBM Ottawa Lab at Riverside, Ottawa
IBM Runtime Technologies
+1-613-356-5625
jinch...@ca.ibm.com
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]
On Wed, 3 Nov 2021 17:33:36 GMT, Naoto Sato wrote: > Looks good. I'd create a new test case class out of > `TestFractionPrinterParser`, as you introduced the new `NanosPrinterParser`. It was only indirectly a test of `FractionPrinterParser`; it's really a test of `PrinterParsers` built using `appendFraction`, which can be either `FractionPrinterParser` or the new `NanosPrinterParser`. So the name is still somewhat appropriate. We could rename it, but splitting it apart seems excessive. I realized though that with my changes the test coverage of `FractionPrinterParser` is substantially reduced, since most of the testing is done using `NANO_OF_SECOND`. I'm adding a set of tests using similar input for `MICRO_OF_SECOND` that will exercise `FractionPrinterParser`. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: Incorrect assumption on the class name in compile(InputSource input, String name) at XSLTC.java
Hi There, Hotspot developers already identified a bug in verification (failure to capture an invalid package name "die/verwandlung/" existing in the constant pool of bytecode) at https://bugs.openjdk.java.net/browse/JDK-8276241 as I raised via https://mail.openjdk.java.net/pipermail/hotspot-dev/2021-October/055618.html , which was associated with the issue here in XSLT transformation given the invalid package name "die/verwandlung/" was actually triggered by OpenJDK due to the incorrect result from setClassName(Util.baseName(systemId)) (systemId = "xxx:" in which case Util.baseName(systemId) returns null and setClassName(null) ended up with "die/verwandlung/" in generating the bytecode). So I expect someone in OpenJDK to take a look into this issue to see what really happened to the code there in such case. Thanks. Thanks and Best Regards Cheng Jin
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v3]
On Tue, 2 Nov 2021 06:25:33 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 `java/lang/StrictMath`, `java/lang/Math` >> - [x] Linux x86_64 fastdebug `tier1` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Keep intrinsics on StrictMath Thanks! I am going to push this tomorrow morning, if no other comments show up. - PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v17]
> 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 TestUpcall * reverse() has a bug, as it doesn't tweak parameter types * reverse() is applied to the wrong MH - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/9fafb2a6..b9432473 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=15-16 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 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: 8276048: Error in javadoc regarding Long method public static Long valueOf(String s)
On Wed, 27 Oct 2021 09:57:43 GMT, swati sharma wrote:
> 8276048: Error in javadoc regarding Long method public static Long
> valueOf(String s)
> Fix: Changing integer to {@code Long}
src/java.base/share/classes/java/lang/Long.java line 1141:
> 1139: * exactly as if the argument were given to the {@link
> 1140: * #parseLong(java.lang.String)} method. The result is a
> 1141: * {@code Long} object that represents the {@code Long} value
I think it should be `{@code long}`, i.e., the primitive type, not the class,
for consistency with other numeric classes.
-
PR: https://git.openjdk.java.net/jdk/pull/6135
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]
On Wed, 3 Nov 2021 17:23:51 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 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Minor cleanup Looks good. I'd create a new test case class out of `TestFractionPrinterParser`, as you introduced the new `NanosPrinterParser`. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8273660: De-Serialization Stack is suppressing ClassNotFoundException [v2]
On Fri, 29 Oct 2021 15:35:50 GMT, Roger Riggs wrote: >> The ObjectInputStream.GetField method `get(String name, Object val)` should >> have been throwing >> a ClassNotFoundException if the class was not found. Instead the >> implementation was returning null. >> A design error does not allow the `get(String name, Object val)` method to >> throw CNFE as it should. >> However, an exception must be thrown to prevent invalid data from being >> returned. >> Wrapping the CNFE in IOException allows it to be thrown and the exception >> handled. >> The call to `get(String name, Object val)` is always from within a >> `readObject` method >> so the deserialization logic can catch the IOException and unwrap it to >> handle the CNFE. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct comment on the handling of ClassNotFoundException Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6053
RFR: 8276048: Error in javadoc regarding Long method public static Long valueOf(String s)
8276048: Error in javadoc regarding Long method public static Long
valueOf(String s)
Fix: Changing integer to {@code Long}
-
Commit messages:
- Update Long.java
Changes: https://git.openjdk.java.net/jdk/pull/6135/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6135=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8276048
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/6135.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6135/head:pull/6135
PR: https://git.openjdk.java.net/jdk/pull/6135
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]
> 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: Minor cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/1d21a1a5..3ce6d977 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=05-06 Stats: 4 lines in 1 file changed: 0 ins; 4 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: 8273660: De-Serialization Stack is suppressing ClassNotFoundException [v2]
On Fri, 29 Oct 2021 15:35:50 GMT, Roger Riggs wrote: >> The ObjectInputStream.GetField method `get(String name, Object val)` should >> have been throwing >> a ClassNotFoundException if the class was not found. Instead the >> implementation was returning null. >> A design error does not allow the `get(String name, Object val)` method to >> throw CNFE as it should. >> However, an exception must be thrown to prevent invalid data from being >> returned. >> Wrapping the CNFE in IOException allows it to be thrown and the exception >> handled. >> The call to `get(String name, Object val)` is always from within a >> `readObject` method >> so the deserialization logic can catch the IOException and unwrap it to >> handle the CNFE. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct comment on the handling of ClassNotFoundException Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6053
Re: RFR: 8276338: Minor improve of wording for String.to(Lower|Upper)Case
On Wed, 3 Nov 2021 11:58:42 GMT, Pavel Rappo wrote: > This PR fixes two sentences which conflate a string with its length, and also > makes the "equivalency wording" consistent. > > There are many ways to fix "the resulting String may be a different length > than the original String". The proposed way might be one of the most > lightweight ways to do that. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6232
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v6]
On Wed, 3 Nov 2021 14:24:28 GMT, Stephen Colebourne wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add test verifying OOB values throw exception > > Thanks for adding the new tests, and finding that fraction formatting is more > constrained than I thought it was. > > I think there is a case for a spec update in `DateTimeFormatterBuilder` to > clarify the constraint on the input value (at this point, that seems better > than changing the behaviour of fraction formatting). As things stand, the > exception that is thrown is not described by the spec. (The spec change could > be part of this PR or a separate one). Thanks for reviewing, @jodastephen! I think a spec update sounds good, but I think that should be done in as a follow-up. If you would be willing to provide the wording for such a spec update I can take care of creating a CSR. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v6]
On Wed, 3 Nov 2021 13:14:42 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 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add test verifying OOB values throw exception Thanks for adding the new tests, and finding that fraction formatting is more constrained than I thought it was. I think there is a case for a spec update in `DateTimeFormatterBuilder` to clarify the constraint on the input value (at this point, that seems better than changing the behaviour of fraction formatting). As things stand, the exception that is thrown is not described by the spec. (The spec change could be part of this PR or a separate one). - Marked as reviewed by scolebourne (Author). PR: https://git.openjdk.java.net/jdk/pull/6188
RFR: 8276408: Deprecate Runtime.exec methods with a single string command line argument
The three `java.lang.Runtime.exec` methods that tokenize a command line to produce an array of string arguments are easily misused, sometimes with erroneous results. For example, on some operating systems, spaces are supported in filenames and are in common use. The tokenization uses only whitespace characters, ignoring quote characters. It is error prone because quotes may appear in the string but are ignored. The implementation (on Windows) includes a heuristic for the executable argument that tries to re-parse the command line respecting quotes but it is undocumented. - Commit messages: - 8276408: Deprecate Runtime.exec methods with a single string command line argument Changes: https://git.openjdk.java.net/jdk/pull/6233/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6233=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276408 Stats: 21 lines in 1 file changed: 21 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6233.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6233/head:pull/6233 PR: https://git.openjdk.java.net/jdk/pull/6233
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote:
> This PR follows up one of the recent PRs, where I used a non-canonical
> modifier order. Since the problem was noticed [^1], why not to address it en
> masse?
>
> As far as I remember, the first mass-canonicalization of modifiers took place
> in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453
> files. Since then modifiers have become a bit loose, and it makes sense to
> re-bless (using the JDK-8136583 terminology) them.
>
> This change was produced by running the below command followed by updating
> the copyright years on the affected files where necessary:
>
> $ sh ./bin/blessed-modifier-order.sh src/java.base
>
> The resulting change is much smaller than that of 2015: 39 lines spanning 21
> files.
>
> [^1]:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
> [^2]:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
> _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on
> [core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_
>
> On 3/11/2021 9:26 pm, Pavel Rappo wrote:
>
> > On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz
> > wrote:
> > > > > Pragmatically, fix the script to ignore those keywords on comment
> > > > > lines. Learn Perl, its just a regular expression pattern match and
> > > > > replace expression.
> > > >
> > > >
> > > > I understand in principle how to modify that script to ignore doc
> > > > comments. The thing I was referring to when said "btw, how would we do
> > > > that?" was this: not all comment lines are prose. Some of those lines
> > > > belong to snippets of code, which I guess you would also like to be
> > > > properly formatted.
> > > > > But having seen several reviewers be unmoved by the difference, the
> > > > > real pragmatic view is to ignore the English.
> > > >
> > > >
> > > > I'm sorry you feel that way. Would it be okay if I made it clear that
> > > > those two words are not English adjectives but are special symbols that
> > > > happen to use Latin script and originate from the English words they
> > > > resemble? If so, I could enclose each of them in `{@code ... }`. If
> > > > not, I could drop that particular change from this PR.
> > >
> > >
> > > The blessed-modifier-order.sh script intentionally modifies comments,
> > > with the hope of finding code snippets (it did!)
> > > Probably I manually deleted the change to Object.java back in 2015, to
> > > avoid the sort of controversy we're seeing now.
> > > I don't have a strong feeling either way on changing that file.
> > > I agree with @pavelrappo that script-generated changes should not be
> > > mixed with manual changes.
> > > I would also not update copyright years for such changes.
> > > It's a feature of blessed-modifier-order.sh that all existing formatting
> > > is perfectly preserved.
> >
> >
> > One more thing. Please have a look at this other line in the same file;
> > this line was there before the change
> > https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
> > So before the change, the file was somewhat inconsistent. The change made
> > it consistent. **If one is going to ever revert that controversial part of
> > the change, please update both lines so that the file remains consistent.**
>
> Line 281 is (was!) consistent with line 277 because it is distinguishing a
> synchronized "static method" from a synchronized "instance method". There was
> no need to make any change to line 281 because of the blessed-order of
> modifiers as defined for method declarations! This text is just prose. Now
> for consistency you should change line 277 to refer to a "non-static
> synchronized method" (as "instance synchronized method" would be truly awful).
Thanks, David. You've provided a clear and convincing argument, and I can see
the inconsistency I introduced. I can revert that particular piece back if you
think that it would be appropriate.
That said, this line will have to be filtered out every time the script is run.
I could probably provide a more involved script that harnesses the power of AST
(com.sun.source.doctree) to try to filter out prose, but it would be impossible
to beat the simplicity of the current script; and simplicity is also important.
> Line 49 places "static synchronized" in code font, implying that it is
> referring to the actual method modifiers and as such using the blessed order
> is appropriate. Line 49 does not need to be "consistent" with line 281. If
> line 49 used a normal font so the words "static" and "synchronized" were just
> prose then either order would be perfectly fine in terms of English language,
> but then you could make a case for having it be consistent with line 281.
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v16]
On Wed, 3 Nov 2021 13:08:55 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 incrementally with one > additional commit since the last revision: > > Make ArenaAllocator impl more flexible in the face of OOME > An ArenaAllocator should remain open for business, even if OOME is thrown > in case other allocations can fit the arena size. Marked as reviewed by jvernee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:17:09 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
>> line 3544:
>>
>>> 3542: BigDecimal valueBD =
>>> BigDecimal.valueOf(value).subtract(minBD);
>>> 3543: BigDecimal fraction = valueBD.divide(rangeBD, 9,
>>> RoundingMode.FLOOR);
>>> 3544: // stripTrailingZeros bug
>>
>> I believe this bug was fixed a while back, so this code could be simplified.
>
> Got a reference to which bug this was and how it manifests?
If you're referring to JDK-6480539: "BigDecimal.stripTrailingZeros() has no
effect on zero itself ("0.0")", it was fixed in JDK 8.
-
PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v12]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Merge branch 'master' into JDK-8244202_JEP418_impl - Merge branch 'master' into JDK-8244202_JEP418_impl - Replace 'system' with 'the system-wide', move comment section - Add @ throws NPE to hosts file resolver javadoc - Changes to address review comments - Update tests to address SM deprecation - Merge branch 'master' into JDK-8244202_JEP418_impl - More javadoc updates to API classes - Review updates + move resolver docs to the provider class (CSR update to follow) - Change InetAddressResolver method names - ... and 8 more: https://git.openjdk.java.net/jdk/compare/a316c06e...0542df51 - Changes: https://git.openjdk.java.net/jdk/pull/5822/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5822=11 Stats: 3141 lines in 56 files changed: 2848 ins; 155 del; 138 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8276338: Minor improve of wording for String.to(Lower|Upper)Case
On Wed, 3 Nov 2021 11:58:42 GMT, Pavel Rappo wrote: > This PR fixes two sentences which conflate a string with its length, and also > makes the "equivalency wording" consistent. > > There are many ways to fix "the resulting String may be a different length > than the original String". The proposed way might be one of the most > lightweight ways to do that. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6232
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:44:39 GMT, Claes Redestad wrote: >> I'll see to it. > > When adding a test for this I discovered that > `FractionPrinterParser::format` will end up calling > `field.range().checkValidValue(value, field)` > [here](https://github.com/openjdk/jdk/blob/579b2c017f24f2266abefd35c2b8f28fa7268d93/src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java#L3543). > This means that the pre-existing implementation does check the value range > and throws exceptions when trying to print a `value` outside of the `field` > range. > > To mimic the existing behavior we have to do the same check in > `NanosPrinterParser::format` and drop the fallback (which would have somewhat > nonsensical output for values outside the range, anyhow). Added a test case showing that values that are outside the range throw `DateTimeException`. This passes with and without the patch and mainly documents the pre-existing behavior. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v6]
> 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: Add test verifying OOB values throw exception - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/f887c0d7..1d21a1a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=04-05 Stats: 18 lines in 1 file changed: 18 ins; 0 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) [v16]
> 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: Make ArenaAllocator impl more flexible in the face of OOME An ArenaAllocator should remain open for business, even if OOME is thrown in case other allocations can fit the arena size. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/7f847271..9fafb2a6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=14-15 Stats: 13 lines in 2 files changed: 3 ins; 6 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: 8276220: Reduce excessive allocations in DateTimeFormatter [v5]
> 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: FractionPrinterParser checks values to be in range; NanosPrinterParser should do the same. Simplify accordingly. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/579b2c01..f887c0d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=03-04 Stats: 15 lines in 2 files changed: 0 ins; 14 del; 1 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
RFR #8275063
Update this core file-- null
Re: RFR: 8276348: Use blessed modifier order in java.base
On 3/11/2021 9:26 pm, Pavel Rappo wrote:
On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz wrote:
Pragmatically, fix the script to ignore those keywords on comment lines. Learn
Perl, its just a regular expression pattern match and replace expression.
I understand in principle how to modify that script to ignore doc comments. The thing I
was referring to when said "btw, how would we do that?" was this: not all
comment lines are prose. Some of those lines belong to snippets of code, which I guess
you would also like to be properly formatted.
But having seen several reviewers be unmoved by the difference, the real
pragmatic view is to ignore the English.
I'm sorry you feel that way. Would it be okay if I made it clear that those two
words are not English adjectives but are special symbols that happen to use
Latin script and originate from the English words they resemble? If so, I could
enclose each of them in `{@code ... }`. If not, I could drop that particular
change from this PR.
The blessed-modifier-order.sh script intentionally modifies comments, with the
hope of finding code snippets (it did!)
Probably I manually deleted the change to Object.java back in 2015, to avoid
the sort of controversy we're seeing now.
I don't have a strong feeling either way on changing that file.
I agree with @pavelrappo that script-generated changes should not be mixed
with manual changes.
I would also not update copyright years for such changes.
It's a feature of blessed-modifier-order.sh that all existing formatting is
perfectly preserved.
One more thing. Please have a look at this other line in the same file; this
line was there before the change
https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
So before the change, the file was somewhat inconsistent. The change made it
consistent. **If one is going to ever revert that controversial part of the
change, please update both lines so that the file remains consistent.**
Line 281 is (was!) consistent with line 277 because it is distinguishing
a synchronized "static method" from a synchronized "instance method".
There was no need to make any change to line 281 because of the
blessed-order of modifiers as defined for method declarations! This text
is just prose. Now for consistency you should change line 277 to refer
to a "non-static synchronized method" (as "instance synchronized method"
would be truly awful).
Line 49 places "static synchronized" in code font, implying that it is
referring to the actual method modifiers and as such using the blessed
order is appropriate. Line 49 does not need to be "consistent" with line
281. If line 49 used a normal font so the words "static" and
"synchronized" were just prose then either order would be perfectly fine
in terms of English language, but then you could make a case for having
it be consistent with line 281.
Cheers,
David
-
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Active file
Submit this core file-- null
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:21:00 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
>> line 3266:
>>
>>> 3264: if (!field.range().isValidIntValue(value)) {
>>> 3265: if (fallback == null) {
>>> 3266: fallback = new FractionPrinterParser(field,
>>> minWidth, maxWidth, decimalPoint, subsequentWidth);
>>
>> It would be nice to see a test case cover this.
>
> I'll see to it.
When adding a test for this I discovered that `FractionPrinterParser::format`
will end up calling `field.range().checkValidValue(value, field)`
[here](https://github.com/openjdk/jdk/blob/579b2c017f24f2266abefd35c2b8f28fa7268d93/src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java#L3543).
This means that the pre-existing implementation does check the value range and
throws exceptions when trying to print a `value` outside of the `field` range.
To mimic the existing behavior we have to do the same check in
`NanosPrinterParser::format` and drop the fallback (which would have somewhat
nonsensical output for values outside the range, anyhow).
-
PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v11]
> 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 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 17 additional commits since the last revision: - Merge branch 'master' into JDK-8244202_JEP418_impl - Replace 'system' with 'the system-wide', move comment section - Add @ throws NPE to hosts file resolver javadoc - Changes to address review comments - Update tests to address SM deprecation - Merge branch 'master' into JDK-8244202_JEP418_impl - More javadoc updates to API classes - Review updates + move resolver docs to the provider class (CSR update to follow) - Change InetAddressResolver method names - Remove no longer used import from IPSupport - ... and 7 more: https://git.openjdk.java.net/jdk/compare/9152e3fb...b31d61d1 - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/f660cc6e..b31d61d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=09-10 Stats: 16073 lines in 487 files changed: 12211 ins; 1991 del; 1871 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v4]
> 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: Single-check idiom - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/21092323..579b2c01 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=02-03 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 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 [v3]
On Wed, 3 Nov 2021 11:53:52 GMT, Stephen Colebourne
wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add fallback for values outside the allowable range
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 3158:
>
>> 3156:
>> 3157: // only instantiated and used if there's ever a value outside
>> the allowed range
>> 3158: private FractionPrinterParser fallback;
>
> This class has to be safe in a multi-threaded environment. I'm not convinced
> it is safe right now, as the usage doesn't follow the standard racy single
> check idiom. At a minimum, there needs to be a comment explaining the
> thread-safety issues.
Yes, I'll make sure to read into a local variable.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 3266:
>
>> 3264: if (!field.range().isValidIntValue(value)) {
>> 3265: if (fallback == null) {
>> 3266: fallback = new FractionPrinterParser(field,
>> minWidth, maxWidth, decimalPoint, subsequentWidth);
>
> It would be nice to see a test case cover this.
I'll see to it.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 3290:
>
>> 3288: range.checkValidValue(value, field);
>> 3289: BigDecimal minBD = BigDecimal.valueOf(range.getMinimum());
>> 3290: BigDecimal rangeBD =
>> BigDecimal.valueOf(range.getMaximum()).subtract(minBD).add(BigDecimal.ONE);
>
> I wouldn't be surprised if there is a way to replace the use of `BigDecimal`
> with calculations using `long`. Fundamentally, calculations like 15/60 ->
> 0.25 are not hard, but it depends on whether the exact results can be matched
> across a wide range of possible inputs.
I think the main engineering challenge is writing tests that ensure that we
don't lose precision on an arbitrary fractional field.
-
PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:04:10 GMT, Stephen Colebourne wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add fallback for values outside the allowable range > > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > line 3544: > >> 3542: BigDecimal valueBD = >> BigDecimal.valueOf(value).subtract(minBD); >> 3543: BigDecimal fraction = valueBD.divide(rangeBD, 9, >> RoundingMode.FLOOR); >> 3544: // stripTrailingZeros bug > > I believe this bug was fixed a while back, so this code could be simplified. Got a reference to which bug this was and how it manifests? - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Tue, 2 Nov 2021 11:03:02 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
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add fallback for values outside the allowable range
Changes requested by scolebourne (Author).
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
3158:
> 3156:
> 3157: // only instantiated and used if there's ever a value outside
> the allowed range
> 3158: private FractionPrinterParser fallback;
This class has to be safe in a multi-threaded environment. I'm not convinced it
is safe right now, as the usage doesn't follow the standard racy single check
idiom. At a minimum, there needs to be a comment explaining the thread-safety
issues.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
3266:
> 3264: if (!field.range().isValidIntValue(value)) {
> 3265: if (fallback == null) {
> 3266: fallback = new FractionPrinterParser(field,
> minWidth, maxWidth, decimalPoint, subsequentWidth);
It would be nice to see a test case cover this.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
3290:
> 3288: range.checkValidValue(value, field);
> 3289: BigDecimal minBD = BigDecimal.valueOf(range.getMinimum());
> 3290: BigDecimal rangeBD =
> BigDecimal.valueOf(range.getMaximum()).subtract(minBD).add(BigDecimal.ONE);
I wouldn't be surprised if there is a way to replace the use of `BigDecimal`
with calculations using `long`. Fundamentally, calculations like 15/60 -> 0.25
are not hard, but it depends on whether the exact results can be matched across
a wide range of possible inputs.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
3544:
> 3542: BigDecimal valueBD =
> BigDecimal.valueOf(value).subtract(minBD);
> 3543: BigDecimal fraction = valueBD.divide(rangeBD, 9,
> RoundingMode.FLOOR);
> 3544: // stripTrailingZeros bug
I believe this bug was fixed a while back, so this code could be simplified.
-
PR: https://git.openjdk.java.net/jdk/pull/6188
RFR: 8276338: Minor improve of wording for String.to(Lower|Upper)Case
This PR fixes two sentences which conflate a string with its length, and also makes the "equivalency wording" consistent. There are many ways to fix "the resulting String may be a different length than the original String". The proposed way might be one of the most lightweight ways to do that. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/6232/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6232=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276338 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6232.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6232/head:pull/6232 PR: https://git.openjdk.java.net/jdk/pull/6232
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v15]
> 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: Simplify ArenaAllocator impl. The arena should respect its boundaries and never allocate more memory than its size specifies. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/c219ae12..7f847271 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=13-14 Stats: 40 lines in 1 file changed: 8 ins; 15 del; 17 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: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz wrote:
>>> Pragmatically, fix the script to ignore those keywords on comment lines.
>>> Learn Perl, its just a regular expression pattern match and replace
>>> expression.
>>
>> I understand in principle how to modify that script to ignore doc comments.
>> The thing I was referring to when said "btw, how would we do that?" was
>> this: not all comment lines are prose. Some of those lines belong to
>> snippets of code, which I guess you would also like to be properly formatted.
>>
>>> But having seen several reviewers be unmoved by the difference, the real
>>> pragmatic view is to ignore the English.
>>
>> I'm sorry you feel that way. Would it be okay if I made it clear that those
>> two words are not English adjectives but are special symbols that happen to
>> use Latin script and originate from the English words they resemble? If so,
>> I could enclose each of them in `{@code ... }`. If not, I could drop that
>> particular change from this PR.
>
> The blessed-modifier-order.sh script intentionally modifies comments, with
> the hope of finding code snippets (it did!)
>
> Probably I manually deleted the change to Object.java back in 2015, to avoid
> the sort of controversy we're seeing now.
> I don't have a strong feeling either way on changing that file.
>
> I agree with @pavelrappo that script-generated changes should not be mixed
> with manual changes.
> I would also not update copyright years for such changes.
>
> It's a feature of blessed-modifier-order.sh that all existing formatting is
> perfectly preserved.
One more thing. Please have a look at this other line in the same file; this
line was there before the change
https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
So before the change, the file was somewhat inconsistent. The change made it
consistent. **If one is going to ever revert that controversial part of the
change, please update both lines so that the file remains consistent.**
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Integrated: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it en > masse? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit loose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html This pull request has now been integrated. Changeset: 61506336 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/615063364ab6bdd3fa83401745e05b45e13eacdb Stats: 39 lines in 21 files changed: 0 ins; 0 del; 39 mod 8276348: Use blessed modifier order in java.base Reviewed-by: dfuchs, darcy, iris, rriggs, martin - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v3]
On Tue, 2 Nov 2021 06:25:33 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 `java/lang/StrictMath`, `java/lang/Math` >> - [x] Linux x86_64 fastdebug `tier1` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Keep intrinsics on StrictMath Marked as reviewed by aph (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6184
Re: RFR: 8276217: Harmonize StrictMath intrinsics handling [v3]
On Tue, 2 Nov 2021 06:25:33 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 `java/lang/StrictMath`, `java/lang/Math` >> - [x] Linux x86_64 fastdebug `tier1` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Keep intrinsics on StrictMath Thanks! I re-ran the tests, they seem to be fine. I need a second (R)eviewer for this. - PR: https://git.openjdk.java.net/jdk/pull/6184
