RFR: 8014814 (str) StringBuffer "null" is not appended
David Holmes
david.holmes at oracle.com
Mon May 20 07:44:49 UTC 2013
On 20/05/2013 4:25 PM, Martin Buchholz wrote:
> 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 will take a look.
> 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?
Yes "hotspot magic". Acquiring a lock you already own doesn't require a
CAS; and if it is locked via biased-locking then it is an even shorter path.
Thanks,
David
>
> On Sun, May 19, 2013 at 10:48 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at 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
>
>
More information about the core-libs-dev
mailing list