On Fri, 4 Dec 2020 06:50:14 GMT, Stuart Marks <sma...@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.
Nice clean description of the algorithm.
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)`
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.
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.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1617
