Re: Potential StringBuffer to StringBuilder clean-up (where warranted)

[Tobias Frech]

Hi, 
concerning the safety of replacing StringBuffers with StringBuilders: not only should these StringBuffers not escape the method they are created in. But also they should not be assigned to an instance or static field of the class in this method or any other method which is called from within that method. Theoretically you risk concurrent access from another Thread then. I would consider it a quite unusual design if a StringBuffer is used that way, but then again why is it synchronized in the first place?

Cheers,
Tobias 

----- Reply message -----
Von: "Martijn Verburg" <martijnverburg at gmail.com>
An: "Mike Duigou" <mike.duigou at oracle.com>
Cc: "jdk8-dev" <jdk8-dev at openjdk.java.net>
Betreff: Potential StringBuffer to StringBuilder clean-up (where warranted)
Datum: Fr., Mai. 4, 2012 18:07


Hi Mike,

Thanks for the detailled reply, we'll get going on the pre-reviews!

Cheers,
Martijn

On 4 May 2012 17:05, Mike Duigou <mike.duigou at oracle.com> wrote:
> Hi Martijn;
>
>
> Stefan as really dived into this project! Thank you for helping him make progress and assisting him with getting his changes ready to commit.
>
> As with the warnings changes, because the changes span multiple projects, it will be necessary to break up the webrevs a bit and submit them to the various jdk sub-mailing lists (awt, net, security, corelibs, langtools, etc.)
>
> StringBuffer to StringBuilder conversions are an excellent contribution.
>
> On May 4 2012, at 07:17 , Martijn Verburg wrote:
>
>> Hi all,
>>
>> Stefan Reich has submitted a patch to the Adopt OpenJDK program which
>> we're looking to pre-review that converts (in theory, all easily and
>> automatically convertible) uses of StringBuffer into StringBuilder.
>> The motivation is to bring some performance benefits where
>> synchronization is not required.
>
> In almost all cases the StringBuffer objects would have been uncontended and hotspot will elid the locking. In theory there won't be significant performance differences but it's still worth doing.
>
>> It covers all files in
>> src/share/classes, but could be extended to cover tools and platform
>> behaviour as well.
>>
>> * The refactoring deals with instances of StringBuffers that are
>> created in a method, and which don't escape out of their scope.
>
> This should make the conversions entirely safe.
>
>> * There are a few instances where fields are converted, but they are
>> in classes that are not serializable, for example in LogStream.
>> * A lot of the changes are in the toString() methods, but there are
>> more frequently used methods in RandomAccessFile (for example,
>> readLine()) and Double that use StringBuffer.
>
> The uses I looked at in Double and RandomAccessFile don't seem to have any special reason to be using StringBuffer. I can't think of any non-escaping uses of StringBuffer where conversion to StringBuilder wouldn't be appropriate.
>
>> * We're going to check that there really is no synchronization that is
>> going on (to the best of our ability)
>
> If the StringBuffer instances don't escape their creating method then this shouldn't be a concern.
>
>> Quick Q:  Is moving from StringBuffer to StringBuilder generally
>> desirable?
>
> Yes.
>
>> I'm concerned it might be too dangerous given that whether
>> or not it's being used in a synchronized context may not be easily
>> detectable.
>>
>> If it is generally desirable, then any extra advice on what we should
>> look out for in pre-reviewing this patch is most welcome.
>
> I think that for the most part you have it covered by ensuring that the StringBuffer instances are all ones that don't escape the method in which they are created.
>
> Mike
>