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 <[email protected]> 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" <[email protected]> > An: "Mike Duigou" <[email protected]> > Cc: "jdk8-dev" <[email protected]> > 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 <[email protected]> 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 >>
