RFR 8223593 : Refactor code for reallocating storage

[Ivan Gerasimov]

Thank you Roger so much for valuable feedback!


On 5/10/19 7:41 AM, Roger Riggs wrote:
> 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.
>
Done.

> Line 583: Making MAX_ARRAY_SIZE public would make it accessible within 
> java.base module.
>
Right, done.
I was wondering if this value could be requested from VM, so we wouldn't 
have to guess with '- 8'.

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

Okay.
Added clarification that the requirement is to return at least 
(oldCapacity + growAtleastBy), which was meant by 'necessary'.

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

> 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.
This function is only called in a context where it is known for sure 
there will be re-allocation.
Thus the assumption growAtleastBy > 0.
For the other argument, preferredGrowBy, there are no restrictions.

>   The asserts themselves are marginally useful except as documentation 
> since they are inoperative in production builds but take up bytecode.
Good point.  I commented them out (just as all other asserts in this file).
>
> 607-610:  As Peter suggested, would clearer using Math.max and would 
> be intrinsified.
>
Done.

> 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.
>
I agree, that it wouldn't be the greatest design for public API.

In this case, however, the function is specially designed to be used in 
specific context.
The branch that throws OOM is expected to be rarely executed, so having 
it in a separate method may help Hotspot avoid unnecessary optimization 
of that code.

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

> 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.
>
The code looks correct, actually.
I see, Pavel already explained that passing oldCapacity as 
preferredGrowBy results in doubling the capacity.

> 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.
>
Oops, is it?
I noticed that PriorityQueue.java has typical Oracle copyright header 
unlike the files like Deque.java or everything from concurrency package.

The mercurial history suggests that PriorityQueue.java has been modified 
via OpenJDK process, e.g.
http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/a6cbb9808e4b

I can drop the changes in this file, if we don't own this code.

With kind regards,
Ivan


> 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.
>>
>
>

-- 
With kind regards,
Ivan Gerasimov