Sherman,
The charsetForName, charsets and aliasesFor methods call init()
first and
then access the maps with no synchronization. While the maps are
being
accessed by one thread and system initialization completes,
another thread
calls charsetForName() that calls init() which will update the
maps. So
the maps is being updated while being accessed by another thread. So
I think the original synchronized(this) is still needed for the
charsetForName, charsets and aliasesFor methods for correctness.
The maps are only updated once and also only if ExtendedCharsets
provider
is instantiated before booted and "sun.nio.cs.map" system property
is set.
I think it's worth further clean up this 2-phase instance
initialization
since ExtendedCharsets is now a singleton. Would
Charset.availableCharsets()
and ExtendedCharsets.charsets() not be called during system
initialization?
If we can restrict that they can't be called, maybe you can move
the 2nd
phase initialization to ExtendedCharsetsHolder (i.e. calling
init() method)
and perhapsExtendedCharsets.aliasesFor so that ExtendedCharsets
instance
methods no longer need to call init. Just one thought.
You propose to remove AbstractCharsetProvider class. I don't have
the
history to comment on whether you should keep/remove the
AbstractCharsetProvider for other charset providers to extend. Just
looking at the current implementation, it's not needed.
More comments inlined below.
On 5/10/2013 10:53 AM, Xueming Shen wrote:
Thanks for the review.
I have updated the Charset.java to use the init-on-depand holder
idiom.
L440: return sun.nio.cs.ext.ExtendedCharsets.getInstance();
You would need to use 'epc' and invoke "getInstance" method via
reflection
to eliminate the static reference to s.n.c.ExtendedCharsets class as
it might not be present.
I suggest to add these 2 methods in the ExtendedProviderHolder class:
static Charset charsetForName(String charsetName) {
return (extendedProvider != null)
?
extendedProvider.charsetForName(charsetName) : null;
}
static Iterator<Charset> charsets() {
return (extendedProvider != null)
? extendedProvider.charsets()
: Collections.<Charset>emptyIterator();
}
The lookup2() and availableCharsets() methods can be simplified to
something like this:
if ((cs = standardProvider.charsetForName(charsetName))
!= null ||
- (cs = lookupExtendedCharset(charsetName)) != null ||
+ (cs =
ExtendedProviderHolder.charsetForName(charsetName)) != null ||
(cs = lookupViaProviders(charsetName)) != null)
+ put(ExtendedProviderHolder.charsets(), m);
for (Iterator<CharsetProvider> i =
providers(); i.hasNext();) {
CharsetProvider cp = i.next();
put(cp.charsets(), m);
I don't think MSISO2022JP/ISO2022_JP_2 are really worth the cost
of adding
two more classes, so I leave them asis.
Are you concerned of the static footprint of the new class?
I think the code is much cleaner but you may think it's overkill:
static class Holder {
static DoubleByte.Decoder DEC0208 = ....
static DoubleByte.Encoder ENC0208 = ....
}
Or can you just cache new JIS_X_0208_MS932()or new JIS_X_0212()?
Agreed that the ExtendedCahrsets change can be separated, but
since we're here
already, It would be convenient to just do them together and the
"cleaner' code
then can be back-port together later when requested.
It's fine with me. I suggest to look into the possibility
separating the second phase update to the extended charsets if you
want to remove the synchronization on the instance methods.
Mandy
http://cr.openjdk.java.net/~sherman/8012326/webrev/
<http://cr.openjdk.java.net/%7Esherman/8012326/webrev/>
-Sherman
