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