Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy
On Tue, 16 Feb 2021 17:20:37 GMT, Julia Boes wrote: >> 8080272 Refactor I/O stream copying to use >> InputStream.transferTo/readAllBytes and Files.copy > > Hi @turbanoff, I'm happy to sponsor but I see two comments by @marschall - > have they been addressed? > https://github.com/openjdk/jdk/pull/1853#discussion_r572815422 > https://github.com/openjdk/jdk/pull/1853#discussion_r572380746 @FrauBoes fixed in the last commit. Is there any way to de-_integrate_ PR (to include last commit)? - PR: https://git.openjdk.java.net/jdk/pull/1853
RFR: 8261940: Fix references to IOException in BigDecimal javadoc
Noticed by some of Jon Gibbons's doc linting & checking tooling, this changeset fixes two javadoc issues for BigDecimal's serialization-related methods, improving the serial form page. - Commit messages: - 8261940: Fix references to IOException in BigDecimal javadoc Changes: https://git.openjdk.java.net/jdk/pull/2623/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2623&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261940 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2623.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2623/head:pull/2623 PR: https://git.openjdk.java.net/jdk/pull/2623
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v2]
On Fri, 12 Feb 2021 15:22:15 GMT, Roger Riggs wrote: > > > The table is informative and should not be construed as specification. > The wording "has supported" should be sufficient. If this is not specification then doesn't that imply that any provider of any version of OpenJDK would be free to support, or not, whatever version of Unicode that they chose? Surely a minimum supported version must be part of the platform specification? - PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 8261938: ASN1Formatter.annotate should not return in the finally block [v2]
> Hi all, > > ASN1Formatter.annotate should be able to throw an IOException according to > this comment [1]. > But it fails to do so due to the return [2] in the finally block, which would > swallow the IOException. > > Generally, it seems not good to put a return statement in a finally block > because it would overwrite other return-statements or Exceptions [3]. > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 > [2] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 > [3] > https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Update the copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/2620/files - new: https://git.openjdk.java.net/jdk/pull/2620/files/c0af12b1..b537e060 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620
RFR: 8261938: ASN1Formatter.annotate should not return in the finally block
Hi all, ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. Thanks. Best regards, Jie [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning - Commit messages: - 8261938: ASN1Formatter.annotate should not return in the finally block Changes: https://git.openjdk.java.net/jdk/pull/2620/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2620&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261938 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter [v2]
On Wed, 17 Feb 2021 20:21:57 GMT, Naoto Sato wrote: >> Please review this simple doc fix. A CSR will be filed accordingly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Made the additional text an @apiNote Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v3]
On Wed, 17 Feb 2021 23:10:57 GMT, Brian Burkhalter wrote: >> Please review this minor specification update to highlight that >> `File.renameTo(File)` does not modify the `File` instance on which the >> method is invoked. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 6245663: Update copyright year. Thanks. Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v3]
> Please review this minor specification update to highlight that > `File.renameTo(File)` does not modify the `File` instance on which the method > is invoked. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 6245663: Update copyright year. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2618/files - new: https://git.openjdk.java.net/jdk/pull/2618/files/43e46a38..e7ff82d2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2618&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2618&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2618.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2618/head:pull/2618 PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v2]
On Wed, 17 Feb 2021 22:12:06 GMT, Brian Burkhalter wrote: >> Please review this minor specification update to highlight that >> `File.renameTo(File)` does not modify the `File` instance on which the >> method is invoked. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 6245663: Mention 'dest' parameter in the added doc Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v2]
On Wed, 17 Feb 2021 22:24:53 GMT, Naoto Sato wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 6245663: Mention 'dest' parameter in the added doc
>
> src/java.base/share/classes/java/io/File.java line 1381:
>
>> 1379: * that the rename operation was successful. As instances of
>> {@code File}
>> 1380: * are immutable, the abstract pathname represented by this {@code
>> File}
>> 1381: * object does not itself change although the filesystem object it
>> denoted
>
> I guess you meant `file` object here, instead of `filesystem`?
No, I intended `filesystem` but in the sense of "object in the filesystem" but
it does seem awkward.
-
PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v2]
On Wed, 17 Feb 2021 22:12:06 GMT, Brian Burkhalter wrote:
>> Please review this minor specification update to highlight that
>> `File.renameTo(File)` does not modify the `File` instance on which the
>> method is invoked.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 6245663: Mention 'dest' parameter in the added doc
src/java.base/share/classes/java/io/File.java line 1381:
> 1379: * that the rename operation was successful. As instances of
> {@code File}
> 1380: * are immutable, the abstract pathname represented by this {@code
> File}
> 1381: * object does not itself change although the filesystem object it
> denoted
I guess you meant `file` object here, instead of `filesystem`?
-
PR: https://git.openjdk.java.net/jdk/pull/2618
Integrated: 8261621: Delegate Unicode history from JLS to j.l.Character
On Fri, 12 Feb 2021 02:50:35 GMT, Naoto Sato wrote: > Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. This pull request has now been integrated. Changeset: ea5bf45c Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/ea5bf45c Stats: 38 lines in 1 file changed: 36 ins; 0 del; 2 mod 8261621: Delegate Unicode history from JLS to j.l.Character Reviewed-by: bpb, joehw, rriggs, darcy - PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v2]
On Wed, 17 Feb 2021 22:04:38 GMT, Roger Riggs wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 6245663: Mention 'dest' parameter in the added doc
>
> src/java.base/share/classes/java/io/File.java line 1383:
>
>> 1381: * object does not itself change although the filesystem object it
>> denoted
>> 1382: * might have moved.
>> 1383: *
>
> Is there a way to mix in a mention of the {@ code "dest"} argument.
> "might have moved to the abstract pathname provided in the dest argument".
Good idea.
-
PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v2]
> Please review this minor specification update to highlight that > `File.renameTo(File)` does not modify the `File` instance on which the method > is invoked. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 6245663: Mention 'dest' parameter in the added doc - Changes: - all: https://git.openjdk.java.net/jdk/pull/2618/files - new: https://git.openjdk.java.net/jdk/pull/2618/files/a8e2cb1e..43e46a38 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2618&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2618&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2618.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2618/head:pull/2618 PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v2]
On Wed, 17 Feb 2021 22:08:47 GMT, Brian Burkhalter wrote:
>> Please review this minor specification update to highlight that
>> `File.renameTo(File)` does not modify the `File` instance on which the
>> method is invoked.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 6245663: Mention 'dest' parameter in the added doc
src/java.base/share/classes/java/io/File.java line 1383:
> 1381: * object does not itself change although the filesystem object it
> denoted
> 1382: * might have moved.
> 1383: *
Is there a way to mix in a mention of the {@ code "dest"} argument.
"might have moved to the abstract pathname provided in the dest argument".
-
PR: https://git.openjdk.java.net/jdk/pull/2618
RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance
Please review this minor specification update to highlight that `File.renameTo(File)` does not modify the `File` instance on which the method is invoked. - Commit messages: - 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance Changes: https://git.openjdk.java.net/jdk/pull/2618/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2618&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-6245663 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2618.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2618/head:pull/2618 PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter [v2]
On Wed, 17 Feb 2021 20:04:33 GMT, Lance Andersen wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Made the additional text an @apiNote > > Hi Naoto, > > Looks good. > > Any thoughts to making the new wording stick out more perhaps via a something > like "Note: Consider" and bolding "Note:" > > > Not sure if it is @apiNote worthy but I guess that is an option also? > Not sure if it is @APinote worthy but I guess that is an option also? Good point. Modified. - PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter [v2]
On Wed, 17 Feb 2021 20:18:56 GMT, Naoto Sato wrote: >> Please review this simple doc fix. A CSR will be filed accordingly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Made the additional text an @apiNote Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter [v2]
On Wed, 17 Feb 2021 20:18:56 GMT, Naoto Sato wrote: >> Please review this simple doc fix. A CSR will be filed accordingly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Made the additional text an @apiNote Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter [v2]
> Please review this simple doc fix. A CSR will be filed accordingly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Made the additional text an @apiNote - Changes: - all: https://git.openjdk.java.net/jdk/pull/2616/files - new: https://git.openjdk.java.net/jdk/pull/2616/files/d840b562..c93badb4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2616&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2616&range=00-01 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2616.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2616/head:pull/2616 PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v3]
On Wed, 17 Feb 2021 14:14:57 GMT, Claes Redestad wrote: >> No? This unmodifiable set here just delegates call to the backing field `c`, >> so all exceptions from `c`'s calls are just delegated, aren't they? The NPE >> will still be thrown; it's just that the stack trace will be different (i.e. >> one more level of call in the stacktrace) > > Yes, my bad. There might be more issues similar to the one @michaelhixson > points out elsewhere in this PR, so a thorough review is probably needed here. Resolving these for now. Going to punt these inconsistencies to another bug before bringing this back in. This change will focus on the idempotency of these methods vs. the classes contained in `Collections`. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter
On Wed, 17 Feb 2021 19:34:47 GMT, Naoto Sato wrote: > Please review this simple doc fix. A CSR will be filed accordingly. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter
On Wed, 17 Feb 2021 19:34:47 GMT, Naoto Sato wrote: > Please review this simple doc fix. A CSR will be filed accordingly. Hi Naoto, Looks good. Any thoughts to making the new wording stick out more perhaps via a something like "Note: Consider" and bolding "Note:" Not sure if it is @apiNote worthy but I guess that is an option also? - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v3]
> Modify the `unmodifiable*` methods in `java.util.Collections` to be > idempotent. That is, when given an immutable collection from > `java.util.ImmutableCollections` or `java.util.Collections`, these methods > will return the reference instead of creating a new immutable collection that > wraps the existing one. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Removing ImmutableCollections.* idempotency - Changes: - all: https://git.openjdk.java.net/jdk/pull/2596/files - new: https://git.openjdk.java.net/jdk/pull/2596/files/53e366a9..a9588e9c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=01-02 Stats: 38 lines in 2 files changed: 7 ins; 17 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/2596.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596 PR: https://git.openjdk.java.net/jdk/pull/2596
RFR: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"
8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now" - Commit messages: - 8261483: Try to eliminate flakiness of the test by extending its allowed runtime and reducing the memory Changes: https://git.openjdk.java.net/jdk/pull/2617/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2617&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261483 Stats: 6 lines in 3 files changed: 2 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2617/head:pull/2617 PR: https://git.openjdk.java.net/jdk/pull/2617
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter
On Wed, 17 Feb 2021 19:34:47 GMT, Naoto Sato wrote: > Please review this simple doc fix. A CSR will be filed accordingly. LGTM - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2616
RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter
Please review this simple doc fix. A CSR will be filed accordingly. - Commit messages: - 8261728: SimpleDateFormat should link to DateTimeFormatter Changes: https://git.openjdk.java.net/jdk/pull/2616/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2616&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261728 Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2616.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2616/head:pull/2616 PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v2]
On Wed, 17 Feb 2021 18:24:39 GMT, Ian Graves wrote: >> 2 remarks: >> 1. MapN's entry set extends abstract set, whose `contains` is null-friendly >> like >> https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/AbstractCollection.java#L104 >> 2. The problem of unmodifiable map's entry set not always delegating >> everything to the backing entry set still exists. >> https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/Collections.java#L1724 >> This will bypass the underlying logic when the argument is `null` or not an >> entry. >> >> The behavior pointed out by michaelhixson is the conglomeration of these 2 >> unspecified behaviors. > > This raises some interesting issues and makes me wonder if we should allow a > single-wrap of the `ImmutableCollections` classes for now to make this less > onerous. Yes -- I think in response to this it makes more sense to pull the `ImmutableCollections` classes out for now and only focus on the wrapping of the classes within `Collections` so we aren't blocked by studying and rectifying these inconsistencies. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v4]
On Wed, 17 Feb 2021 17:39:00 GMT, Naoto Sato wrote: >> Please review this doc fix to j.l.Character, which now includes the table of >> the history of supported Unicode versions. A corresponding CSR will be filed >> accordingly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added a text about the variations from the base Unicode versions. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v2]
On Wed, 17 Feb 2021 14:37:52 GMT, liach wrote: >> This sounds like an inconsistency between `Map1` and `MapN` that should >> perhaps be considered a bug that needs fixing. /ping @stuart-marks > > 2 remarks: > 1. MapN's entry set extends abstract set, whose `contains` is null-friendly > like > https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/AbstractCollection.java#L104 > 2. The problem of unmodifiable map's entry set not always delegating > everything to the backing entry set still exists. > https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/Collections.java#L1724 > This will bypass the underlying logic when the argument is `null` or not an > entry. > > The behavior pointed out by michaelhixson is the conglomeration of these 2 > unspecified behaviors. This raises some interesting issues and makes me wonder if we should allow a single-wrap of the `ImmutableCollections` classes for now to make this less onerous. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v2]
> Modify the `unmodifiable*` methods in `java.util.Collections` to be > idempotent. That is, when given an immutable collection from > `java.util.ImmutableCollections` or `java.util.Collections`, these methods > will return the reference instead of creating a new immutable collection that > wraps the existing one. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: If-block formatting - Changes: - all: https://git.openjdk.java.net/jdk/pull/2596/files - new: https://git.openjdk.java.net/jdk/pull/2596/files/e0a06bf7..53e366a9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=00-01 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/2596.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596 PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 17:24:50 GMT, Claes Redestad wrote: >> For static methods, since in java language you cannot declare static method >> in instance inner classes, I'd say making them static makes more sense >> language-wise. Also making them static reduces compiler synthetic instance >> field and constructors. > > Incidentally, Java-the-language allows static methods in inner instance > classes since JDK 16. And I'm not sure this was ever a restriction at the > JVMS level since we've been generating static methods (using ASM) into these > inner instance classes since at least JDK 9. Inner classes doesn't really exist for the JVM, it's just an attribute (in fact, a pair of attributes) that is read/write by javac (it's very similar to the way generics work). So it is Ok to have static methods in inner classes since Java 1.1 from the JVM POV with the caveat that you may not be all to call them from Java-the-language. Also since Java 11, inner classes are also nestmate and those attributes (NestHost/NestMembers) change the behavior of the VM. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Sat, 13 Feb 2021 21:30:12 GMT, Andrey Turbanov
wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added table of available algorithms.
>
> test/jdk/java/util/Random/RandomTestBsi1999.java line 227:
>
>> 225: static int checkRunStats(int[] stats) {
>> 226:int runFailure = 0;
>> 227:runFailure |= ((2267 <= stats[1]) || (stats[1] <= 2733)) ? 0
>> : 1;
>
> 1. confusing formatting.
> 2. This condition is always `true`. Looks like `&&` should be used instead of
> `||`
Correct. Changed.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v4]
> Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added a text about the variations from the base Unicode versions. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2538/files - new: https://git.openjdk.java.net/jdk/pull/2538/files/b94b41c8..c90cd663 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2538&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2538&range=02-03 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2538.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2538/head:pull/2538 PR: https://git.openjdk.java.net/jdk/pull/2538
Integrated: 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library
On Tue, 16 Feb 2021 22:27:12 GMT, Mandy Chung wrote: > Update test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java > to use ForceGC from test library which has additional instrumentation to > diagnose problem. In addition, this will get the fix for JDK-8258007. This pull request has now been integrated. Changeset: bf75a3a0 Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/bf75a3a0 Stats: 39 lines in 1 file changed: 1 ins; 37 del; 1 mod 8261851: update ReflectionCallerCacheTest.java test to use ForceGC from test library Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/2597
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v18]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 48 commits: - Merge branch 'master' into 8248862 - Flatten out use of all() - Add review edits - Added table of available algorithms. - Merge branch 'master' into 8248862 - Update SplittableRandom to remove unnecessary overrides - Updated API based on CSR review. - Update package info to credit LMX algorithm - Merge branch 'master' into 8248862 - Correct range used by nextBytes - ... and 38 more: https://git.openjdk.java.net/jdk/compare/05301f5f...16f066fe - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1292&range=17 Stats: 13341 lines in 26 files changed: 11195 ins; 2055 del; 91 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:35:02 GMT, liach wrote: >> I'll just revert them > > For static methods, since in java language you cannot declare static method > in instance inner classes, I'd say making them static makes more sense > language-wise. Also making them static reduces compiler synthetic instance > field and constructors. Incidentally, Java-the-language allows static methods in inner instance classes since JDK 16. And I'm not sure this was ever a restriction at the JVMS level since we've been generating static methods (using ASM) into these inner instance classes since at least JDK 9. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]
On Wed, 17 Feb 2021 17:19:58 GMT, Alan Bateman wrote: >> Great improvement! Looks good to me overall. I wondered about the changes in >> JLA as Alan already mentioned. > >> Right, I'm not exactly sure why the more limited changes I attempted in >> [5f4e87f](https://github.com/openjdk/jdk/commit/5f4e87f50f49e64b8616063c176ea35632b0347e) >> failed. In that change I simply changed the initialization order, which >> made the failing (closed) tests pass locally - but not in our CI. Since this >> is in initPhase1 there should be no concurrency possible. > > The Reference Handler thread is started by the initializer in > jl.ref.Reference so could be a candidate. The Finalizer thread is another but > this should VM.awaitInitLevel(1) and not touch JLA until initPhase1 is done. > but you're probably right and it would be good to make the name more explicit > when exporting it outside of the package internal use. How about > `inflateBytesToChars`? That should be okay. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]
On Tue, 16 Feb 2021 19:51:21 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert rem assertions > > Great improvement! Looks good to me overall. I wondered about the changes in > JLA as Alan already mentioned. > Right, I'm not exactly sure why the more limited changes I attempted in > [5f4e87f](https://github.com/openjdk/jdk/commit/5f4e87f50f49e64b8616063c176ea35632b0347e) > failed. In that change I simply changed the initialization order, which made > the failing (closed) tests pass locally - but not in our CI. Since this is in > initPhase1 there should be no concurrency possible. The Reference Handler thread is started by the initializer in jl.ref.Reference so could be a candidate. The Finalizer thread is another but this should VM.awaitInitLevel(1) and not touch JLA until initPhase1 is done. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
> Non-static classes hold a link to their parent classes, which in many cases > can be avoided. Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8261880: Remove static from declarations of Holder nested classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/2589/files - new: https://git.openjdk.java.net/jdk/pull/2589/files/5650cce4..c6f9cf6b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2589&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2589&range=00-01 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2589/head:pull/2589 PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:32:39 GMT, Сергей Цыпанов
wrote:
>> src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java
>> line 192:
>>
>>> 190:
>>> 191: /* Placeholder class for DelegatingMethodHandles generated ahead
>>> of time */
>>> 192: static final class Holder {}
>>
>> For the four `Holder` classes in `java.lang.invoke`, the class is generated
>> when running jlink via `java.lang.invoke.GenerateJLIClassesHelper`. To stay
>> in sync with the definition you'd have to update that code. Also, since
>> these `Holder` classes will only contain static methods and are never
>> instantiated then I'm not sure it matters whether the classes are static or
>> not.
>
> I'll just revert them
For static methods, since in java language you cannot declare static method in
instance inner classes, I'd say making them static makes more sense
language-wise. Also making them static reduces compiler synthetic instance
field and constructors.
-
PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:24:46 GMT, Claes Redestad wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8261880: Remove static from declarations of Holder nested classes
>
> src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java line
> 192:
>
>> 190:
>> 191: /* Placeholder class for DelegatingMethodHandles generated ahead of
>> time */
>> 192: static final class Holder {}
>
> For the four `Holder` classes in `java.lang.invoke`, the class is generated
> when running jlink via `java.lang.invoke.GenerateJLIClassesHelper`. To stay
> in sync with the definition you'd have to update that code. Also, since these
> `Holder` classes will only contain static methods and are never instantiated
> then I'm not sure it matters whether the classes are static or not.
I'll just revert them
-
PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible
On Tue, 16 Feb 2021 14:30:58 GMT, Сергей Цыпанов
wrote:
> Non-static classes hold a link to their parent classes, which in many cases
> can be avoided.
src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java line
192:
> 190:
> 191: /* Placeholder class for DelegatingMethodHandles generated ahead of
> time */
> 192: static final class Holder {}
For the four `Holder` classes in `java.lang.invoke`, the class is generated
when running jlink via `java.lang.invoke.GenerateJLIClassesHelper`. To stay in
sync with the definition you'd have to update that code. Also, since these
`Holder` classes will only contain static methods and are never instantiated
then I'm not sure it matters whether the classes are static or not.
-
PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v18]
On Mon, 15 Feb 2021 17:45:32 GMT, Anton Kozlov wrote: >> I'm not sure it can wait. This change turns already-messy code into >> something significantly messier, to the extent that it's not really good >> enough to go into mainline. > > The latest merge with JDK-8261071 should resolve the issue. Please take a > look. Looks much better, thanks. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v8]
On Tue, 16 Feb 2021 06:24:05 GMT, Vladimir Kempik wrote: >> This is when passing a float, yes? In the case where we have more float >> arguments than n_float_register_parameters_c. >> I don't understand why you think it's acceptable to bail in this case. Can >> you explain, please? > > it's for everything that uses less than 8 bytes on a stack( ints ( 4), > shorts(2), bytes(1), floats(4)). > currently native wrapper generation does not support such cases at all, it > needs refactoring before this can be implemented. > So when a method has more argument than can be placed in registers, we may > have issues. > > So we just bailing out to interpreter in case when a smaller (<=4 b) type is > going to be passed thru the stack. > > There was attempt to implement handling such cases but currently it requires > some hacks (like using some vectors for non-specific task) - > https://github.com/openjdk/aarch64-port/pull/3 OK. I checked and the Panama preview doesn't support direct native calls for stack arguments, so we're good for now. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 4926314: Optimize Reader.read(CharBuffer) [v7]
On Fri, 12 Feb 2021 09:18:10 GMT, Philippe Marschall wrote: >> Implement three optimiztations for Reader.read(CharBuffer) >> >> * Add a code path for heap buffers in Reader#read to use the backing array >> instead of allocating a new one. >> * Change the code path for direct buffers in Reader#read to limit the >> intermediate allocation to `TRANSFER_BUFFER_SIZE`. >> * Implement `InputStreamReader#read(CharBuffer)` and delegate to >> `StreamDecoder`. >> * Implement `StreamDecoder#read(CharBuffer)` and avoid buffer allocation. > > Philippe Marschall has updated the pull request incrementally with two > additional commits since the last revision: > > - Replace c-style array declarations > - Share work buffer between #skip and #read src/java.base/share/classes/java/io/Reader.java line 221: > 219: // if the last call to read returned -1 or the > number of bytes > 220: // requested have been read then break > 221: } while (n >= 0 && remaining > 0); The code for case that the char buffer has a backing array looks okay but I'm not sure about the direct buffer/other cases. One concern is that this is a read method, not a transferXXX method so we shouldn't be calling the underlying read several times. You'll see what I mean if you consider the scenario where you read < rem, then read again and the second read blocks or throws. I'l also concerned about "workBuffer" adding more per-stream footprint for cases where skip or read(CB) is used. Objects such as InputStreamReader are already a problem due to the underlying stream decoder. - PR: https://git.openjdk.java.net/jdk/pull/1915
RFR: 8261880: Change nested classes in java.base to static nested classes where possible
Non-static classes hold a link to their parent classes, which in many cases can be avoided. - Commit messages: - 8261880: Change nested classes in java.base to static nested classes where possible Changes: https://git.openjdk.java.net/jdk/pull/2589/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2589&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261880 Stats: 20 lines in 16 files changed: 0 ins; 0 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/2589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2589/head:pull/2589 PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v4]
On Wed, 17 Feb 2021 12:24:46 GMT, Daniel Fuchs wrote:
>> Evan Whelan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8252883: Remove ClassPathException copyright statement
>
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 45:
>
>> 43: if (!(args.length == 2 || args.length == 1)) {
>> 44: System.out.println("Usage error: expects java
>> FileHandlerAccessTest [process/thread] ");
>> 45: return;
>
> Ah - sorry - since this is a test, instead of return you should probably
> throw an exception - e.g.:
> throw new IllegalArgumentException("Usage error: expects java
> FileHandlerAccessTest [process/thread] ");
Done! Thanks Daniel
> test/jdk/java/util/logging/FileHandlerAccessTest.java line 47:
>
>> 45: return;
>> 46: }
>> 47: else if (args.length == 2) {
>
> nit: `} else if (...) {`
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v5]
> Hi, > > Please review this fix for JDK-8252883. This handles the case when an > AccessDeniedException is being thrown on Windows, due to a delay in deleting > the lock file it is trying to write to. > > This fix passes all testing. > > Kind regards, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: 8252883: Added IllegalArgumentException for incorrect usage - Changes: - all: https://git.openjdk.java.net/jdk/pull/2572/files - new: https://git.openjdk.java.net/jdk/pull/2572/files/26c56c47..b5259dcb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=03-04 Stats: 6 lines in 1 file changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2572.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572 PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
On Wed, 17 Feb 2021 14:21:37 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/util/Collections.java line 1473:
>>
>>> 1471: public static Map unmodifiableMap(Map>> extends V> m) {
>>> 1472: if(m.getClass() == UnmodifiableMap.class ||
>>> 1473:m.getClass() == ImmutableCollections.Map1.class ||
>>
>> (I'm not a reviewer.)
>>
>> I think this causes a change in behavior to this silly program.
>>
>> var map1 = Map.of(1, 2);
>> var map2 = Collections.unmodifiableMap(map1);
>>
>> try {
>> System.out.println("map1 returned " + map1.entrySet().contains(null));
>> } catch (NullPointerException e) {
>> System.out.println("map1 threw");
>> }
>>
>> try {
>> System.out.println("map2 returned " + map2.entrySet().contains(null));
>> } catch (NullPointerException e) {
>> System.out.println("map2 threw");
>> }
>>
>> With JDK 15 the output is:
>>> map1 threw
>>> map2 returned false
>>
>> With this change I think the output will be:
>>> map1 threw
>>> map2 threw
>>
>> It seems unlikely that anyone will be bit by this, but it is a change to
>> behavior and it wasn't called out in the Jira issue, so I felt it was worth
>> mentioning.
>>
>> I think it is just this one specific case that changes -- only `Map1`, and
>> only `entrySet().contains(null)`. Other sub-collections like `keySet()` and
>> `values()` and `subList(...)` already throw on `contains(null)` for both the
>> `ImmutableCollections.*` implementation and the `Collections.umodifiable*`
>> wrapper. `MapN`'s `entrySet().contains(null)` already returns `false` for
>> both.
>
> This sounds like an inconsistency between `Map1` and `MapN` that should
> perhaps be considered a bug that needs fixing. /ping @stuart-marks
2 remarks:
1. MapN's entry set extends abstract set, whose `contains` is null-friendly
like
https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/AbstractCollection.java#L104
2. The problem of unmodifiable map's entry set not always delegating everything
to the backing entry set still exists.
https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/Collections.java#L1724
This will bypass the underlying logic when the argument is `null` or not an
entry.
The behavior pointed out by michaelhixson is the conglomeration of these 2
unspecified behaviors.
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
On Wed, 17 Feb 2021 00:30:09 GMT, Michael Hixson
wrote:
>> Modify the `unmodifiable*` methods in `java.util.Collections` to be
>> idempotent. That is, when given an immutable collection from
>> `java.util.ImmutableCollections` or `java.util.Collections`, these methods
>> will return the reference instead of creating a new immutable collection
>> that wraps the existing one.
>
> src/java.base/share/classes/java/util/Collections.java line 1473:
>
>> 1471: public static Map unmodifiableMap(Map> extends V> m) {
>> 1472: if(m.getClass() == UnmodifiableMap.class ||
>> 1473:m.getClass() == ImmutableCollections.Map1.class ||
>
> (I'm not a reviewer.)
>
> I think this causes a change in behavior to this silly program.
>
> var map1 = Map.of(1, 2);
> var map2 = Collections.unmodifiableMap(map1);
>
> try {
> System.out.println("map1 returned " + map1.entrySet().contains(null));
> } catch (NullPointerException e) {
> System.out.println("map1 threw");
> }
>
> try {
> System.out.println("map2 returned " + map2.entrySet().contains(null));
> } catch (NullPointerException e) {
> System.out.println("map2 threw");
> }
>
> With JDK 15 the output is:
>> map1 threw
>> map2 returned false
>
> With this change I think the output will be:
>> map1 threw
>> map2 threw
>
> It seems unlikely that anyone will be bit by this, but it is a change to
> behavior and it wasn't called out in the Jira issue, so I felt it was worth
> mentioning.
>
> I think it is just this one specific case that changes -- only `Map1`, and
> only `entrySet().contains(null)`. Other sub-collections like `keySet()` and
> `values()` and `subList(...)` already throw on `contains(null)` for both the
> `ImmutableCollections.*` implementation and the `Collections.umodifiable*`
> wrapper. `MapN`'s `entrySet().contains(null)` already returns `false` for
> both.
This sounds like an inconsistency between `Map1` and `MapN` that should perhaps
be considered a bug that needs fixing. /ping @stuart-marks
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
On Wed, 17 Feb 2021 02:27:57 GMT, liach
wrote:
>> src/java.base/share/classes/java/util/Collections.java line 1130:
>>
>>> 1128: public static Set unmodifiableSet(Set s) {
>>> 1129: if(s.getClass() == UnmodifiableSet.class ||
>>> 1130:s.getClass() == ImmutableCollections.Set12.class ||
>>
>> These might be problematic: `ImmutableCollections.Set*` differs in behavior
>> subtly from `UnmodifiableSet`, e.g., `set.contains(null)` will throw an NPE.
>> I'd limit the check and optimization to `UnmodifiableSet` here
>
> No? This unmodifiable set here just delegates call to the backing field `c`,
> so all exceptions from `c`'s calls are just delegated, aren't they? The NPE
> will still be thrown; it's just that the stack trace will be different (i.e.
> one more level of call in the stacktrace)
Yes, my bad. There might be more issues similar to the one @michaelhixson
points out elsewhere in this PR, so a thorough review is probably needed here.
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Re: Comment on 8259896: Base64 MIME decoder should allow unrecognised characters within padding
Hello Raffaello, I have added this comment from you, to that JDK-8259896 issue. -Jaikiran On 02/02/21 12:43 am, Raffaello Giulietti wrote: Hi, in my opinion, the reporter of [1] is right in requiring that extraneous characters be discarded, even inside the padding. Indeed, the first full paragraph on [2] reads: "Any characters outside of the base64 alphabet are to be ignored in base64-encoded data." where "the base64 alphabet" also includes the padding character '=' and "base64-encoded data" extends to padding as well, because padding is an essential part of encoding. The legitimate doubt expressed in comment [3] should thus be solved in favor of a bug fix. My 2 cents Greetings Raffaello [1] https://bugs.openjdk.java.net/browse/JDK-8259896 [2] https://tools.ietf.org/html/rfc2045#page-26 [3] https://bugs.openjdk.java.net/browse/JDK-8259896?focusedCommentId=14395485&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14395485
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 22:42:22 GMT, Daniel D. Daugherty wrote:
>> Anton Kozlov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> support macos_aarch64 in hsdis
>
> src/hotspot/share/logging/logStream.hpp line 35:
>
>> 33: class LogStream : public outputStream {
>> 34: // see test/hotspot/gtest/logging/test_logStream.cpp
>> 35: friend class LogStreamTest;
>
> It's not clear why this change is made for this port.
This was done for previous implementation of W^X, for gtests be able to access
this test. This not required anymore, this hunk was reverted.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Mon, 15 Feb 2021 16:21:53 GMT, Bernhard Urban-Forster wrote: >> This is the version of w^x on-demand switch implemented by microsoft guys. >> This is enabled only for debug builds. >> @lewurm could you comment here please > > Those values can be observed in the debugger, but aren't documented or > defined in header files. > > This mode was useful for the initial bootstrap of the platform (it helped > with missing W^X transitions), but shouldn't be required anymore today. I'm > fine with removing it altogether. OK, I'm going to remove this block. So we'll be able to revert changes in globals.hpp https://github.com/openjdk/jdk/pull/2200/files#r568986339 - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v18]
> Please review the implementation of JEP 391: macOS/AArch64 Port. > > It's heavily based on existing ports to linux/aarch64, macos/x86_64, and > windows/aarch64. > > Major changes are in: > * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks > JDK-8253817, JDK-8253818) > * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary > adjustments (JDK-8253819) > * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), > required on macOS/AArch64 platform. It's implemented with > pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a > thread, so W^X mode change relates to the java thread state change (for java > threads). In most cases, JVM executes in write-only mode, except when calling > a generated stub like SafeFetch, which requires a temporary switch to > execute-only mode. The same execute-only mode is enabled when a java thread > executes in java or native states. This approach of managing W^X mode turned > out to be simple and efficient enough. > * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) Anton Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 88 commits: - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos - Re-do safefetch.hpp - Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline' into jdk-macos - stubRoutines.inline.hpp -> safefetch.hpp - Update copyrights - Merge remote-tracking branch 'upstream/jdk/master' into 8261075-stubroutines-inline - Merge remote-tracking branch 'upstream/jdk/master' into 8261075-stubroutines-inline - Extract SafeFetch32/N to stubRoutines.inline.hpp - Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp" This reverts commit b873c25f31dd21349d140b790713cc9ccb5f2dc0. - Merge pull request #9 from VladimirKempik/pull/2200 Removed unused variables - ... and 78 more: https://git.openjdk.java.net/jdk/compare/b955f85e...ab72613c - Changes: https://git.openjdk.java.net/jdk/pull/2200/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=17 Stats: 2946 lines in 74 files changed: 2861 ins; 27 del; 58 mod Patch: https://git.openjdk.java.net/jdk/pull/2200.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2200/head:pull/2200 PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 21:51:56 GMT, Daniel D. Daugherty wrote: >> Anton Kozlov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> support macos_aarch64 in hsdis > > src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp line 31: > >> 29: #include "asm/assembler.inline.hpp" >> 30: #include "oops/compressedOops.hpp" >> 31: #include "runtime/vm_version.hpp" > > It's not clear why this include needed to be added. Line 448 calls `VM_Version::features()`. It seems the declaration is included indirectly somehow on the rest of the platforms, through OS specific headers. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v4]
On Wed, 17 Feb 2021 11:50:05 GMT, Evan Whelan wrote:
>> Hi,
>>
>> Please review this fix for JDK-8252883. This handles the case when an
>> AccessDeniedException is being thrown on Windows, due to a delay in deleting
>> the lock file it is trying to write to.
>>
>> This fix passes all testing.
>>
>> Kind regards,
>> Evan
>
> Evan Whelan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8252883: Remove ClassPathException copyright statement
test/jdk/java/util/logging/FileHandlerAccessTest.java line 45:
> 43: if (!(args.length == 2 || args.length == 1)) {
> 44: System.out.println("Usage error: expects java
> FileHandlerAccessTest [process/thread] ");
> 45: return;
Ah - sorry - since this is a test, instead of return you should probably throw
an exception - e.g.:
throw new IllegalArgumentException("Usage error: expects java
FileHandlerAccessTest [process/thread] ");
test/jdk/java/util/logging/FileHandlerAccessTest.java line 47:
> 45: return;
> 46: }
> 47: else if (args.length == 2) {
nit: `} else if (...) {`
-
PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v4]
> Hi, > > Please review this fix for JDK-8252883. This handles the case when an > AccessDeniedException is being thrown on Windows, due to a delay in deleting > the lock file it is trying to write to. > > This fix passes all testing. > > Kind regards, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: 8252883: Remove ClassPathException copyright statement - Changes: - all: https://git.openjdk.java.net/jdk/pull/2572/files - new: https://git.openjdk.java.net/jdk/pull/2572/files/9472f73c..26c56c47 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2572&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2572.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572 PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v3]
On Wed, 17 Feb 2021 07:37:42 GMT, David Holmes wrote: >> Evan Whelan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8252883: Throw exception if exit code of child process is non-zero > > test/jdk/java/util/logging/FileHandlerAccessTest.java line 9: > >> 7: * published by the Free Software Foundation. Oracle designates this >> 8: * particular file as subject to the "Classpath" exception as provided >> 9: * by Oracle in the LICENSE file that accompanied this code. > > Tests do not need, and should not have, the CPE. Fixed, thanks - PR: https://git.openjdk.java.net/jdk/pull/2572
