Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Sun, 24 Jan 2021 20:14:04 GMT, Martin Buchholz wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix typo, clarify asserts disabled, test prefGrowth==0 > > Marked as reviewed by martin (Reviewer). The new docs and tests are superb. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote:
>> This rewrites the doc of ArraysSupport.newLength, adds detail to the
>> exception message, and adds a test. In addition to some renaming and a bit
>> of refactoring of the actual code, I also made two changes of substance to
>> the code:
>>
>> 1. I fixed a problem with overflow checking. In the original code, if
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this
>> method could return a negative value. It turns out that writing tests helps
>> find bugs!
>>
>> 2. Under the old policy, if oldLength and minGrowth required a length above
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to
>> allocate an array of that length will almost certainly cause the Hotspot to
>> throw OOME because its implementation limit was exceeded. Instead, if the
>> required length is in this range, this method returns that required length.
>>
>> Separately, I'll work on retrofitting various call sites around the JDK to
>> use this method.
>
> Stuart Marks has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix typo, clarify asserts disabled, test prefGrowth==0
Marked as reviewed by martin (Reviewer).
test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 101:
> 99: fail("expected OutOfMemoryError, got normal return value of "
> + r);
> 100: } catch (OutOfMemoryError oome) {
> 101: // ok
Instead of an //ok comment, I like to give the exception a name that makes the
intent clear.
} catch (IllegalArgumentException success) {}
-
PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Fri, 4 Dec 2020 17:31:20 GMT, Stuart Marks wrote:
>> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 654:
>>
>>> 652: return SOFT_MAX_ARRAY_LENGTH;
>>> 653: } else {
>>> 654: return minLength;
>>
>> Isn't this last `else if... then.. else` the same as:
>> `return Math.max(minLength, SOFT_MAX_ARRAY_LENGTH)`
>
> It is, and I considered replacing it, but I felt that it obscured what was
> going on.
Competing parts of Martin's brain are voting for Roger's and Stuart's versions.
-
PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote:
>> This rewrites the doc of ArraysSupport.newLength, adds detail to the
>> exception message, and adds a test. In addition to some renaming and a bit
>> of refactoring of the actual code, I also made two changes of substance to
>> the code:
>>
>> 1. I fixed a problem with overflow checking. In the original code, if
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this
>> method could return a negative value. It turns out that writing tests helps
>> find bugs!
>>
>> 2. Under the old policy, if oldLength and minGrowth required a length above
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to
>> allocate an array of that length will almost certainly cause the Hotspot to
>> throw OOME because its implementation limit was exceeded. Instead, if the
>> required length is in this range, this method returns that required length.
>>
>> Separately, I'll work on retrofitting various call sites around the JDK to
>> use this method.
>
> Stuart Marks has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix typo, clarify asserts disabled, test prefGrowth==0
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 643:
> 641: return prefLength;
> 642: } else {
> 643: return hugeLength(oldLength, minGrowth);
Could add a comment like
// outline cold code
-
PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote: >> This rewrites the doc of ArraysSupport.newLength, adds detail to the >> exception message, and adds a test. In addition to some renaming and a bit >> of refactoring of the actual code, I also made two changes of substance to >> the code: >> >> 1. I fixed a problem with overflow checking. In the original code, if >> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this >> method could return a negative value. It turns out that writing tests helps >> find bugs! >> >> 2. Under the old policy, if oldLength and minGrowth required a length above >> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would >> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to >> allocate an array of that length will almost certainly cause the Hotspot to >> throw OOME because its implementation limit was exceeded. Instead, if the >> required length is in this range, this method returns that required length. >> >> Separately, I'll work on retrofitting various call sites around the JDK to >> use this method. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo, clarify asserts disabled, test prefGrowth==0 src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 631: > 629: * @param minGrowth minimum required growth amount (must be > positive) > 630: * @param prefGrowth preferred growth amount > 631: * @return the new length of the array Slightly better seems s/@return the new length of the array/@return the new array length/ - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Wed, 9 Dec 2020 00:32:37 GMT, Paul Sandoz wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix typo, clarify asserts disabled, test prefGrowth==0 > > src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 616: > >> 614: * greater than the soft maximum but does not exceed >> Integer.MAX_VALUE, the minimum >> 615: * required length is returned. Otherwise, the minimum required >> length exceeds >> 616: * Integer.MAX_VALUE, which can never be fulfilled, so this method >> throws OutOfMemoryError. > > I think you can simplify with: > > Suggestion: > > * If the preferred length exceeds the soft maximum, we use the minimum > growth > * amount. The minimum required length is determined by adding the > minimum growth > * amount to the current length. > * If the minimum required length exceeds Integer.MAX_VALUE, then this > method > * throws OutOfMemoryError. Otherwise, this method returns the soft > maximum or > * minimum required length, which ever is greater. > > Then i think it follows that `Math.max` can be used in the implementation. I agree with Paul's comment of Dec 8. Especially the s/Since/If/ I would amend with s/which ever/whichever/ but that might be my dialect of North American - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote: >> This rewrites the doc of ArraysSupport.newLength, adds detail to the >> exception message, and adds a test. In addition to some renaming and a bit >> of refactoring of the actual code, I also made two changes of substance to >> the code: >> >> 1. I fixed a problem with overflow checking. In the original code, if >> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this >> method could return a negative value. It turns out that writing tests helps >> find bugs! >> >> 2. Under the old policy, if oldLength and minGrowth required a length above >> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would >> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to >> allocate an array of that length will almost certainly cause the Hotspot to >> throw OOME because its implementation limit was exceeded. Instead, if the >> required length is in this range, this method returns that required length. >> >> Separately, I'll work on retrofitting various call sites around the JDK to >> use this method. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo, clarify asserts disabled, test prefGrowth==0 Many years ago, when I wrote the unfactored MAX_ARRAY_LENGTH code, I considered refactoring it, but didn't follow through because various implementations had too many small differences . I'm glad you made it work. I'm happy to see good tests added. Testability is a benefit of refactoring. I'm happy to see the name change to SOFT_MAX_ARRAY_LENGTH - that's a better name. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Wed, 9 Dec 2020 00:32:44 GMT, Paul Sandoz wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix typo, clarify asserts disabled, test prefGrowth==0 > > Marked as reviewed by psandoz (Reviewer). @Martin-Buchholz Any comments on this? - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote: >> This rewrites the doc of ArraysSupport.newLength, adds detail to the >> exception message, and adds a test. In addition to some renaming and a bit >> of refactoring of the actual code, I also made two changes of substance to >> the code: >> >> 1. I fixed a problem with overflow checking. In the original code, if >> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this >> method could return a negative value. It turns out that writing tests helps >> find bugs! >> >> 2. Under the old policy, if oldLength and minGrowth required a length above >> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would >> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to >> allocate an array of that length will almost certainly cause the Hotspot to >> throw OOME because its implementation limit was exceeded. Instead, if the >> required length is in this range, this method returns that required length. >> >> Separately, I'll work on retrofitting various call sites around the JDK to >> use this method. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo, clarify asserts disabled, test prefGrowth==0 Marked as reviewed by psandoz (Reviewer). src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 616: > 614: * greater than the soft maximum but does not exceed > Integer.MAX_VALUE, the minimum > 615: * required length is returned. Otherwise, the minimum required > length exceeds > 616: * Integer.MAX_VALUE, which can never be fulfilled, so this method > throws OutOfMemoryError. I think you can simplify with: Suggestion: * If the preferred length exceeds the soft maximum, we use the minimum growth * amount. The minimum required length is determined by adding the minimum growth * amount to the current length. * If the minimum required length exceeds Integer.MAX_VALUE, then this method * throws OutOfMemoryError. Otherwise, this method returns the soft maximum or * minimum required length, which ever is greater. Then i think it follows that `Math.max` can be used in the implementation. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote: >> This rewrites the doc of ArraysSupport.newLength, adds detail to the >> exception message, and adds a test. In addition to some renaming and a bit >> of refactoring of the actual code, I also made two changes of substance to >> the code: >> >> 1. I fixed a problem with overflow checking. In the original code, if >> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this >> method could return a negative value. It turns out that writing tests helps >> find bugs! >> >> 2. Under the old policy, if oldLength and minGrowth required a length above >> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would >> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to >> allocate an array of that length will almost certainly cause the Hotspot to >> throw OOME because its implementation limit was exceeded. Instead, if the >> required length is in this range, this method returns that required length. >> >> Separately, I'll work on retrofitting various call sites around the JDK to >> use this method. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo, clarify asserts disabled, test prefGrowth==0 Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 00:48:55 GMT, Stuart Marks wrote: >> The algorithm can be well defined for minGrowth and prefGrowth == 0 without >> extra checks or exceptions with a careful look at the inequality checks. >> For example, as currently coded, if both are zero, it returns >> SOFT_MAX_ARRAY_LENGTH. >> Changing the `0 < prefLength` to `0 <= prefLength` would return minGrowth >> and avoid a very large but unintentional allocation. > > That's only true if oldLength is zero. The behavior is different if oldLength > is positive. In any case, this method is called when the array needs to > **grow**, which means the caller needs to reallocate and copy. If the caller > passes zero for both minGrowth and prefGrowth, the caller doesn't need to > reallocate and copy, and there's no point in it calling this method in the > first place. Not calling for a zero value is usually an optimization, not a necessity to work around an inconsistency or unpredictability in the API or the range of arguments it accepts. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
> This rewrites the doc of ArraysSupport.newLength, adds detail to the > exception message, and adds a test. In addition to some renaming and a bit of > refactoring of the actual code, I also made two changes of substance to the > code: > > 1. I fixed a problem with overflow checking. In the original code, if > oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this > method could return a negative value. It turns out that writing tests helps > find bugs! > > 2. Under the old policy, if oldLength and minGrowth required a length above > SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would > return Integer.MAX_VALUE. That doesn't make any sense, because attempting to > allocate an array of that length will almost certainly cause the Hotspot to > throw OOME because its implementation limit was exceeded. Instead, if the > required length is in this range, this method returns that required length. > > Separately, I'll work on retrofitting various call sites around the JDK to > use this method. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: fix typo, clarify asserts disabled, test prefGrowth==0 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1617/files - new: https://git.openjdk.java.net/jdk/pull/1617/files/03b7922f..bacb5f91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1617=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1617=00-01 Stats: 3 lines in 2 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617 PR: https://git.openjdk.java.net/jdk/pull/1617
