RFR: 8161203: ResourceBundle.getBundle performance regression

[Peter Levart]

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
>>
>