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

[Masayoshi Okutsu]

Hi Peter,

The change (ResourceBundle part) looks very good to me. There's a simple 
workaround for the problem -- call 
ResourceBundle.clearCache(ClassLoader). So I don't know how serious the 
problem is, though. I still like this change.

Yes, the comment in ReferenceTest should be removed.

Thanks,
Masayoshi

On 7/22/2016 10:13 PM, Peter Levart wrote:
> 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
>>>>>
>>>>
>>>
>>
>