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
