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.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.
--
With kind regards,
Ivan Gerasimov
