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
