RFR: 8013395 StringBuffer.toString performance regression impacting embedded benchmarks

[Peter Levart]

On 05/13/2013 10:07 AM, Remi Forax wrote:
> Hi David,
>
> On 05/13/2013 09:12 AM, 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.
>>
>> So I tried to implement a simple copy-on-write-if-shared (COWIS) scheme:
>>
>> http://cr.openjdk.java.net/~dholmes/8013395/webrev.v4/
>>
>> but I must have done something silly as the resulting JDK won't run - 
>> for some reason it causes classes and resources to not be found. :(
>
> I don't know if it's the only error but take a look to the constructor 
> String(char[], boolean),
> you will understand what's wrong.

Hi David, Rémi,

I think the problem is the toString() method (have you had the same 
thing in mind, Rémi?):

  686     @Override
  687     public synchronized String toString() {
  688         isShared = true;
  689         return new String(value, true);
  690     }

...the returned String is always constructed with the whole 'value' 
array. It should only take the 1st 'count' chars...

I think that toString array caching aspect and copy-on-write aspect can 
only be combined in situations when 'count == value.length'. Otherwise 
they are different aspects and the copy-on-write aspect which can only 
be used when count == value.length, could be useful also in 
StringBuilder, so it should perhaps be built into the 
AbstractStringBuilder logic. Although it can only be effective when 
count == value.length, the performance concious code could benefit (when 
logic can predict the final length of resulting String, the building of 
it could be performed without extra array copying).

For example, to concatenate 2 Strings most optimally, the following 
would not be doing any more copies than necessary:

new StringBuilder(s1.length() + 
s2.length()).append(s1).append(s2).toString();

Regards, Peter

>
> The other possible source of errors is that the VM has some string 
> optimizations that
> recognize patterns like buffer.append(s).append(s2).toString().
>
>>
>> David
>> -----
>
> Rémi
>
>>
>> On 11/05/2013 3:36 AM, Mike Duigou wrote:
>>> The implementation looks OK to me. I like Peter's suggestion of 
>>> caching the char[] rather than string.
>>>
>>> I do wish that the cache could be in a soft reference but understand 
>>> that there are tradeoffs to doing so. I am worried about leaks with 
>>> this implementation.
>>>
>>> Another possibility, to go totally nuts, is to consider implementing 
>>> a form of copy-on-write-following-toString. This would be the exact 
>>> opposite of "minimal set of changes to address this specific 
>>> problem" and probably not wise to attempt for Java 8.
>>>
>>> Just as an FYI, this issue has in-flight conflicts with Martin's 
>>> ongoing CharSequence.getChars patch.
>>>
>>> Mike
>>>
>>> On May 9 2013, at 23:03 , David Holmes wrote:
>>>
>>>> Short version:
>>>>
>>>> Cache the value returned by toString and use it to copy-construct a 
>>>> new String on subsequent calls to toString(). Clear the cache on 
>>>> any mutating operation.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~dholmes/8013395/webrev.v2/
>>>>
>>>> Testing: microbenchmark for toString performance; new regression 
>>>> test for correctness; JPRT testset core as a sanity check
>>>>
>>>> Still TBD - full SE benchmark (?)
>>>>
>>>> Thanks,
>>>> David
>>>> ---------
>>>>
>>>> Long version:
>>>>
>>>> One of the goals for JDK8 is to provide a path from Java ME CDC to 
>>>> Java SE (or SE Embedded). In the embedded space some pretty old 
>>>> benchmarks still get used for doing comparisons between JRE's. One 
>>>> of which makes heavy use of StringBuffer.toString, without 
>>>> modifying the StringBuffer in between.
>>>>
>>>> Up to Java 1.4.2 a StringBuffer and a String could share the 
>>>> underlying char[]. This meant that toString simply needed to create 
>>>> a new String that referenced the StringBuffer's char[] with no 
>>>> copying of the array needed. In Java 5 the String/StringBuffer 
>>>> implementations were completely revised: StringBuilder was 
>>>> introduced for non-synchronized use, and a new 
>>>> AbstractStringBuilder base class added for it and StringBuffer. In 
>>>> that implementation toString now has to copy the StringBuffer's 
>>>> char[]. This resulted in a significant performance regression for 
>>>> toString() and a bug - 6219959 - was opened. There is quite an 
>>>> elaborate evaluation in that bug report but bottom line was that 
>>>> "real code doesn't depend on this - won't fix".
>>>>
>>>> At some stage ME also updated to the new Java 5 code and they also 
>>>> noticed the problem. As a result CDC6 included a variation of the 
>>>> caching strategy that is proposed here.
>>>>
>>>> Going forward because we want people to be able to compare ME and 
>>>> SE with their familiar benchmarks, we would like to address this 
>>>> corner case and fix it using the caching strategy outlined. As a 
>>>> data point an 8K StringBuffer that takes ~1ms to be converted to a 
>>>> String initially, can process subsequent toString() calls in a few 
>>>> microseconds. So that performance issue is addressed.
>>>>
>>>> However we've added a write to a field in all the mutating methods 
>>>> which obviously adds some additional computational effort - though 
>>>> I have no doubt it is lost in the noise for all but the smallest of 
>>>> mutating methods. Even so this should be run against regular SE 
>>>> benchmarks to ensure there are no performance regressions there - 
>>>> so if anyone has a suggestion as to the best benchmark to run to 
>>>> exercise StringBuffer (if it exists), please let me know.
>>>>
>>>> Thanks for reading this far :)
>>>
>