RFR 8223593 : Refactor code for reallocating storage

[Ivan Gerasimov]

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