On Fri, 4 Dec 2020 15:53:01 GMT, Roger Riggs <rri...@openjdk.org> 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.
>
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 640:
>
>> 638: int prefLength = oldLength + Math.max(minGrowth, prefGrowth);
>> // might overflow
>> 639: if (0 < prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
>> 640: return prefLength;
>
> In spite of the assert `minGrowth > 0`, that is unchecked, I would suggest
> prefLength == 0 to return prefLength.
> ```if (0 <= prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
> return prefLength;```
> Otherwise, it falls into hughLength(...) which will return the
> SOFT_MAX_ARRAY_LENGTH.
>
> It would be more robust if the algorithm was well defined if either min or
> pref were zero.
The preconditions aren't checked, because this is an internal method, and the
code size is minimized in order to help inlining. That's also why `hugeLength`
is a separate method. (I guess I could add comments to this effect.) With this
in mind it's hard to reason about robustness. If prefLength is zero, this can
only be because some precondition was violated (e.g., oldLength is negative).
If this were to occur there doesn't seem to be any advantage choosing one
undefined behavior over another.
> test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 70:
>
>> 68: { IMAX-2, 1, IMAX, IMAX-1 },
>> 69: { IMAX-1, 1, IMAX, IMAX }
>> 70: };
>
> Adding test cases for zero (0) for the min and preferred would be good to
> include and show any unpredictable behavior.
No, I don't want to add test cases that violate the preconditions. I guess I
could add a prefGrowth==0 case though.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1617
