Thank you Pavel for careful review.
On 5/9/19 5:09 AM, Pavel Rappo wrote:
Ivan,
Many thanks for doing this! This code is both error-prone and abundant in the
JDK, which indeed makes it a perfect candidate for refactoring.
Since that particular change touches quite a few foundational parts of the
libraries I felt the urge to make sure the change doesn't introduce any
artifacts. I couldn't find any degradation in amortized performances in the
worst case scenarios.
Intent was to have a fast path for the common case: I.e. when the
capacity is increased by the 'preferred' amount.
In this scenario, work amount is minimum: One summation and two checks
if the new capacity is 1) large enough and 2) not too large.
You seem to have carefully preserved all the growth (3/2,
2) factors, and +1's for eventual increase. The fact that the tests have passed
adds a lot of confidence too.
Nevertheless, I would strongly suggest you wait for reviews from the experts in
the areas the change touches. At least for Collections and String. Even though
the core of the change seems to be area-agnostic.
On a separate note. I wonder why all assertion statements were commented out
from ArraysSupport in the first place?
Nits:
1. If `ArraysSupport` is the right place for this functionality, then I think we
should respect its formatting and align methods' arguments into a column:
public static int newCapacity(int oldCapacity,
int growAtLeastBy,
int preferredGrowBy)
2. A typo:
* @throws OutOfMemoryError if increasing {@code oldCapacity) by
* {@code growAtLeastBy} overfows.
~~~~~^
Thanks! Both formatting and the typo are fixed in-place.
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
-Pavel
--
With kind regards,
Ivan Gerasimov
