Request for review: Race conditions in java.nio.charset.Charset (original) (raw)

Ulf Zibis Ulf.Zibis at gmx.de
Wed Oct 7 19:55:03 UTC 2009

Sherman, thanks for your review ...

Am 07.10.2009 19:56, Xueming Shen schrieb:

Though I did not write the cache code my reading suggests the existing cache impl tries to avoid the relatively "expensive" synchronization for most use scenarios, your approach however hits the synchronization for each/every lookup invocation. So you should at least consider to put the sync block in a separate lookup2.

Agreed: (But why separate lookup2? I suspect that Charset.forName() would occur in excessive loops, so hotspot potentially cold inline it.)

private static Charset lookup(String charsetName) {
    if (charsetName == null)
        throw new IllegalArgumentException("Null charset name");

    CacheItem ci = cache[0];
    if (ci != null && charsetName.equals(ci.name))
        return ci.charset;

    synchronized (Charset.class) {
        int pos;
        for (pos=1; ; pos++) {
        ...
    }
}

Is synchronization more expensive, than instantiating new Object[] each time + 2 castings (to String + to Charset)?

But personally I doubt we really care this "race condition", it's a cache, a cache miss is not a disaster,

OK, but why didn't you you reject Bug Id 6881442 with the same argumentation. ... and missing the class's name in Class.name could only happen once, in contrast to "my" race condition, which theoretically could happen every time.

in fact in most cases, since the charset has been in the cache already, it should be in the providers' "cache" as well, especially for the bundled standard & extended charset provider, the lookup actually is 1 or 2 O(1) hashmap lookup

... it's 2 hashmap lookup, see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6850361

(yes, an additional name canonicalization and the "expensive" sync).

So maybe we can do something like b = cache1; cache1 = a; cache2 = b;

Hm, if content of cache1 was updated to content, which is hold in a by other thread, saving cache1in b doesn't help. (Maybe I didn't understand your trick.)

to make the situation a little better, or maybe worse:-)

And what do you think about breaking the endless loop, while getting the default charset, and what about avoiding to cache wrong default charset?

-Ulf

More information about the core-libs-dev mailing list