Okay, Vitaly, you convinced me and I replaced the combined check with
two ORed checks:
- return (((SAFE_BOUND - newCapacity) | newCapacity) < 0)
+ return (newCapacity < 0 || SAFE_BOUND - newCapacity < 0)
The webrev was updated in place:
http://cr.openjdk.java.net/~igerasim/8149330/01/webrev/
Sincerely yours,
Ivan
On 23.02.2016 3:00, Vitaly Davidovich wrote:
Yes, but my hope would be that code could be written in the
natural/"canonical" manner and the JIT could then optimize accordingly
(possibly based on the target). I looked at 8u60 C2 generated asm and
it does not do a transformation similar to the manual elimination in
your code; I played around with similar looking C++ code using gcc 5.3
and clang 3.7, and they actually sometimes (depends on what's inside
the if/else blocks themselves) generate better code with the || present.
But really, this is the JIT's domain -- it knows the target arch, it
knows whether it's an OoO cpu or not, it knows whether (highly
predictable) branch elimination can be worthwhile, it can estimate
whether logical OR version (as written above) that carries a
dependency on both sides of the expression (granted dependencies are
in registers/stack slots here) is better than letting cpu run both
sides in parallel (no data dependency), and so on. I'm not even
entirely certain the above transform yields any benefit on modern
cpus, yet it contorts the code somewhat (this particular example isn't
too bad though); that's not even taking into account that what follows
this nano optimization is an Arrays.copyOf operation, which will
almost certainly dominate the cost here.
I recall seeing Martin doing similar gymnastics in the
ArrayList/Vector changes he alluded to, but my hope would be that code
can be written in a "canonical" manner and let the JIT optimize it as
it sees fit.
Just my $.02 :)
On Mon, Feb 22, 2016 at 6:40 PM, Ivan Gerasimov
<[email protected] <mailto:[email protected]>> wrote:
On 22.02.2016 23:43, Vitaly Davidovich wrote:
165 final int SAFE_BOUND = (MAX_ARRAY_SIZE >> coder);
166 if (((SAFE_BOUND - newCapacity) | newCapacity) < 0) {
Do the hotspot compiler engineers know you guys are doing manual
branch elimination like this? :)
Well, both these checks will be performed in the common case (and
we know it in advance), so combining them together seems to be
worthwhile.
Sincerely yours,
Ivan
On Mon, Feb 22, 2016 at 2:20 PM, Ivan Gerasimov
<[email protected] <mailto:[email protected]>> wrote:
Hello!
When the capacity of a StringBuilder needs to be increased
(either due to append() or due to explicit call to
ensureCapacity()), the new capacity is calculated as "twice
the old capacity, plus 2", rounded down to Integer.MAX_VALUE:
http://docs.oracle.com/javase/8/docs/api/java/lang/StringBuilder.html#ensureCapacity-int-
Because of that, StringBuilder throws OOM early, even though
there may be room to grow.
The proposed solution is to reserve a few bytes at the top of
the range and only try to allocate them if absolutely required.
The regression test is @ignored by default, as it is too
greedy for memory.
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8149330
WEBREV:
http://cr.openjdk.java.net/~igerasim/8149330/00/webrev/
<http://cr.openjdk.java.net/%7Eigerasim/8149330/00/webrev/>
Sincerely yours,
Ivan
