RFR [10]: 8186517: sun.nio.cs.StandardCharsets$Aliases and ClassMap can be lazily loaded

Xueming Shen xueming.shen at oracle.com
Thu Aug 24 15:56:22 UTC 2017


On 8/24/17, 8:34 AM, Peter Levart wrote:
> Hi,
>
> On 08/24/2017 12:24 PM, Peter Levart wrote:
>> Hi Claes,
>>
>> On 08/24/2017 12:13 AM, Claes Redestad wrote:
>>>
>>>>>
>>>>> This allows us to lazily load the $Cache class too, further reducing
>>>>> work done during System.initPhase1:
>>>>>
>>>>> http://cr.openjdk.java.net/~redestad/8186517/jdk.02/
>>>>>
>>>>
>>> /Claes
>>
>> According to 2nd webrev, StandardCharsets.charsetForName("UTF-8") 
>> returns a different instance than 
>> StandardCharsets.charsetForName("utf-8"). Perhaps the argument of 
>> lookup() should 1st be passed through 
>> canonicalize(toLower(charsetName)) and then used in the 3 
>> special-case ifs..
>>
>> Regards, Peter
>>
>
> ... but then canonicalization would pull-in the Aliases.
>
> While looking at the code of StandardCharsets, I noticed a real danger 
> of using a thread-unsafe object while it is being modified. The method 
> charsets() returns an Iterator<Charset> which lazily invokes an 
> unsynchronized lookup() method for each charset as it returns them. 
> lookup() uses a Cache (a PreHashedMap) which is not a thread-safe 
> object. private lookup() is called from a synchronized block of public 
> charsetForName() method, but it is invoked unsynchronized from iterator.

There were tasty initialization/synchronization/deadlock issues here in 
the past (decade ago,
happened only on customers' site with specific use scenarios). Our 
system initialization
"sequence" has been changed and the way the standard charset provider is 
being loaded has
been changed since, it might be reasonable to reconsider how this part 
of code should work,
but I would still be very cautious here as the initial goal is for 
startup performance.

-sherman

>
> One way to fix it would be to enclose the lookup() invocation in 
> Iterator.next with a synchronized block, but then I thought that maybe 
> a PreHashedMap is not the right tool for the job. What if the cache 
> was simply a ConcurrentHashMap which need not be pre-populated with 
> all possible keys (saving on footprint) and could therefore be 
> constructed eagerly. The whole lookup() could then be unsynchronized. 
> Here's what I came up with:
>
> http://cr.openjdk.java.net/~plevart/jdk10-dev/8186517_StandardCharsets_opt/webrev.01/ 
>
>
> I also ensured that the same instance of Charset object gets returned 
> for all aliases of a charset (including standard 3).
>
> So, what do you think?
>
> Regards, Peter
>



More information about the nio-dev mailing list