RFR: 8013395 StringBuffer.toString performance regression impacting embedded benchmarks

[David Holmes]

On 13/05/2013 6:39 PM, Peter Levart wrote:
> 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...

Ah! Yes now I see - I said it was a silly mistake.

> I think that toString array caching aspect and copy-on-write aspect can
> only be combined in situations when 'count == value.length'. Otherwise

Right so for this simple StringBuffer case we are basically stuck with 
the memory waste.

> 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).

This is going too far afield. The change becomes too significant for 
this stage of JDK8 release.

Thanks,
David

> 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 :)
>>>>
>>
>