RFR 8223593 : Refactor code for reallocating storage

[Ivan Gerasimov]

Hi Peter!

Thank you for reviewing!


On 5/10/19 1:52 AM, Peter Levart wrote:
> Hi Ivan,
>
> On 5/9/19 8:07 PM, Ivan Gerasimov wrote:
>>> 3. I know that this is not new and has been copied from the old 
>>> code. However,
>>> I'm not sure I understand the meaning of "unless necessary" here:
>>>
>>>      /**
>>>       * The maximum size of array to allocate (unless necessary).
>> It means that if the minimum requested new capacity (oldCapacity + 
>> growAtLeastBy) is greater than MAX_ARRAY_SIZE (though still not 
>> greater than Integer.MAX_VALUE) then the result *will* be greater 
>> than MAX_ARRAY_SIZE.
>>
>> Current implementation returns Integer.MAX_VALUE in this case. I was 
>> thinking about changing it to returning the actual sum (oldCapacity + 
>> growAtLeastBy), but decided not to do that to preserve the behavior.
>>
>> Practically, it shouldn't matter much, as both variants would likely 
>> lead to OOM anyway.
>>
>> With kind regards,
>> Ivan 
>
> Is there a case where returning > MAX_ARRAY_SIZE will not lead to OOME?
>
Yes.  I just tried (successfully) and could allocate up to 
byte[Integer.MAX_INTEGER - 2].
Greater size result in OutOfMemoryError: Requested array size exceeds VM 
limit.

> If this utility method is meant for re-sizing arrays only (currently 
> it is only used for that), then perhaps the method could throw OOME 
> explicitly in this case. You already throw OOME for the overflow case, 
> so currently the method is not uniform in returning exceptional values 
> (i.e. values that lead to exceptions).
>
> Unless you expect some VMs to tolerate arrays as large as 
> Integer.MAX_VALUE ?
>
I prefer to go conservative with this refactoring, and preserve the 
behavior as close as possible to the current one.

Later, I think, it could be reconsidered.
For example, as I wrote earlier, it may be desirable to make 
hugeCapacity(minCapacity) return minCapacity instead of Integer.MAX_VALUE.

For now, I'd rather keep the implementation to minimize risk of regressions.

> These lines:
>
>  607         int newCapacity = oldCapacity + preferredGrowBy;
>  608         if (preferredGrowBy < growAtLeastBy) {
>  609             newCapacity = oldCapacity + growAtLeastBy;
>  610         }
>
> ...could perhaps be more easily grasped as:
>
> int newCapacity = oldCapacity + Math.max(preferredGrowBy, growAtLeastBy);
>

Okay, let's do it with Math.max().  I'll update the webrev shortly.

Thank you!

Ivan



> Regards, Peter
>
>

-- 
With kind regards,
Ivan Gerasimov