Note that my pending change http://cr.openjdk.java.net/~martin/webrevs/openjdk8/getChars/getChars.patch does the same kind of thing, but without recursive lock acquisitions.
I'm curious why a recursive lock acquisition would be considered "very" cheap. Is there some hotspot magic, or is it simply that we have another write to a cache line that is already probably owned by the cpu by virtue of the previous cas to acquire? On Sun, May 19, 2013 at 10:48 PM, David Holmes <david.hol...@oracle.com>wrote: > The change put through for JDK-8013395 (StringBuffer toString cache) has > exposed a previously unnoticed bug in the StringBuffer.append(**CharSequence > cs) method. That method claimed to achieve synchronization (and then > correct toStringCache behaviour) by the super.append method calling other > StringBuffer methods after narrowing of cs to a specific type. But that is > incorrect if cs==null as in that case the AbstractStringBuilder.**appendNull > method is called directly, with no calls to an overridden StringBuffer > method. (I have verified that none of the other methods claiming to not > need sync suffer from a similar flaw - this is an isolated case.) > > Consequently we started failing some existing append(null) tests. > > The fix is quite simple: append(CharSequence) behaves as for other append > methods and is declared synchronized and clears the cache explicitly. The > existing test is extended to check append(null). > > webrev: > > http://cr.openjdk.java.net/~**dholmes/8014814/webrev/<http://cr.openjdk.java.net/~dholmes/8014814/webrev/> > > This fix does mean that recursive synchronization will now be used for > append(CharSequence) but recursive synchronization is very cheap. An > alternative fix suggested by Alan Bateman in the bug report is to override > appendNull and add the synchronization there. That requires a change to the > accessibility of AbstractStringBuilder.**appendNull so I chose the more > constrained fix. Alan's fix will also introduce nested synchronization, but > only for the append(null) case. As I said I don't think performance will be > a concern here. > > Testing (in progress): JPRT -testset core, SQE test that caught this > > Thanks, > David >
