hg: jdk8/tl/jdk: 7145454: JVM wide monitor lock in Currency.getInstance(String)

[David Holmes]

On 24/03/2012 2:47 AM, Naoto Sato wrote:
> OK, I created a bug 7156459 for this and created a patch. Can you please
> review this? It is exactly as you suggested.
>
> http://cr.openjdk.java.net/~naoto/7156459/

Looks good to me. But probably better to start a new email thread.

Thanks,
David

> Naoto
>
> On 3/22/12 6:09 P, David Holmes wrote:
>> On 23/03/2012 1:33 AM, Naoto Sato wrote:
>>> Hi David,
>>>
>>> Sorry, the review was done in the i18n team and did not go to core libs
>>> alias.
>>>
>>> In your suggested fix, I think there is a very slight chance that two
>>> threads could get different Currency instances if the first thread was
>>> interrupted just after instantiating "currencyVal" instance, which has
>>> not been the case in the prior implementation.
>>
>> What do you mean by "interrupted" here? If two threads are racing to
>> install their own Currency instance one will win and putIfAbsent will
>> return null; and the other will lose and putIfAbsent returns the
>> instance installed by the winner. There is no difference between the old
>> code and new code in this regard.
>>
>>> Although returning the
>>> singleton instance is not mandated by the spec, I though it is safer to
>>> keep the same behavior and that get() cost is almost negligible.
>>
>> Cost depends on how large and well-balanced the hashmap is.
>>
>> Cheers,
>> David
>>
>>> Naoto
>>>
>>> On 3/21/12 7:27 P, David Holmes wrote:
>>>> Hi,
>>>>
>>>> I'm sorry I missed the review of this change. The following is somewhat
>>>> inefficient:
>>>>
>>>> instance = instances.putIfAbsent(currencyCode,
>>>> new Currency(currencyCode, defaultFractionDigits, numericCode));
>>>> return (instance != null ? instance : instances.get(currencyCode));
>>>>
>>>> If the putIfAbsent succeeds then the value to return is the newly
>>>> constructed Currency instance. So if we track that object then we don't
>>>> need to do the additional get():
>>>>
>>>> Currency currencyVal =
>>>> new Currency(currencyCode, defaultFractionDigits, numericCode);
>>>> instance = instances.putIfAbsent(currencyCode, currencyVal);
>>>> return (instance != null ? instance : currencyVal);
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>
>>>> On 22/03/2012 3:12 AM, naoto.sato at oracle.com wrote:
>>>>> Changeset: 4a5817f9e249
>>>>> Author: naoto
>>>>> Date: 2012-03-21 10:10 -0700
>>>>> URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4a5817f9e249
>>>>>
>>>>> 7145454: JVM wide monitor lock in Currency.getInstance(String)
>>>>> Reviewed-by: okutsu
>>>>>
>>>>> ! src/share/classes/java/util/Currency.java
>>>>>
>>>
>