Thanks Mandy for reviewing.
Webrev has been updated to
(1) added the sync back to charset/deleteCharset(). As you suggested, there
is race condition risk that the map is being accessed while the system
initialization completes.
(2) added the "holder" pattern for msiso2022_jp and iso2022_jp_2, as suggested.
(3) keep the AbstractCharsetProvider in repo for now.
I have not eliminate the static reference to s.n.c.ExtendedCharsets class, yet,
since it is only invoked/referred after we have checked Class.forName. Is it
really a problem here? for example, the hotspo re-arrange the order of
execution?
http://cr.openjdk.java.net/~sherman/8012326/webrev/
Thanks!
-Sherman
http://cr.openjdk.java.net/~sherman/8012326/webrev/
On 05/10/2013 04:55 PM, Mandy Chung wrote:
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/
-Sherman
