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.hol...@oracle.com
<mailto: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
