<i18n dev> RFR: 8161203: ResourceBundle.getBundle performance regression
masayoshi.okutsu at oracle.com
Mon Jul 25 06:08:55 UTC 2016
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.
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:
> 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
> * 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.
>>> 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:
>>>>> 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.
More information about the i18n-dev