<i18n dev> RFR: 8161203: ResourceBundle.getBundle performance regression

[Peter Levart]

Hi Masayoshi,

Thinking of how the ResourceBundle caching is implemented, I think it 
has a serious flaw. The cache is currently the following:

private static final ConcurrentMap<CacheKey, BundleReference> cacheList

...where the CacheKey is an object which contains WeakReference(s) to 
the caller's ClassLoader and Module. This is OK.

BundleReference, OTOH, is a SoftReference<ResourceBundle>. The problem 
is ResourceBundle(s) can be arbitrary user subclasses, which means that 
they may have an implicit reference to the ClassLoader that appears in 
the CacheKey. The consequence is that such cache can prevent GC-ing of 
the ClassLoader for arbitrary amount of time (SoftReferences are cleared 
on memory pressure).

Luckily there might be a solution. If the ResourceBundle subclasses are 
always resolvable from the ClassLoader that appears in the CacheKey, 
then there is a utility class that I created recently: 
java.lang.reflect.ClassLoaderValue. This a package-private class as it 
is currently used only in java.lang.reflect.Proxy for caching of dynamic 
Proxy classes, but could be made public and moved to some 
jdk.internal... package.

Basing ResourceBundle caching on this utility class could also simplify 
the implementation as you wouldn't have to deal with WeakReference(s) to 
ClassLoader and Module objects. You could still have a BundleReference 
extending SoftReference<ResourceBundle> to release the bundles on memory 
pressure but since such SoftReference(s) would be anchored in the 
particular ClassLoader instance that can resolve the ResourceBundle 
subclasses, it would not prevent such ClassLoader from being GCed 
immediately.

I can prototype such caching if you like.

Regards, Peter

On 07/22/2016 06:07 AM, Masayoshi Okutsu wrote:
> Hi Peter,
>
> Thank you for your suggestion. Actually CacheKey is used for storing 
> information required to load resource bundles during a 
> ResourceBundle.getBundle call. The current CacheKey usage may be too 
> tricky and cause some maintenance problems. I will file a separate 
> issue on that problem. I wanted to work on some clean up of the 
> CacheKey usage, but I haven't had a chance.
>
> Thanks,
> Masayoshi
>
>
> On 7/21/2016 10: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
>>>
>>
>