RFR: 8161203: ResourceBundle.getBundle performance regression

[Peter Levart]

Hi Claes,


On 07/21/2016 03:21 PM, Claes Redestad wrote:
>
>
> On 2016-07-21 15:13, Peter Levart wrote:
>> Hi Masayoshi,
>>
>> Previously the CacheKey::clone() method cleared a reference to 
>> 'providers' in the clone making the provides unreachable from the 
>> clone and making the clone unable to obtain providers. Now you also 
>> reset the 'providersChecked' flag which makes the clone be able to 
>> re-obtain the providers. This is dangerous as the clone is used as a 
>> key in the cache and is strongly reachable from the cache. A slight 
>> future modification of code could unintentionally produce a class 
>> loader leak. To prevent that, I would somehow mark the clone so that 
>> any attempt to invoke getProviders() on the clone would throw 
>> IllegalStateException.
>
> Seems to me that simply setting providersChecked = true; in clone() 
> should keep the old behavior?
>
> Also, while not trying to make ResourceBundle thread-safe, perhaps 
> getProviders() would be more idiomatically implemented as:
>
>     ServiceLoader<ResourceBundleProvider> getProviders() {
>         ServiceLoader<ResourceBundleProvider> providers = this.providers;
>         if (!providersChecked) {
> this.providers = providers = getServiceLoader(getModule(), name);
> providersChecked = true;
> }
>         return providers;
>     }
>
> ... to guard against the method ever returning null on weakly-ordered 
> CPUs.
>
> Thanks!
>
> /Claes

I thought of that too, but then I checked where these methods are called 
from and it seems that the CacheKey instance that is used to obtain 
providers is freshly allocated at each call to static getBundleImpl() 
and just passed down to other methods. So only a single thread is ever 
accessing a particular CackeKey instance.

A clone of CackeKey is then inserted into the shared cache, but without 
a reference to providers.

Regards, Peter

>
>>
>> Regards, Peter
>>
>> On 07/21/2016 06:14 AM, Masayoshi Okutsu wrote:
>>> Hi,
>>>
>>> Please review the fix for JDK-8161203. The fix is to lazily load 
>>> ResourceBundleProviders. It's not necessary to load providers before 
>>> cache look-up.
>>>
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8161203
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~okutsu/9/8161203/webrev.01
>>>
>>> Thanks,
>>> Masayoshi
>>>
>>
>