Potential StringBuffer to StringBuilder clean-up (where warranted)

[Ludwig, Mark]

See, it's the "safe(ish)" that's bothersome, and that's why I think the new way is better; it's better to expect the code around the StringBuilder to do any necessary synchronization.  (In this case, by ensuring that only one thread touches the object, one can consider the object's contents to be consistent and correct in the way we associate with "thread safe.")

I like the appearance of code that manages the access to its encapsulated objects, because with an object's built-in synchronization such as is in StringBuffer, the assumption has to be that each method makes an atomic change to the object.  For building strings, there are almost always higher-level constructs that must be atomically appended to the string (or inserted somewhere in the middle!) but by expected use, the arguments are little bits and pieces, not complete "atoms."  For real multitasking, this anyway requires the outer code to synchronize so that its atoms all get into the buffer atomically.

Thanks,
Mark

> -----Original Message-----
> From: jdk8-dev-bounces at openjdk.java.net [mailto:jdk8-dev-
> bounces at openjdk.java.net] On Behalf Of Martijn Verburg
> Sent: Friday, May 04, 2012 1:10 PM
> To: Tobias Frech
> Cc: jdk8-dev
> Subject: Re: Potential StringBuffer to StringBuilder clean-up (where
> warranted)
> 
> Hi Tobias,
> 
> Thanks for the extra analysis on this, I'll add the two extra points
> to the pre-reviewers check list.  I'll admit I'm always cautious about
> moving from something that is thread-safe(ish) by design to something
> which isn't by design.
> 
> Cheers,
> Martijn
> 
> On 4 May 2012 18:56, Tobias Frech <tobias at frech.info> wrote:
> > 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
> >>