RFR: 8013395 StringBuffer.toString performance regression impacting embedded benchmarks

[Mike Duigou]

Looks good David.

Mike

On May 13 2013, at 22:10 , David Holmes wrote:

> On 14/05/2013 5:05 AM, 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.
> 
> Thank you! I had been comparing the cached case with the ancient sharing case - where there was no space penalty. As you say in the existing code we already use 2x the space at the time toString is called, so no change in that regard. Doh!
> 
>> Clearly there are other usages which could be problematic.
> 
> Yes but we think those rare enough to not be a concern.
> 
>> 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.
> 
> Right - I was still using a offset/count based mental model for String but that doesn't apply any more so we can't share directly except in one case. And from past experiences with StringBuffer issues it seems that accurately sizing the SB based on the expected final size is quite a rarity so the copy-on-write optimization is not worth pursuing (even if it were implementable in a reasonable way - thanks Peter for the additional investigation here!)
> 
> So here is hopefully final webrev:
> 
> http://cr.openjdk.java.net/~dholmes/8013395/webrev.v5/
> 
> It is the same approach as v3, but as Florian pointed out the cache should be cleared before the mutating action - just in case there is an exception.
> 
> That leaves one issue that was flagged by a couple of folks: hotspot intrinsification of specific "string" usage patterns. I tracked this down in the hotspot code and I think it only applies in situations where the StringBuffer/StringBuilder could be elided completely - and so would not be an issue here. But I'm confirming this with the hotspot compiler folk (unfortunately the optimization is not clearly documented anywhere.)
> 
> Thanks,
> David
> 
>> -Alan.