RFR: 8014814 (str) StringBuffer "null" is not appended

[David Holmes]

On 20/05/2013 5:44 PM, David Holmes wrote:
> 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.

So it looks like I can do my push with nested sync with no concern 
because once you do your getChars update the nested sync will be 
removed. I like that outcome.

That said I'm concerned by your getChars method but I shall take that up 
on another thread.

Thanks,
David

>> 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
>>
>>