Re: RFR: 8068958: Timestamp.from(Instant) should throw when conversion is not possible [v2]
On Fri, 22 Dec 2023 22:55:09 GMT, Eamonn McManus wrote: >> Multiplying with `*` never produces `ArithmeticException`, so the catch in >> the existing code is never triggered. `Math.multiplyExact` does produce >> `ArithmeticException` if the multiplication overflows. So we can use that, >> and rethrow `IllegalArgumentException` as the specification says. >> >> There is a small compatibility risk, in that code may have been relying on >> the previous silent overflow, and will now get an exception. But an >> exception is surely better than the nonsense results that overflow produces. >> >> Thanks to Kurt Kluever for the test cases. > > Eamonn McManus has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments about the new test. Don't forget to update the copyright years in both files, please ;-) - PR Comment: https://git.openjdk.org/jdk/pull/17181#issuecomment-1868127480
Re: RFR: 8068958: Timestamp.from(Instant) should throw when conversion is not possible [v2]
> Multiplying with `*` never produces `ArithmeticException`, so the catch in > the existing code is never triggered. `Math.multiplyExact` does produce > `ArithmeticException` if the multiplication overflows. So we can use that, > and rethrow `IllegalArgumentException` as the specification says. > > There is a small compatibility risk, in that code may have been relying on > the previous silent overflow, and will now get an exception. But an exception > is surely better than the nonsense results that overflow produces. > > Thanks to Kurt Kluever for the test cases. Eamonn McManus has updated the pull request incrementally with one additional commit since the last revision: Address review comments about the new test. - Changes: - all: https://git.openjdk.org/jdk/pull/17181/files - new: https://git.openjdk.org/jdk/pull/17181/files/32b3de53..8f2e929c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17181&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17181&range=00-01 Stats: 20 lines in 1 file changed: 4 ins; 8 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17181.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17181/head:pull/17181 PR: https://git.openjdk.org/jdk/pull/17181
Re: RFR: 8068958: Timestamp.from(Instant) should throw when conversion is not possible [v2]
On Fri, 22 Dec 2023 09:07:31 GMT, Raffaello Giulietti
wrote:
>> Eamonn McManus has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments about the new test.
>
> test/jdk/java/sql/testng/test/sql/TimestampTests.java line 649:
>
>> 647: // The latest Instant that can be converted to a Timestamp.
>> 648: Instant instant1 = Instant.ofEpochSecond(Long.MAX_VALUE / 1000,
>> 999_999_999);
>> 649: assertEquals(instant1, Timestamp.from(instant1).toInstant());
>
> The 1st arg to `assertEquals()` should be the actual value, the 2nd should be
> the expected result. I guess you expect `instant1` to be the expected result.
> (TestNG and JUnit have opposite conventions...)
Thanks, I hadn't realized that.
> test/jdk/java/sql/testng/test/sql/TimestampTests.java line 658:
>
>> 656: } catch (IllegalArgumentException expected) {
>> 657: }
>> 658:
>
> TestNG supports a better way for expected exceptions, as an attribute of the
> @Test annotation. Maybe it can be used here for the expected failing cases?
I'm not very familiar with TestNG, as you'll have noticed. But I see it has a
better way yet, namely `expectThrows`, like JUnit's `assertThrows`. So I've
updated the code to use that. Thanks for the hint!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1435375742
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1435376042
Re: RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect
On Fri, 22 Dec 2023 21:36:32 GMT, Naoto Sato wrote: >> make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java line 2: >> >>> 1: /* >>> 2: * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights >>> reserved. >> >> 2024 already? ;-) > > Thanks, Joe. Not planning to integrate it this year, so. > There's a Japanese proverb, saying demons would laugh if speaking of the next > year, though 🙂 I know what you mean. Ugh, I mean I know nothing about next year least 来年の事を言えば鬼が笑う - PR Review Comment: https://git.openjdk.org/jdk/pull/17187#discussion_r1435370022
Re: RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect
On Fri, 22 Dec 2023 20:19:17 GMT, Joe Wang wrote:
>> This is a regression caused by the fix to
>> [JDK-8317979](https://bugs.openjdk.org/browse/JDK-8317979), where the
>> parsing of TZDB files was incorrect. With this fix, `TimeZoneNames_en.java`
>> which is generated during the build time has the following diffs from the
>> previous (incorrect) one:
>>
>> ---
>> master/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
>>2023-12-18 10:28:57
>> +++
>> tz/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
>>2023-12-22 10:09:13
>> @@ -304,11 +304,11 @@
>> };
>> final String[] Azores = new String[] {
>> "Azores Standard Time",
>> - "HMT",
>> + "",
>> "Azores Summer Time",
>> - "HMT",
>> + "",
>> "Azores Time",
>> - "HMT",
>> + "",
>> };
>> final String[] Bhutan = new String[] {
>> "Bhutan Time",
>> @@ -968,11 +968,11 @@
>> };
>> final String[] Africa_Central = new String[] {
>> "Central Africa Time",
>> - "SAST",
>> - "",
>> - "SAST",
>> + "CAT",
>> "",
>> - "SAST",
>> + "CAT",
>> + "",
>> + "CAT",
>> };
>> final String[] Africa_Eastern = new String[] {
>> "East Africa Time",
>> @@ -1016,11 +1016,11 @@
>> };
>> final String[] Europe_Western = new String[] {
>> "Western European Standard Time",
>> - "FMT",
>> - "Western European Summer Time",
>> - "FMT",
>> + "WET",
>> + "Western European Summer Time",
>> + "WEST",
>> "Western European Time",
>> - "FMT",
>> + "WET",
>> };
>> final String[] Mexico_Pacific = new String[] {
>> "Mexican Pacific Standard Time",
>> @@ -1152,11 +1152,11 @@
>> };
>> final String[] Australia_Western = new String[] {
>> "Australian Western Standard Time",
>> - "",
>> + "AWST",
>> "Australian Western Daylight Time",
>> - "",
>> + "AWDT",
>> "Western Australia Time",
>> - "",
>> + "AWT",
>> };
>> final String[] Greenland_Eastern =...
>
> make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights
>> reserved.
>
> 2024 already? ;-)
Thanks, Joe. Not planning to integrate it this year, so.
There's a Japanese proverb, saying demons would laugh if speaking of the next
year, though 🙂
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17187#discussion_r1435356191
Re: RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect
On Fri, 22 Dec 2023 18:59:20 GMT, Naoto Sato wrote:
> This is a regression caused by the fix to
> [JDK-8317979](https://bugs.openjdk.org/browse/JDK-8317979), where the parsing
> of TZDB files was incorrect. With this fix, `TimeZoneNames_en.java` which is
> generated during the build time has the following diffs from the previous
> (incorrect) one:
>
> ---
> master/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-18 10:28:57
> +++
> tz/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-22 10:09:13
> @@ -304,11 +304,11 @@
> };
> final String[] Azores = new String[] {
> "Azores Standard Time",
> - "HMT",
> + "",
> "Azores Summer Time",
> - "HMT",
> + "",
> "Azores Time",
> - "HMT",
> + "",
> };
> final String[] Bhutan = new String[] {
> "Bhutan Time",
> @@ -968,11 +968,11 @@
> };
> final String[] Africa_Central = new String[] {
> "Central Africa Time",
> - "SAST",
> - "",
> - "SAST",
> + "CAT",
> "",
> - "SAST",
> + "CAT",
> + "",
> + "CAT",
> };
> final String[] Africa_Eastern = new String[] {
> "East Africa Time",
> @@ -1016,11 +1016,11 @@
> };
> final String[] Europe_Western = new String[] {
> "Western European Standard Time",
> - "FMT",
> - "Western European Summer Time",
> - "FMT",
> + "WET",
> + "Western European Summer Time",
> + "WEST",
> "Western European Time",
> - "FMT",
> + "WET",
> };
> final String[] Mexico_Pacific = new String[] {
> "Mexican Pacific Standard Time",
> @@ -1152,11 +1152,11 @@
> };
> final String[] Australia_Western = new String[] {
> "Australian Western Standard Time",
> - "",
> + "AWST",
> "Australian Western Daylight Time",
> - "",
> + "AWDT",
> "Western Australia Time",
> - "",
> + "AWT",
> };
> final String[] Greenland_Eastern = new String[] {
> "East Greenland Standard Time",
>
> Previously, they all had wrong short names due to incorrect parsi...
Marked as reviewed by iris (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/17187#pullrequestreview-1795097842
Re: RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect
On Fri, 22 Dec 2023 18:59:20 GMT, Naoto Sato wrote:
> This is a regression caused by the fix to
> [JDK-8317979](https://bugs.openjdk.org/browse/JDK-8317979), where the parsing
> of TZDB files was incorrect. With this fix, `TimeZoneNames_en.java` which is
> generated during the build time has the following diffs from the previous
> (incorrect) one:
>
> ---
> master/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-18 10:28:57
> +++
> tz/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-22 10:09:13
> @@ -304,11 +304,11 @@
> };
> final String[] Azores = new String[] {
> "Azores Standard Time",
> - "HMT",
> + "",
> "Azores Summer Time",
> - "HMT",
> + "",
> "Azores Time",
> - "HMT",
> + "",
> };
> final String[] Bhutan = new String[] {
> "Bhutan Time",
> @@ -968,11 +968,11 @@
> };
> final String[] Africa_Central = new String[] {
> "Central Africa Time",
> - "SAST",
> - "",
> - "SAST",
> + "CAT",
> "",
> - "SAST",
> + "CAT",
> + "",
> + "CAT",
> };
> final String[] Africa_Eastern = new String[] {
> "East Africa Time",
> @@ -1016,11 +1016,11 @@
> };
> final String[] Europe_Western = new String[] {
> "Western European Standard Time",
> - "FMT",
> - "Western European Summer Time",
> - "FMT",
> + "WET",
> + "Western European Summer Time",
> + "WEST",
> "Western European Time",
> - "FMT",
> + "WET",
> };
> final String[] Mexico_Pacific = new String[] {
> "Mexican Pacific Standard Time",
> @@ -1152,11 +1152,11 @@
> };
> final String[] Australia_Western = new String[] {
> "Australian Western Standard Time",
> - "",
> + "AWST",
> "Australian Western Daylight Time",
> - "",
> + "AWDT",
> "Western Australia Time",
> - "",
> + "AWT",
> };
> final String[] Greenland_Eastern = new String[] {
> "East Greenland Standard Time",
>
> Previously, they all had wrong short names due to incorrect parsi...
Marked as reviewed by joehw (Reviewer).
make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java line 2:
> 1: /*
> 2: * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights
> reserved.
2024 already? ;-)
-
PR Review: https://git.openjdk.org/jdk/pull/17187#pullrequestreview-1795051559
PR Review Comment: https://git.openjdk.org/jdk/pull/17187#discussion_r1435330060
RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect
This is a regression caused by the fix to
[JDK-8317979](https://bugs.openjdk.org/browse/JDK-8317979), where the parsing
of TZDB files was incorrect. With this fix, `TimeZoneNames_en.java` which is
generated during the build time has the following diffs from the previous
(incorrect) one:
---
master/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
2023-12-18 10:28:57
+++
tz/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
2023-12-22 10:09:13
@@ -304,11 +304,11 @@
};
final String[] Azores = new String[] {
"Azores Standard Time",
- "HMT",
+ "",
"Azores Summer Time",
- "HMT",
+ "",
"Azores Time",
- "HMT",
+ "",
};
final String[] Bhutan = new String[] {
"Bhutan Time",
@@ -968,11 +968,11 @@
};
final String[] Africa_Central = new String[] {
"Central Africa Time",
- "SAST",
- "",
- "SAST",
+ "CAT",
"",
- "SAST",
+ "CAT",
+ "",
+ "CAT",
};
final String[] Africa_Eastern = new String[] {
"East Africa Time",
@@ -1016,11 +1016,11 @@
};
final String[] Europe_Western = new String[] {
"Western European Standard Time",
- "FMT",
- "Western European Summer Time",
- "FMT",
+ "WET",
+ "Western European Summer Time",
+ "WEST",
"Western European Time",
- "FMT",
+ "WET",
};
final String[] Mexico_Pacific = new String[] {
"Mexican Pacific Standard Time",
@@ -1152,11 +1152,11 @@
};
final String[] Australia_Western = new String[] {
"Australian Western Standard Time",
- "",
+ "AWST",
"Australian Western Daylight Time",
- "",
+ "AWDT",
"Western Australia Time",
- "",
+ "AWT",
};
final String[] Greenland_Eastern = new String[] {
"East Greenland Standard Time",
Previously, they all had wrong short names due to incorrect parsing.
-
Commit messages:
- initial commit
Changes: https://git.openjdk.org/jdk/pull/17187/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17187&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8322647
Stats: 94 lines in 3 files changed: 72 ins; 2 del; 20 mod
Patch: https://git.openjdk.org/jdk/pull/17187.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17187/head:pull/17187
PR: https://git.openjdk.org/jdk/pull/17187
Re: RFR: 8322322: Support archived full module graph when -Xbootclasspath/a is used [v2]
On Fri, 22 Dec 2023 08:40:08 GMT, Alan Bateman wrote: >> Calvin Cheung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> comments from Alan and Ioi > > src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java line 71: > >> 69: URLClassPath bootAppendUcp = (append != null && >> !append.isEmpty()) >> 70: ? new URLClassPath(append, true) >> 71: : null; > > Would you mind renaming this to bootUcp or bcp? The reason is that this is > the boot loader's class path, not the append path. The system property has > "append" in the name as it's communicating the value of -Xbootclasspath/a to > the library code, this came about when -Xbootclasspath and -Xbootclasspath/p > were removed. A full bcp would contain the path to the module image as the first path. This "append" property only contains the path specified in -Xbootclasspath/a. That's why I had bootAppendUcp as the variable name. I've changed it to `bootUcp` per your suggestion. > src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java line 76: > >> 74: BOOT_LOADER = (BootClassLoader) >> archivedClassLoaders.bootLoader(); >> 75: setArchivedServicesCatalog(BOOT_LOADER); >> 76: BOOT_LOADER.setClassPath(bootAppendUcp); > > It might be clearer to set this before calling setArchivedServicesCatalog. Fixed. Also removed the commented assert statement. - PR Review Comment: https://git.openjdk.org/jdk/pull/17178#discussion_r143527 PR Review Comment: https://git.openjdk.org/jdk/pull/17178#discussion_r1435274493
Re: RFR: 8322322: Support archived full module graph when -Xbootclasspath/a is used [v2]
> Please review this change for enabling full module graph even when > -Xbootclasspath/a is specified. The validation of -Xbootclasspath/a is > already being done in `FileMapInfo::validate_boot_class_paths()`. The full > module graph will be disabled if there's a mismatch in -Xbootclasspath/a > between dump time and runtime. > > The changes in ClassLoaders.java is for setting up the append class path for > the boot loader retrieved from the CDS archive. It is required because some > existing tests are using the `getResourceAsStream()` api. Those tests would > fail without the change. > > Passed tiers 1 - 4 testing. Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision: comments from Alan and Ioi - Changes: - all: https://git.openjdk.org/jdk/pull/17178/files - new: https://git.openjdk.org/jdk/pull/17178/files/b177c6c0..d06b98ff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17178&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17178&range=00-01 Stats: 22 lines in 3 files changed: 18 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17178.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17178/head:pull/17178 PR: https://git.openjdk.org/jdk/pull/17178
Integrated: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem
On Mon, 11 Dec 2023 15:38:28 GMT, Eirik Bjørsnøs wrote: > Please review this PR which adds validation of incorrect LOC signatures in > `ZipFileSystem`. > > `ZipFile` already rejects the case where the offset pointed to from the CEN > header does not start with the expected LOC signature. It makes sense to add > this check to `ZipFileSystem` as well. This pull request has now been integrated. Changeset: 93fedc12 Author:Eirik Bjørsnøs URL: https://git.openjdk.org/jdk/commit/93fedc12db95d1e61c17537652cac3d4e27ddf2c Stats: 15 lines in 2 files changed: 13 ins; 0 del; 2 mod 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem Reviewed-by: alanb, lancea - PR: https://git.openjdk.org/jdk/pull/17059
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 14:37:26 GMT, Vladimir Sitnikov wrote: >> I think there is a misundertanding. This PR is not intendend to reduce the >> *amount* of allocated heap, it is about sparing time by not creating >> *temporary copies*. The latter should be rather easy to check: Invoke >> `transferTo(out)` two times in a row and compare the *identity* of the two >> byte arrays passed to `out.write()`. If they stay the same, then apparently >> no *temporary copy* was created. Two achieve this, the BIS must be wrapper >> around an extendable input stream (like `FileInputStream`) so between calls >> the stream could get extended (e. g. by writing into the file). > > Markus, could you please double-check? A temporary allocation *is* an > allocation, and it is the point of the change to avoid allocation and copying > the temp data when the output stream is known to behave well. I am sure the > allocation "before the change" would be non-trivial (more than several KiB), > and after the change the allocation within .transferTo would be minimal (few > bytes) assuming the output stream has already allocated its buffer IMHO the trigger for this PR was sparing *time*, not necessarily sparing *bytes* (the default buffer size is just 8K); the latter certainly is a nice and beneficial side effect. But I may be wrong here, then the original contributor should chime in now and clarify. - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435146173
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 14:27:16 GMT, Markus KARG wrote: >> Can anybody give a hint how one can create assertions in OpenJDK test code >> that would check the amount of allocated heap for the tested method? >> >> Since the change here is "removal of an allocation", the assert in the code >> should probably allow only a very small allocation in `.transferTo`. >> >> Does `com.sun.management.ThreadMXBean#getCurrentThreadAllocatedBytes()` >> sound right for the test? > > I think there is a misundertanding. This PR is not intendend to reduce the > *amount* of allocated heap, it is about sparing time by not creating > *temporary copies*. The latter should be rather easy to check: Invoke > `transferTo(out)` two times in a row and compare the *identity* of the two > byte arrays passed to `out.write()`. If they stay the same, then apparently > no *temporary copy* was created. Two achieve this, the BIS must be wrapper > around an extendable input stream (like `FileInputStream`) so between calls > the stream could get extended (e. g. by writing into the file). Markus, could you please double-check? A temporary allocation *is* an allocation, and it is the point of the change to avoid allocation and copying the temp data when the output stream is known to behave well. I am sure the allocation "before the change" would be non-trivial (more than several KiB), and after the change the allocation within .transferTo would be minimal (few bytes) assuming the output stream has already allocated its buffer - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435120325
Re: RFR: 8316662: Remove one allocation per conversion in Double.toString(double) and Float.toString(float) [v3]
On Fri, 27 Oct 2023 12:10:07 GMT, Raffaello Giulietti wrote: >> By correctly sizing an intermediate `byte[]` and making use of the internal >> `newStringNoRepl()` method, one allocation per conversion can be avoided >> when the runtime uses compact strings. > > Raffaello Giulietti 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 three additional > commits since the last revision: > > - Merge branch 'master' into 8316662 > - Uppercase JLA. > - 8316662: Remove one allocation per conversion in Double.toString(double) > and Float.toString(float) Ping - PR Comment: https://git.openjdk.org/jdk/pull/15861#issuecomment-1867753439
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 13:53:42 GMT, Vladimir Sitnikov wrote: >> test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 68: >> >>> 66: >>> 67: var bis = new BufferedInputStream(new >>> ByteArrayInputStream(dup)); >>> 68: bis.mark(dup.length); >> >> If you take a closer look into `BIS::transferTo()` then you will notice that >> in case you call `bis.mark()` then your optimization will effectively *not >> getting called at all*, due to this line: >> https://github.com/openjdk/jdk/pull/16879/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1L643. >> So Brian's comment still applies and you should fix your test before this >> PR can be accepted for a merge (hence: you MUST get rid of `mark()`). > > Can anybody give a hint how one can create assertions in OpenJDK test code > that would check the amount of allocated heap for the tested method? > > Since the change here is "removal of an allocation", the assert in the code > should probably allow only a very small allocation in `.transferTo`. > > Does `com.sun.management.ThreadMXBean#getCurrentThreadAllocatedBytes()` sound > right for the test? I think there is a misundertanding. This PR is not intendend to reduce the *amount* of allocated heap, it is about sparing time by not creating *temporary copies*. The latter should be rather easy to check: Invoke `transferTo(out)` two times in a row and compare the *identity* of the two byte arrays passed to `out.write()`. If they stay the same, then apparently no *temporary copy* was created. Two achieve this, the BIS must be wrapper around an extendable input stream (like `FileInputStream`) so between calls the stream could get extended (e. g. by writing into the file). - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435108416
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 13:19:46 GMT, Markus KARG wrote: >> Sergey Tsypanov 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 22 additional >> commits since the last revision: >> >> - Merge branch 'master' into 8320971 >> - 8320971: Inline tests >> - 8322292: Remove TransferToTrusted.checkedOutputStream() >> - 8320971: Adjust JavaDoc >> - Merge branch 'master' into 8320971 >> - 8320971: Revert irrelevant changes >> - 8320971: Add more tests >> - 8320971: Fix JavaDoc >> - 8320971: Whitespaces >> - 8320971: Fix build >> - ... and 12 more: https://git.openjdk.org/jdk/compare/d38d95d8...ee035998 > > test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 68: > >> 66: >> 67: var bis = new BufferedInputStream(new ByteArrayInputStream(dup)); >> 68: bis.mark(dup.length); > > If you take a closer look into `BIS::transferTo()` then you will notice that > in case you call `bis.mark()` then your optimization will effectively *not > getting called at all*, due to this line: > https://github.com/openjdk/jdk/pull/16879/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1L643. > So Brian's comment still applies and you should fix your test before this PR > can be accepted for a merge (hence: you MUST get rid of `mark()`). Can anybody give a hint how one can create assertions in OpenJDK test code that would check the amount of allocated heap for the tested method? Since the change here is "removal of an allocation", the assert in the code should probably allow only a very small allocation in `.transferTo`. Does `com.sun.management.ThreadMXBean#getCurrentThreadAllocatedBytes()` sound right for the test? - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435074076
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
On Fri, 22 Dec 2023 09:55:15 GMT, Sergey Tsypanov wrote: >> It looks like we can skip copying of `byte[]` in >> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in >> `java.io`. >> >> See comment by @vlsi in >> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 > > Sergey Tsypanov 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 22 additional > commits since the last revision: > > - Merge branch 'master' into 8320971 > - 8320971: Inline tests > - 8322292: Remove TransferToTrusted.checkedOutputStream() > - 8320971: Adjust JavaDoc > - Merge branch 'master' into 8320971 > - 8320971: Revert irrelevant changes > - 8320971: Add more tests > - 8320971: Fix JavaDoc > - 8320971: Whitespaces > - 8320971: Fix build > - ... and 12 more: https://git.openjdk.org/jdk/compare/17ab55e8...ee035998 test/jdk/java/io/BufferedInputStream/TransferToTrusted.java line 68: > 66: > 67: var bis = new BufferedInputStream(new ByteArrayInputStream(dup)); > 68: bis.mark(dup.length); If you take a closer look into `BIS::transferTo()` then you will notice that in case you call `bis.mark()` then your optimization will effectively *not getting called at all*, due to this line: https://github.com/openjdk/jdk/pull/16879/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1L643. So Brian's comment still applies and you should fix your test before this PR can be accepted for a merge (hence: you MUST get rid of `mark()`). - PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1435050559
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v5]
On Thu, 21 Dec 2023 19:43:50 GMT, ExE Boss wrote:
>> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 224:
>>
>>> 222: var rt2 = mh2.type().returnType();
>>> 223: return Integer.compare(
>>> 224: rt1.isPrimitive() || rt1.isEnum() || rt1.isArray() ? 1
>>> : Iterable.class.isAssignableFrom(rt1) ? -1 : 0,
>>
>> Doesn't this put primitives, enums and arrays at the end instead of at the
>> start? I've tried this with a simple array:
>>
>> Class[] types = { int.class, String.class, List.class, long.class,
>> TimeUnit.class, byte[].class, Integer.class };
>>
>> The result of sorting:
>>
>> Class[7] { interface java.util.List, class java.lang.String, class
>> java.lang.Integer, int, long, class java.util.concurrent.TimeUnit, class [B }
>>
>> By switching the -1 and 1 I get the primitives etc. at the start:
>>
>> Class[7] { int, long, class java.util.concurrent.TimeUnit, class [B, class
>> java.lang.String, class java.lang.Integer, interface java.util.List }
>> ``
>
> The `equalator`s are joined together in reverse order, so this is actually
> correct for the current implementation:
>
>
> record Example(A a, B b, C c) {
> public static void main(String... args) {
> final var left = new Example(new A(), new B(), new C());
> final var right = new Example(new A(), new B(), new C());
>
> left.equals(right);
> // prints:
> // > C::equals()
> // > B::equals()
> // > A::equals()
> }
> }
>
> record A() {
> @Override
> public boolean equals(Object other) {
> System.out.println("A::equals()");
> return other instanceof A;
> }
> }
>
> record B() {
> @Override
> public boolean equals(Object other) {
> System.out.println("B::equals()");
> return other instanceof B;
> }
> }
>
> record C() {
> @Override
> public boolean equals(Object other) {
> System.out.println("C::equals()");
> return other instanceof C;
> }
> }
I've added the test case for the current algorithm
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17143#discussion_r1435034161
Re: RFR: 8322292: Rearrange comparison of fields in Record.equals() [v6]
> Currently if we create a record it's fields are compared in their declaration
> order. This might be ineffective in cases when two objects have "heavy"
> fields equals to each other, but different "lightweight" fields (heavy and
> lightweight in terms of comparison) e.g. primitives, enums,
> nullable/non-nulls etc.
>
> If we have declared a record like
>
> public record MyRecord(String field1, int field2) {}
>
> It's equals() looks like:
>
> public final equals(Ljava/lang/Object;)Z
>L0
> LINENUMBER 3 L0
> ALOAD 0
> ALOAD 1
> INVOKEDYNAMIC
> equals(Lcom/caspianone/openbanking/productservice/controller/MyRecord;Ljava/lang/Object;)Z
> [
> // handle kind 0x6 : INVOKESTATIC
>
> java/lang/runtime/ObjectMethods.bootstrap(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/TypeDescriptor;Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/invoke/MethodHandle;)Ljava/lang/Object;
> // arguments:
> com.caspianone.openbanking.productservice.controller.MyRecord.class,
> "field1;field2",
> // handle kind 0x1 : GETFIELD
>
> com/caspianone/openbanking/productservice/controller/MyRecord.field1(Ljava/lang/String;),
> // handle kind 0x1 : GETFIELD
> com/caspianone/openbanking/productservice/controller/MyRecord.field2(I)
> ]
> IRETURN
>L1
> LOCALVARIABLE this
> Lcom/caspianone/openbanking/productservice/controller/MyRecord; L0 L1 0
> LOCALVARIABLE o Ljava/lang/Object; L0 L1 1
> MAXSTACK = 2
> MAXLOCALS = 2
>
> This can be improved by rearranging the comparison order of the fields moving
> enums and primitives upper, and collections/arrays lower.
Sergey Tsypanov has updated the pull request incrementally with two additional
commits since the last revision:
- Merge remote-tracking branch 'origin/record-equals' into record-equals
- 8322292: Add test case
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17143/files
- new: https://git.openjdk.org/jdk/pull/17143/files/158d9a83..037a3fd1
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17143&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17143&range=04-05
Stats: 42 lines in 1 file changed: 41 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17143.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17143/head:pull/17143
PR: https://git.openjdk.org/jdk/pull/17143
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v17]
> It looks like we can skip copying of `byte[]` in > `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in > `java.io`. > > See comment by @vlsi in > https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612 Sergey Tsypanov 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 22 additional commits since the last revision: - Merge branch 'master' into 8320971 - 8320971: Inline tests - 8322292: Remove TransferToTrusted.checkedOutputStream() - 8320971: Adjust JavaDoc - Merge branch 'master' into 8320971 - 8320971: Revert irrelevant changes - 8320971: Add more tests - 8320971: Fix JavaDoc - 8320971: Whitespaces - 8320971: Fix build - ... and 12 more: https://git.openjdk.org/jdk/compare/15c2671f...ee035998 - Changes: - all: https://git.openjdk.org/jdk/pull/16879/files - new: https://git.openjdk.org/jdk/pull/16879/files/84686bc6..ee035998 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16879&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16879&range=15-16 Stats: 1284 lines in 66 files changed: 844 ins; 243 del; 197 mod Patch: https://git.openjdk.org/jdk/pull/16879.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16879/head:pull/16879 PR: https://git.openjdk.org/jdk/pull/16879
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v16]
On Thu, 21 Dec 2023 17:29:16 GMT, Brian Burkhalter wrote: >> Sergey Tsypanov 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 20 additional >> commits since the last revision: >> >> - 8322292: Remove TransferToTrusted.checkedOutputStream() >> - 8320971: Adjust JavaDoc >> - Merge branch 'master' into 8320971 >> - 8320971: Revert irrelevant changes >> - 8320971: Add more tests >> - 8320971: Fix JavaDoc >> - 8320971: Whitespaces >> - 8320971: Fix build >> - 8320971: Move IOStreams to com.sun.io >> - Merge branch 'master' into 8320971 >> - ... and 10 more: https://git.openjdk.org/jdk/compare/6b2d77cf...84686bc6 > > The test does not fail when run against the mainline (HEAD: Thu Dec 21 > 15:20:01 2023 + UTC). As previously mentioned elsewhere, normally a > deterministic test should fail before the proposed source patch is applied, > and succeed thereafter. @bplb the test doesn't fail because buffer copying is already there in BIS - PR Comment: https://git.openjdk.org/jdk/pull/16879#issuecomment-1867475999
Re: RFR: 8068958: Timestamp.from(Instant) should throw when conversion is not possible
On Thu, 21 Dec 2023 21:51:10 GMT, Eamonn McManus wrote:
> Multiplying with `*` never produces `ArithmeticException`, so the catch in
> the existing code is never triggered. `Math.multiplyExact` does produce
> `ArithmeticException` if the multiplication overflows. So we can use that,
> and rethrow `IllegalArgumentException` as the specification says.
>
> There is a small compatibility risk, in that code may have been relying on
> the previous silent overflow, and will now get an exception. But an exception
> is surely better than the nonsense results that overflow produces.
>
> Thanks to Kurt Kluever for the test cases.
Please update the copyright years in both the files.
test/jdk/java/sql/testng/test/sql/TimestampTests.java line 649:
> 647: // The latest Instant that can be converted to a Timestamp.
> 648: Instant instant1 = Instant.ofEpochSecond(Long.MAX_VALUE / 1000,
> 999_999_999);
> 649: assertEquals(instant1, Timestamp.from(instant1).toInstant());
The 1st arg to `assertEquals()` should be the actual value, the 2nd should be
the expected result. I guess you expect `instant1` to be the expected result.
(TestNG and JUnit have opposite conventions...)
test/jdk/java/sql/testng/test/sql/TimestampTests.java line 658:
> 656: } catch (IllegalArgumentException expected) {
> 657: }
> 658:
TestNG supports a better way for expected exceptions, as an attribute of the
@Test annotation. Maybe it can be used here for the expected failing cases?
-
PR Review: https://git.openjdk.org/jdk/pull/17181#pullrequestreview-1794185027
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1434867044
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1434867239
Re: RFR: 8322322: Support archived full module graph when -Xbootclasspath/a is used
On Thu, 21 Dec 2023 19:10:59 GMT, Calvin Cheung wrote: > Please review this change for enabling full module graph even when > -Xbootclasspath/a is specified. The validation of -Xbootclasspath/a is > already being done in `FileMapInfo::validate_boot_class_paths()`. The full > module graph will be disabled if there's a mismatch in -Xbootclasspath/a > between dump time and runtime. > > The changes in ClassLoaders.java is for setting up the append class path for > the boot loader retrieved from the CDS archive. It is required because some > existing tests are using the `getResourceAsStream()` api. Those tests would > fail without the change. > > Passed tiers 1 - 4 testing. Changes looks okay although running -Xbootclasspath/a is an outliner that I wouldn't expect to see at dump time or runtime, the motive here seems to be tests. src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java line 71: > 69: URLClassPath bootAppendUcp = (append != null && !append.isEmpty()) > 70: ? new URLClassPath(append, true) > 71: : null; Would you mind renaming this to bootUcp or bcp? The reason is that this is the boot loader's class path, not the append path. The system property has "append" in the name as it's communicating the value of -Xbootclasspath/a to the library code, this came about when -Xbootclasspath and -Xbootclasspath/p were removed. src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java line 76: > 74: BOOT_LOADER = (BootClassLoader) > archivedClassLoaders.bootLoader(); > 75: setArchivedServicesCatalog(BOOT_LOADER); > 76: BOOT_LOADER.setClassPath(bootAppendUcp); It might be clearer to set this before calling setArchivedServicesCatalog. - PR Review: https://git.openjdk.org/jdk/pull/17178#pullrequestreview-1794149461 PR Review Comment: https://git.openjdk.org/jdk/pull/17178#discussion_r1434844648 PR Review Comment: https://git.openjdk.org/jdk/pull/17178#discussion_r1434845035
