RFR: 8149330: Capacity of StringBuilder should not get close to Integer.MAX_VALUE unless necessary

[Ivan Gerasimov]

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 
> <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> 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
>>     <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> 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
>>
>>
>
>