Hi Martin,
Thanks for digging into this. There are some subtle issues here. I have a few
questions.
One is that in list.addAll(other), the sizes of list and other exceeds
Integer.MAX_VALUE, then grow(int) will be called with a negative value for
minCapacity. The code *seems* to handle this ok, but the negative minCapacity
value can get pretty deeply into the helper methods before being caught. Would
it be better to check it at the top of grow(int) and throw an exception there?
(Probably OOME.) I think it would make the subsequent code easier to follow.
It looks like there are a variety of ways for minCapacity that is positive but
greater than MAX_ARRAY_SIZE to reach newCapacity(). If this occurs, and other
conditions are right, it looks like the code will end up attempting to allocate
an array greater than MAX_ARRAY_SIZE.
One style point I noticed (which might be an issue of me not being used to it)
is the use of an elementData local variable to shadow the elementData field.
This is more-or-less ok in places where elementData is initialized from
this.elementData, but in readObject(), the local elementData is introduced in a
nested scope. This means that elementData has different meanings in different
parts of the method.
For the test Bug8146568 I think the preferred way to disable a test with extreme
memory requirements is to use @ignore; see
jdk/test/java/math/BigInteger/SymmetricRangeTests.java
for example.
s'marks
On 1/21/16 8:38 AM, Martin Buchholz wrote:
PIng. How about Alan, Chris, Xueming ?
On Tue, Jan 19, 2016 at 9:24 AM, Martin Buchholz <marti...@google.com> wrote:
Hi Stuart et al,
Please review my fix for this corner case bug, including more
importantly some performance improvements.
https://bugs.openjdk.java.net/browse/JDK-8146568
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ArrayList-grow/
