RFR: 8014814 (str) StringBuffer "null" is not appended
David Holmes
david.holmes at oracle.com
Tue May 21 02:12:50 UTC 2013
So I propose to push ahead with this fix in my offered form.
I still need an official Review to do so, or else objections against the
proposal.
Thanks,
David
On 20/05/2013 10:17 PM, David Holmes wrote:
> 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
>>>
>>>
More information about the core-libs-dev
mailing list