RFR: 8013395 StringBuffer.toString performance regression impacting embedded benchmarks

[Peter Levart]

On 05/13/2013 09:05 PM, Alan Bateman wrote:
> On 13/05/2013 08:12, David Holmes wrote:
>> Thanks for all the feedback and discussion. I've rewritten the patch 
>> to use Peter's suggestion of caching the char[] instead of the actual 
>> String:
>>
>> http://cr.openjdk.java.net/~dholmes/8013395/webrev.v3/
>>
>> I too am concerned that any form of caching that avoids the 
>> array-copy (which is the crux of the issue) is going to incur a 2x 
>> space penalty. I can imagine old but well-behaved code that uses 
>> StringBuffer rather than StringBuilder where very long buffers are 
>> created and toString() only called once - and they may get immediate 
>> or subsequent OOME due to the extra char[]; or excessive GC activity 
>> might be induced if a lot of StringBuffers are used.
> Assuming the well-behaved case is where the StringBuffer is discarded 
> after calling toString then I wouldn't expect it to too different to 
> today. That is, you have the 2x penalty when toString is called now. 
> Clearly there are other usages which could be problematic.
>
> I'm not sure what to say about the copy-on-change-after-toString 
> approach. As the offset/count fields have been removed from String 
> then it could only be the count == value.length case. It would clearly 
> be a win in some cases but no help in others.
>
> -Alan.

Hi David, Alan,

It would be a win in cases where the capacity of StringBuilder/Buffer 
could be anticipated to match the final length of string. I tried to 
implement it just for exercise and it turns out to be tricky. Currently 
I have not been able to do it correctly without introducing some sort of 
synchronization to StringBuilder, which would severely impact 
performance, since StringBuilder is designed to be used by single 
thread. In case of sharing char array with String, the "shared" boolean 
flag would have to be checked/updated atomically together with any 
'value' de-referencing and array updating or there could be data races 
that would render String instances produced with such unsynchronized 
StringBuilder appear mutable.

While doing that, I noticed a synchronization bug in 
String.contentEquals method. If called with a StringBuffer argument 
while concurrent thread is modifying the StringBuffer, the method can 
either throw ArrayIndexOutOfBoundsException or return true even though 
the content of the StringBuffer has never been the same as the String's.

Here's a proposed patch:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.01/

Regards, Peter