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 >
