RFR: 8161203: ResourceBundle.getBundle performance regression

[Peter Levart]

On 07/22/2016 10:46 AM, Peter Levart wrote:
> Hi Masayoshi,

Here's a preview of work to migrate ResourceBundle caching to using 
ClassLoaderValue utility:

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

The changes are very straightforward. They preserve the general 
structure of the logic. I renamed the CacheKey nested class to 
LoadSession as it now only functions as a mutable object passed around 
the methods while executing the getBundle() process. LoadSession is now 
a factory for ClassLoaderValue cache key. I moved the loadTime and 
expirationTime fields from LoadSession (old CacheKey) to ResourceBundle. 
As each cached entry must maintain it's own loadTime/expirationTime, I 
changed the NONEXISTENT_BUNDLE constant into a private subclass of 
ResourceBundle. The back-link from ResourceBundle to CacheKey is not 
needed any more. There is a backlink from BundleReference to 
ClassLoaderValue key and the associated ClassLoader to enable expunging.

All 41 java/util/ResourceBundle tests pass with this change applied. 
Including the ReferenceTest which I had to modify a little to remove a 
dependency on ResourceBundle.cacheList field which is only used in test 
to report the size of the Map. It is not possible to obtain the size of 
the Map any more with this change applied, since the entries are 
distributed to multiple Map(s) - one Map per ClassLoader instance. The 
following comment in ReferenceTest could now be removed:

  * This test relies on the current behavior of the garbage collector and is
  * therefore no clear indicator of whether the fix for 4405807 works.
  * If the test fails, it might indicate a regression, or it might just mean
  * that a less aggressive garbage collector is used.

...as the ClassLoader(s) unloading does not depend on eagerness of 
SoftReference(s) clearing or any other activity any more.

What do you think?

Regards, Peter

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