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