RFR 8223593 : Refactor code for reallocating storage

[Roger Riggs]

Hi Ivan,

Thanks for refactoring[1] this sensitive function.

ArraySupport.java:

Line 33: Please add a period at the end of the sentence.
   I would have added a new sentence instead of mixing functions.

Line 583: Making MAX_ARRAY_SIZE public would make it accessible within 
java.base module.

Line 592: 'necessary' seems a bit vague. can you be specific.

Line 595-596: Since these are javadoc'd parameters, can you add the 
assumption that they are  non-negative implied by the asserts.

605: Does the assert growAtleastBy > 0 imply that the caller needs to 
check for zero or will get undefined behavior?
   I don't see a reason to require either atLeast or preferred to be 
non-zero or to leave the behavior undefined if they are.
   The asserts themselves are marginally useful except as documentation 
since they are inoperative in production builds but take up bytecode.

607-610:  As Peter suggested, would clearer using Math.max and would be 
intrinsified.

617:  A concern about the utility method throwing the OOM exception vs 
just returning a sentinel value
   is that this utility method will be expensive to use in other 
situations where the caller does not
   want to throw an exception and it buries the exception in a named 
method that does not clearly have to throw.
   On the pro-side, the location of the exception clearly identifies 
overflow as the cause.

ByteArrayOutputStream:
92: Please add a period at the end of the sentence.

98:  I think you've dropped the normal doubling of the buffer size that 
comes from old line 115.
   The buffer should be doubling in size, but at least minsize.

PriorityQueue.java is part of JSR 166 and the changes should be done 
upstream or deferred due to adding a dependency on a JDK 13 API.

Thanks, Roger

[1] http://cr.openjdk.java.net/~igerasim/8223593/00/webrev/index.html


On 05/10/2019 07:06 AM, Pavel Rappo wrote:
>> On 10 May 2019, at 09:52, Peter Levart <peter.levart at gmail.com> wrote:
>>
>> <snip>
>>
>> Is there a case where returning > MAX_ARRAY_SIZE will not lead to OOME?
>>
>> 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 think the proposed behaviour is equivalent to what there is now. After all,
> it's a refactoring effort and as such *should* result in equivalent behaviour.
>
> If understand you correctly, your question can be answered by answering
>
>      Why there is MAX_ARRAY_SIZE in the first place?
>
>> 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);
> I'm not an expert here, but if I understood Ivan correctly, the purpose was to
> avoid branching. Maybe intrinsified Math.max is superior in both readability and
> performance. I simply don't know. If you feel strongly about using it, you could
> maybe compare those approaches by benchmarking.
>