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

[Vitaly Davidovich]

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>
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
> > 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/
>>
>> Sincerely yours,
>> Ivan
>>
>
>
>