RFR: 8161203: ResourceBundle.getBundle performance regression

[Peter Levart]

Hi Claes and Masayoshi,

On 07/21/2016 06:54 PM, Claes Redestad wrote:
> Doesn't this fail to address part of the regression, i.e., we 
> shouldn't go into various privileged actions at all if the module is 
> unnamed?
>
> /Claes

Right, it addresses the part that lazily looks up providers via service 
loader instead of in CacheKey constructor, but it introduces a 
regression where previously the privileged action was not executed if 
the lookup for providers failed (couldn't load the provider type or 
ServiceLoader threw ServiceConfigurationError) as opposed to when the 
lookup was successful but didn't find any providers in which case the 
privileged action was executed and returned null, but it also set 
cacheKey.callerHasProvider = Boolean.FALSE if it was null. A slight 
semantical difference. I wonder if it was intended.

Anyway, here's how the old behavior can be restored:

http://cr.openjdk.java.net/~plevart/jdk9-dev/ResourceBundle.cleanup/webrev.02/

Regards, Peter


>
> On 2016-07-21 18:16, Peter Levart wrote:
>> Hi Masayoshi,
>>
>> The whole story about InterruptedException (in my previous message) 
>> is moot. I have checked all the places where the 'cause' is reported 
>> into the CacheKey and there's nowhere a possibility that an 
>> InterruptedException would get set. So the cloning of cacheKey (in 
>> line 1766) and checking for InterruptedException in the clone can all 
>> be just removed from the code.
>>
>> Code around providers can be simplified too. Without the need for a 
>> boolean flag, you just have to change the signature of 'providers' 
>> field from ServiceLoader<ResourceBundleProvider> to 
>> Iterable<ResourceBundleProvider> and assign a Collections.emptyList() 
>> to it in clone().
>>
>> getServiceLoader() method(s) can similarly just return 
>> Collections.emptyList() instead of null after changing their 
>> signature(s). CacheKey does not need a hasProviders() method and any 
>> checking for null can get away.
>>
>> I also found a race in putBundleInCache() method. It tries hard to 
>> return the bundle that gets installed into cache 1st, but it doesn't 
>> try hard enough. If there is an expired entry already in the cache, 
>> it just overwrites it with new bundle, but doesn't take into account 
>> the possibility that two or more threads can do the same thing - just 
>> overwrite each other's entry and each return its own instance to the 
>> caller.
>>
>> The effort that the code takes in this method to prevent the newly 
>> allocated BundleReference from being enqueued into the ReferenceQueue 
>> is also moot. The reference will only get discovered and enqueued if 
>> it remains reachable. If if doesn't get installed into the cache it 
>> will be GCed and not enqueued.
>>
>> All of the above can be fixed in the following way:
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/ResourceBundle.cleanup/webrev.01/ 
>>
>>
>>
>> Take whatever you want from it. The rest can be included in some new 
>> cleanup task if you like it.
>>
>> Regards, Peter
>>
>>
>> On 07/21/2016 03:13 PM, 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.
>>>
>>> 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
>>>>
>>>
>>
>