Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
On Wed, 7 Jul 2021 16:22:25 GMT, Mandy Chung wrote: >> Hi Mandy, thanks for reviewing this. >> >>> I suggest to separate the client changes (both src and test) in a separate >>> PR for the client review. >> >> Does "client changes" means changes involving src/java.desktop and >> test/java/awt? >> >>> src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java >>> needs to be updated in JSR 166 upstream repo. Better to file a separate >>> issue for this change to ensure that gets fixed in the upstream project. >> >> Can you please paste a link for that? I'm not sure where I can find JSR 166 >> upstream repo.. >> >>> Nit: The above formatting (line 70-97) is inconsistent with the formatting >>> in line 110-124. It'd be good to use the same formatting. >> >> Restored. > >> Does "client changes" means changes involving src/java.desktop and >> test/java/awt? > > src/java.desktop, test/java/awt, and test/javax/imageio > > src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java > > needs to be updated in JSR 166 upstream repo. Better to file a separate > > issue for this change to ensure that gets fixed in the upstream project. > > Can you please paste a link for that? I'm not sure where I can find JSR 166 > upstream repo.. What I meant is to file a JBS issue for this change and revert the change in this patch. That can be fixed when the next JSR 166 changes are imported to JDK. I wasn't sure if this is the right repo: http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/ - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
On Wed, 7 Jul 2021 02:10:10 GMT, Yi Yang wrote: > Does "client changes" means changes involving src/java.desktop and > test/java/awt? src/java.desktop, test/java/awt, and test/javax/imageio - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v10]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with two additional commits since the last revision: - restore code format - restore code format - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/f43ffc3a..903f0305 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=08-09 Stats: 36 lines in 2 files changed: 0 ins; 6 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v9]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with four additional commits since the last revision: - restore code format - restore code format - restore code format - restore code format - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/ab1b509d..f43ffc3a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=07-08 Stats: 58 lines in 2 files changed: 0 ins; 10 del; 48 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
On Tue, 6 Jul 2021 19:06:43 GMT, Mandy Chung wrote: >> Yi Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> tests rely on IOOBE exception message > > test/jdk/java/lang/StringBuffer/Exceptions.java line 73: > >> 71: new StringBuffer(); >> 72: } >> 73: }); > > Nit: The above formatting (line 70-97) is inconsistent with the formatting in > line 110-124. It'd be good to use the same formatting. Hi Mandy, thanks for reviewing this. > I suggest to separate the client changes (both src and test) in a separate PR > for the client review. Does "client changes" means changes involving src/java.desktop and test/java/awt? > src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java > needs to be updated in JSR 166 upstream repo. Better to file a separate issue > for this change to ensure that gets fixed in the upstream project. Can you please paste a link for that? I'm not sure where I can find JSR 166 upstream repo.. > Nit: The above formatting (line 70-97) is inconsistent with the formatting in > line 110-124. It'd be good to use the same formatting. Restored. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
On Wed, 23 Jun 2021 03:54:55 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > tests rely on IOOBE exception message I suggest to separate the client changes (both src and test) in a separate PR for the client review. I review the rest of the files, which looks okay to me. I will take another pass carefully. src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java needs to be updated in JSR 166 upstream repo.Better to file a separate issue for this change to ensure that gets fixed in the upstream project. test/jdk/java/lang/StringBuffer/Exceptions.java line 73: > 71: new StringBuffer(); > 72: } > 73: }); Nit: The above formatting (line 70-97) is inconsistent with the formatting in line 110-124. It'd be good to use the same formatting. test/jdk/java/lang/StringBuilder/Exceptions.java line 73: > 71: new StringBuilder(); > 72: } > 73: }); Nit: it'd be good to make the formatting of all calls to tryCatch method consistent. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Mon, 5 Jul 2021 06:29:58 GMT, David Holmes wrote: >> @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest >> version? Thanks. > > @kelthuzadx I did not see any response to my query about the change in > initialization (not load) order. I also remain concerned about introducing > cross dependencies between core classes (e.g. String now uses Precondition, > so does that impact Precondition using String?) But these are things for the > core-libs folk to be concerned about, or not. > > Cheers, > David > @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest > version? Thanks. Alan and Paul are currently on vacation. I'm reviewing it. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Mon, 5 Jul 2021 06:01:23 GMT, Yi Yang wrote: >> Class loading order is different to class initialization order. >> >> There are a lot more tests than just tier1. :) I don't expect many, if any, >> tests to be looking for a specific IOOBE message, and I can't see an easy >> way to find such tests without running them. If core-libs folk are okay >> with this update then I guess we can just handle any test failures if they >> arise. But I'll run this through our tier 1-3 testing. >> >> Thanks, >> David > > @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest > version? Thanks. > @kelthuzadx I did not see any response to my query about the change in > initialization (not load) order. I also remain concerned about introducing > cross dependencies between core classes (e.g. String now uses Precondition, > so does that impact Precondition using String?) But these are things for the > core-libs folk to be concerned about, or not. > > Cheers, > David Hi David, the following are initialization orders: $./java -Xlog:class+init -version [0.255s][info][class,init] 0 Initializing 'java/lang/Object'(no method) (0x00080e18) [0.255s][info][class,init] 1 Initializing 'java/lang/CharSequence'(no method) (0x00080006ba68) [0.255s][info][class,init] 2 Initializing 'java/lang/String' (0x0008e978) [0.255s][info][class,init] 3 Initializing 'java/util/Comparator'(no method) (0x0008000fd760) [0.255s][info][class,init] 4 Initializing 'java/lang/String$CaseInsensitiveComparator'(no method) (0x000800130988) [0.255s][info][class,init] 5 Initializing 'java/lang/System' (0x00088c68) [0.256s][info][class,init] 6 Initializing 'java/lang/reflect/AnnotatedElement'(no method) (0x00081f88) [0.256s][info][class,init] 7 Initializing 'java/lang/reflect/Type'(no method) (0x00082930) [0.256s][info][class,init] 8 Initializing 'java/lang/Class' (0x00081830) [0.256s][info][class,init] 9 Initializing 'java/lang/ThreadGroup'(no method) (0x00080001d348) [0.257s][info][class,init] 10 Initializing 'java/lang/Thread' (0x00080001c470) [0.258s][info][class,init] 11 Initializing 'java/security/AccessController' (0x00089a80) [0.258s][info][class,init] 12 Initializing 'java/security/AccessControlContext' (0x0008000743c8) [0.258s][info][class,init] 13 Initializing 'java/lang/Module' (0x0008bab0) [0.259s][info][class,init] 14 Initializing 'java/lang/Module$ArchivedData' (0x0008001a05a8) [0.259s][info][class,init] 15 Initializing 'jdk/internal/misc/CDS' (0x000800018bf8) [0.259s][info][class,init] 16 Initializing 'java/lang/Iterable'(no method) (0x00080008be28) [0.259s][info][class,init] 17 Initializing 'java/util/Collection'(no method) (0x00080008c028) [0.260s][info][class,init] 18 Initializing 'java/util/AbstractCollection'(no method) (0x0008000381e0) [0.260s][info][class,init] 19 Initializing 'java/util/ImmutableCollections$AbstractImmutableCollection'(no method) (0x00080008c2f8) [0.260s][info][class,init] 20 Initializing 'java/util/Set'(no method) (0x0008000189f8) [0.260s][info][class,init] 21 Initializing 'java/util/ImmutableCollections$AbstractImmutableSet'(no method) (0x00080008d018) [0.260s][info][class,init] 22 Initializing 'java/util/ImmutableCollections$Set12'(no method) (0x00080008ba18) [0.369s][info][class,init] 23 Initializing 'jdk/internal/misc/UnsafeConstants' (0x0008000b4ea0) [0.369s][info][class,init] 24 Initializing 'java/lang/reflect/AccessibleObject' (0x00080005b4d8) [0.370s][info][class,init] 25 Initializing 'java/lang/reflect/ReflectAccess'(no method) (0x000800013610) [0.370s][info][class,init] 26 Initializing 'jdk/internal/access/SharedSecrets' (0x000800013408) [0.370s][info][class,init] 27 Initializing 'java/lang/invoke/MethodHandles' (0x000800131720) [0.371s][info][class,init] 28 Initializing 'java/lang/invoke/MemberName' (0x0008000de230) [0.371s][info][class,init] 29 Initializing 'java/lang/invoke/MemberName$Factory' (0x000800023468) [0.372s][info][class,init] 30 Initializing 'java/security/Permission'(no method) (0x000800023020) [0.372s][info][class,init] 31 Initializing 'java/security/BasicPermission'(no method) (0x000800025de8) [0.372s][info][class,init] 32 Initializing 'java/lang/reflect/ReflectPermission'(no method) (0x000800025b98) [0.372s][info][class,init] 33 Initializing 'java/lang/StringLatin1' (0x000800022e18) [0.373s][info][class,init] 34 Initializing 'java/lang/invoke/MethodHandles$Lookup' (0x0008000e3708) [0.373s][info][class,init] 35 Initializing 'java/util/Objects'(no method) (0x00080008b810) [0.374s][info][class,init] 36 Initializing 'jdk/internal/reflect/Reflection' (0x0008fb28) [0.374s][info][class,init] 37 Initializing 'java/util/ImmutableCollections' (0x00080008d420) [0.374s][info][class,init] 38 Initializing 'java/util/List'(no method)
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Mon, 5 Jul 2021 06:01:23 GMT, Yi Yang wrote: >> Class loading order is different to class initialization order. >> >> There are a lot more tests than just tier1. :) I don't expect many, if any, >> tests to be looking for a specific IOOBE message, and I can't see an easy >> way to find such tests without running them. If core-libs folk are okay >> with this update then I guess we can just handle any test failures if they >> arise. But I'll run this through our tier 1-3 testing. >> >> Thanks, >> David > > @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest > version? Thanks. @kelthuzadx I did not see any response to my query about the change in initialization (not load) order. I also remain concerned about introducing cross dependencies between core classes (e.g. String now uses Precondition, so does that impact Precondition using String?) But these are things for the core-libs folk to be concerned about, or not. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Class loading order is different to class initialization order. > > There are a lot more tests than just tier1. :) I don't expect many, if any, > tests to be looking for a specific IOOBE message, and I can't see an easy way > to find such tests without running them. If core-libs folk are okay with > this update then I guess we can just handle any test failures if they arise. > But I'll run this through our tier 1-3 testing. > > Thanks, > David @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest version? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v6]
On 6/21/2021 2:02 PM, Paul Sandoz wrote: On Mon, 21 Jun 2021 05:17:09 GMT, Yi Yang wrote: After JDK-8265518(#3615), it's possible to replace all variants of checkIndex by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: more replacement 2 All the updates to the check* methods look ok (requires some careful looking!). I cannot recall what others said about the change in exception messages. @jddarcy any advice here? Generally, the JDK does not have the text of exception message as a supported interface meant to be relied on by users. This doesn't stop developers from occasionally parsing those messages, but that is usually a sign of a missing API which we try to rectify more directly. HTH, -Joe
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: tests rely on IOOBE exception message - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/2330ee38..ab1b509d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=06-07 Stats: 104 lines in 2 files changed: 18 ins; 2 del; 84 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v7]
On Tue, 22 Jun 2021 02:58:28 GMT, Yi Yang wrote: > I found that after solving the problem that Preconditions cannot be used > during the VM startup, a series of functions such as > String.checkIndex/checkOffset/.. can also be harmlessly replaced, but this > changeset is somewhat large and may overwrite the previous review progress. > If it will confuse the current review progress for reviewers involving in > this PR, I'd like to file a new PR to do this change later. Yes, I think that helps to review. It's often easier, review-wise, to have separate PRs for specific areas, rather than keep expanding an existing PR. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v7]
On Tue, 22 Jun 2021 02:39:01 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > correct exception type; use anonymous inner classes I found that after solving the problem that Preconditions cannot be used during the VM startup, a series of functions such as String.checkIndex/checkOffset/.. can also be harmlessly replaced, but this changeset is somewhat large and may overwrite the previous review progress. If it will confuse the current review progress for reviewers involving in this PR, I'd like to file a new PR to do this change later. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v6]
On Mon, 21 Jun 2021 20:49:56 GMT, Paul Sandoz wrote: >> Yi Yang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> more replacement 2 > > src/java.base/share/classes/jdk/internal/util/Preconditions.java line 78: > >> 76: = Preconditions.outOfBoundsExceptionFormatter(new >> StringIndexOutOfBoundsExceptionProducer()); >> 77: >> 78: public static final BiFunction, >> StringIndexOutOfBoundsException> AIOOBE_FORMATTER > > Using incorrect exception type. Suggest you embed as inner class rather than > separate declaration, since they are only used in one place. Fixed. FYI: Current exception message looks like this: Exception in thread "main" java.lang.StringIndexOutOfBoundsException: Range [3, 1) out of bounds for length 6 at CheckIndex$StringIndexOutOfBoundsExceptionProducer.apply(CheckIndex.java:77) at CheckIndex$StringIndexOutOfBoundsExceptionProducer.apply(CheckIndex.java:72) at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:159) at java.base/jdk.internal.util.Preconditions$1.apply(Preconditions.java:156) at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:62) at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromToIndex(Preconditions.java:76) at java.base/jdk.internal.util.Preconditions.checkFromToIndex(Preconditions.java:295) at CheckIndex.main(CheckIndex.java:110) I think now it expresses more exception information than before(and more consistent). - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v7]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: correct exception type; use anonymous inner classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/3a8875ec..2330ee38 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=05-06 Stats: 21 lines in 1 file changed: 0 ins; 9 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v6]
On Mon, 21 Jun 2021 05:17:09 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > more replacement 2 All the updates to the check* methods look ok (requires some careful looking!). I cannot recall what others said about the change in exception messages. @jddarcy any advice here? src/java.base/share/classes/jdk/internal/util/Preconditions.java line 78: > 76: = Preconditions.outOfBoundsExceptionFormatter(new > StringIndexOutOfBoundsExceptionProducer()); > 77: > 78: public static final BiFunction, > StringIndexOutOfBoundsException> AIOOBE_FORMATTER Using incorrect exception type. Suggest you embed as inner class rather than separate declaration, since they are only used in one place. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v6]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: more replacement 2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/c8b2106e..3a8875ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=04-05 Stats: 32 lines in 2 files changed: 1 ins; 20 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v5]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: more replacements - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/154989a0..c8b2106e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=03-04 Stats: 59 lines in 8 files changed: 11 ins; 37 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v4]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: format - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/c1dd2f76..154989a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=02-03 Stats: 15 lines in 1 file changed: 4 ins; 1 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Fri, 18 Jun 2021 18:03:44 GMT, Paul Sandoz wrote:
>> Yi Yang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> restore IndexOfOufBoundsException; split exception line
>
> src/java.base/share/classes/java/util/Base64.java line 935:
>
>> 933: throw new IOException("Stream is closed");
>> 934: Preconditions.checkFromIndexSize(len, off, b.length,
>> 935:
>> Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new));
>
> `outOfBoundsExceptionFormatter` will allocate. Store the result of
> `Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new))`
> in a public static final on `Preconditions`, and replace the method ref with
> a inner class (thereby making it usable earlier at VM startup.
Thanks for the clarification! Fixed.
This incremental change does many stuff:
- Create inner classes and public static final fields within Preconditions
- Use Preconditions.check* in j.l.String
- Use Preconditions.*IOOBE_FORMATTER in java.util.zip.* classes
- Use Preconditions.*IOOBE_FORMATTER in java.util.Base64
- Use Preconditions.*IOOBE_FORMATTER in X-VarHandle.java.template and
X-VarHandleByteArrayView.java.template
- Use Preconditions.*IOOBE_FORMATTER in sun.security.provider.DigestBase
- Use Preconditions.*IOOBE_FORMATTER in sun.security.util.ArrayUtil
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v3]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: new Preconditions.IOOBE_FORMATTER - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/ec0a4d44..c1dd2f76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=01-02 Stats: 129 lines in 13 files changed: 43 ins; 40 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Fri, 18 Jun 2021 05:54:01 GMT, Yi Yang wrote:
>> After JDK-8265518(#3615), it's possible to replace all variants of
>> checkIndex by
>> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
>> the whole JDK codebase.
>
> Yi Yang has updated the pull request incrementally with one additional commit
> since the last revision:
>
> restore IndexOfOufBoundsException; split exception line
@kahatlen for cases earlier in VM startup you need to avoid method references
and lambda expressions. See the implementation of
`outOfBoundsExceptionFormatter`, and see the usage in `VarHandle` for two
examples.
Custom exception formaters should ideally be constants held in static final
fields.
I think the API complexity comes down to whether it is necessary to preserve
existing exception messages or not when converting existing code to use the
precondition methods. The API is designed to do that and composes reasonably
well for default exception messages with a non-default exceptions. We could
argue (i would!) it is preferable to have a consistent exception messages for
index out of bounds exceptions, thereby we could collapse and simplify, but
this is sometimes considered a change in behaviour.
src/java.base/share/classes/java/util/Base64.java line 935:
> 933: throw new IOException("Stream is closed");
> 934: Preconditions.checkFromIndexSize(len, off, b.length,
> 935:
> Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new));
`outOfBoundsExceptionFormatter` will allocate. Store the result of
`Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new))`
in a public static final on `Preconditions`, and replace the method ref with a
inner class (thereby making it usable earlier at VM startup.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Thu, 17 Jun 2021 15:45:47 GMT, Paul Sandoz wrote:
>> src/java.base/share/classes/java/util/Base64.java line 934:
>>
>>> 932: if (closed)
>>> 933: throw new IOException("Stream is closed");
>>> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
>>> xb) -> new ArrayIndexOutOfBoundsException());
>>
>> You might want to split this really long line too, to avoid inconsistent
>> line length in this source file.
>
> I would suggest factoring out `(xa, xb) -> new
> ArrayIndexOutOfBoundsException()` as a reusable component on `Preconditions`,
> and maybe even supplying an exception message (since it is rather useful, and
> way better than no message).
>
> See the usages of `Preconditions.outOfBoundsExceptionFormatter` (where there
> just happens to be many repeated cases of supplying AIOOBE with a message,
> that could also be consolidated, separate fix perhaps).
Ok, I've replaced
Preconditions.checkFromIndexSize(len, off, b.length, (xa, xb) -> new
ArrayIndexOutOfBoundsException());
with
Preconditions.checkFromIndexSize(len, off, b.length,
Preconditions.outOfBoundsExceptionFormatter(ArrayIndexOutOfBoundsException::new));
I am curious about the motivations of current APIs:
public static
int checkIndex(int index, int length,
BiFunction, X> oobef) {
if (index < 0 || index >= length)
throw outOfBoundsCheckIndex(oobef, index, length);
return index;
}
Are they over-engineered? When I checked all checkIndex-like patterns in the
whole jdk codebase, I found that in most cases, providing an API that accepts
custom exception should be enough for scalability:
int checkIndex(int index, int length, IndexOutOfBoundException iooe) {
if (index < 0 || index >= length)
throw iooe;
return index;
}
In addition to the ease-of-use problem, there is another problem with the
current API design.
Some methods in j.l.String are good replacement candidates for
Preconditions.check{Index,...}:
https://github.com/openjdk/jdk/blob/a051e735cda0d5ee5cb6ce0738aa549a7319a28c/src/java.base/share/classes/java/lang/String.java#L4558-L4604
But we **can not** do such replacement after my practice.
The third parameter of Preconditions.checkIndex is a BiFunction, which can be
passed a lambda as its argument. The generated lambda method exercises many
other codes like MethodHandles, j.l.Package, etc, eventually it called
j.l.String.checkIndex itself, so if we replace j.l.String.checkIndex with
`Preconditions.checkIndex(a,b,(x1,x2)->)`, a StackOverflowError would occur
at VM startup.
If there is an API that accepts user-defined exceptions, I think we can apply
Preconditions in more places.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Fri, 18 Jun 2021 05:54:01 GMT, Yi Yang wrote: >> After JDK-8265518(#3615), it's possible to replace all variants of >> checkIndex by >> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in >> the whole JDK codebase. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > restore IndexOfOufBoundsException; split exception line > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on > [serviceability-dev](mailto:serviceability-...@mail.openjdk.java.net):_ > > On 17/06/2021 8:50 pm, Alan Bateman wrote: > > > On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes > > wrote: > > > There are a lot more tests than just tier1. :) I don't expect many, if > > > any, tests to be looking for a specific IOOBE message, and I can't see an > > > easy way to find such tests without running them. If core-libs folk are > > > okay with this update then I guess we can just handle any test failures > > > if they arise. But I'll run this through our tier 1-3 testing. > > > > > > It would be good to run tier 1-3. Off hand I can't think of any tests that > > are dependent on the exception message, I think I'm more concerned about > > changing behavior (once you throw a more specific IOOBE is some of the very > > core classes then it's hard to change it because libraries get dependent on > > the more specific exception). > > tiers 1-3 on Linux-x64 passed okay. > > Any change to the exact type of exception thrown should be affirmed > through a CSR request. > > Cheers, > David Thank you David for running these tests! - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: restore IndexOfOufBoundsException; split exception line - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/593bf995..ec0a4d44 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=00-01 Stats: 30 lines in 8 files changed: 15 ins; 2 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Thu, 17 Jun 2021 10:19:43 GMT, Alan Bateman wrote:
>> Yi Yang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> restore IndexOfOufBoundsException; split exception line
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 471:
>
>> 469: */
>> 470: public int offsetByCodePoints(int index, int codePointOffset) {
>> 471: checkOffset(index, count);
>
> String.offsetByCodePoints is specified to throw IOOBE. checkOffset may throw
> the more specific StringIndexOutOfBoundsException. That's a compatible change
> but I worry that we might want to throw the less specific exception in the
> future. I only mention it because there have been cases in java.lang where
> IOOBE was specified and AIOOBE by the implementation and it has been
> problematic to touch the implementation as a result.
Yes, changing the type of thrown exception may break something. And as David
said, this also requires a CSR approval, which is a relatively long process, so
I restored the original code.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 10:21:35 GMT, Alan Bateman wrote:
>> After JDK-8265518(#3615), it's possible to replace all variants of
>> checkIndex by
>> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
>> the whole JDK codebase.
>
> src/java.base/share/classes/java/util/Base64.java line 934:
>
>> 932: if (closed)
>> 933: throw new IOException("Stream is closed");
>> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
>> xb) -> new ArrayIndexOutOfBoundsException());
>
> You might want to split this really long line too, to avoid inconsistent line
> length in this source file.
I would suggest factoring out `(xa, xb) -> new
ArrayIndexOutOfBoundsException()` as a reusable component on `Preconditions`,
and maybe even supplying an exception message (since it is rather useful, and
way better than no message).
See the usages of `Preconditions.outOfBoundsExceptionFormatter` (where there
just happens to be many repeated cases of supplying AIOOBE with a message, that
could also be consolidated, separate fix perhaps).
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On 17/06/2021 8:50 pm, Alan Bateman wrote: On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes wrote: There are a lot more tests than just tier1. :) I don't expect many, if any, tests to be looking for a specific IOOBE message, and I can't see an easy way to find such tests without running them. If core-libs folk are okay with this update then I guess we can just handle any test failures if they arise. But I'll run this through our tier 1-3 testing. It would be good to run tier 1-3. Off hand I can't think of any tests that are dependent on the exception message, I think I'm more concerned about changing behavior (once you throw a more specific IOOBE is some of the very core classes then it's hard to change it because libraries get dependent on the more specific exception). tiers 1-3 on Linux-x64 passed okay. Any change to the exact type of exception thrown should be affirmed through a CSR request. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes wrote: > There are a lot more tests than just tier1. :) I don't expect many, if any, > tests to be looking for a specific IOOBE message, and I can't see an easy way > to find such tests without running them. If core-libs folk are okay with this > update then I guess we can just handle any test failures if they arise. But > I'll run this through our tier 1-3 testing. It would be good to run tier 1-3. Off hand I can't think of any tests that are dependent on the exception message, I think I'm more concerned about changing behavior (once you throw a more specific IOOBE is some of the very core classes then it's hard to change it because libraries get dependent on the more specific exception). - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote:
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex
> by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
> the whole JDK codebase.
I looked through the changes in java.base and only spotted one case where a
different (and more specific) exception is thrown.
The changes to to files in java.util.zip lead to annoying long lines so would
be good to fix all those.
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 471:
> 469: */
> 470: public int offsetByCodePoints(int index, int codePointOffset) {
> 471: checkOffset(index, count);
String.offsetByCodePoints is specified to throw IOOBE. checkOffset may throw
the more specific StringIndexOutOfBoundsException. That's a compatible change
but I worry that we might want to throw the less specific exception in the
further because code. I only mention is because there have been cases in
java.lang where IOOBE was specified and AIOOBE by the implementation and it has
been problematic to touch the implementation as a result.
src/java.base/share/classes/java/util/Base64.java line 934:
> 932: if (closed)
> 933: throw new IOException("Stream is closed");
> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
> xb) -> new ArrayIndexOutOfBoundsException());
You might to split this really long line to avoid inconsistent line length in
this source file.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote: > After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Class loading order is different to class initialization order. There are a lot more tests than just tier1. :) I don't expect many, if any, tests to be looking for a specific IOOBE message, and I can't see an easy way to find such tests without running them. If core-libs folk are okay with this update then I guess we can just handle any test failures if they arise. But I'll run this through our tier 1-3 testing. Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 01:51:41 GMT, David Holmes wrote: > I skimmed through all these and the changes seem fine in principal. > I have two mild concerns: > > 1. How does this change the class initialization order on VM startup? > 2. Do any tests need adjusting due to potential changes in the exact message > used by the IndexOutOfBoundsException? > > Thanks, > David Hi David, your concerns are reasonable. I think this change would not affect the class initialization order, because regardless of whether the patch is applied or not, `java -Xlog:class+load -version` prints identical class initialization order(for j.l.Objects and jdk.internal.util.Preconditions) as far as I can see. [class_load.log](https://github.com/openjdk/jdk/files/6667168/class_load.log). And tier1 tests are all passed w/o any modifications. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote: > After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. I skimmed through all these and the changes seem fine in principal. I have two mild concerns: 1. How does this change the class initialization order on VM startup? 2. Do any tests need adjusting due to potential changes in the exact message used by the IndexOutOfBoundsException? Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/4507
RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
After JDK-8265518, it's possible to replace all variants of checkIndex by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in the whole JDK codebase. - Commit messages: - use checkIndex globally Changes: https://git.openjdk.java.net/jdk/pull/4507/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4507=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268698 Stats: 206 lines in 34 files changed: 31 ins; 111 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
