RFR: 8161203: ResourceBundle.getBundle performance regression

[Peter Levart]

Hi Claes and Masayoshi again,

Ah sorry, I totally missed the part that skips looking up of the 
providers for unnamed module. Here's that part included:

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

Regards, Peter


On 07/21/2016 09:49 PM, Peter Levart wrote:
> 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
>>>>>
>>>>
>>>
>>
>