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 [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]
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 [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
