Hi Masayosh, Alan,
Thanks for looking at the change.
I suppose application containers are already accustomed to invoke
ResourceBundle.clearCache(ClassLoader) when undeploying an
application so that the corresponding ClassLoader can get GCed right
away. But there's a change in the semantics of this method that
Jigsaw introduced...
Before Jigsaw, this method was specified to:
* Removes all resource bundles from the cache that have been loaded
* using the given class loader.
After Jigsaw, this method is specified to:
* Removes all resource bundles from the cache that have been loaded
* by the caller's module using the given class loader.
Now suppose that the "caller's module" that loads the bundle is the
application's module and the module that clears the cache is the
container's module. The app's cache won't get cleared. I understand
that the change in semantics was an attempt to "isolate" modules
among themselves so that one module could not clear the cache of
another module if they happen to map to the same class loader, but
this also means that an application container can't easily clear the
cache of the application it wishes to undeploy unless it uses the new
ResourceBundle.clearCache(Module) method on each application's module
before undeploying it.
I also wonder if the change of the semantics of this method is
backwards compatible. Suppose that some container is using this
method to clear the cache of the application loaded by say
"classloaderA" before undeploying it. It calls
ResourceBundle.clearCache(classloaderA). Now suppose the container
code is loaded by some other class loader (say "classloaderC") which
is usually the case in today's containers. Both container and
application classes are part of the unnamed modules of their
corresponding class loaders. With the change in the semantics of this
method introduced by jigsaw, the container will not clear the cache
of the application any more since the container module is not the
same as the application module (they are distinct unnamed modules).
Anyway, I noticed an inconsistency in my webrev.04 immediately after
posting it a week ago. I was off for a week then, so now that I'm
back, here's the corrected webrev of my proposed change:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ResourceBundle.cleanup/webrev.05/
The only real change of logic compared to webrev.04 is that I moved
the "format" field from the LoadSession (previous CacheKey) to
ResourceBundle itself. This field is needed to hold the bundle's
format for invoking the Control.needsReload() method. It is not
semantically correct to use the value from the LoadSession as done in
webrev.04. This has to be the value that the bundle was created with,
so it belongs to the bundle itself. In original code it was taken
from the cloned CacheKey that was attached to the bundle.
All other changes from webrev.04 are limited to ResourceBundle.java
file and include some comments and nits that don't change the overall
logic. All ResourceBundle tests still pass.
The diff from webrev.04 is the following:
*** ResourceBundle.webrev.04.java 2016-08-01 15:33:49.408565452
+0200
--- ResourceBundle.java 2016-08-01 14:43:59.875401697 +0200
***************
*** 40,55 ****
package java.util;
- import jdk.internal.misc.JavaUtilResourceBundleAccess;
- import jdk.internal.misc.SharedSecrets;
- import jdk.internal.reflect.CallerSensitive;
- import jdk.internal.reflect.Reflection;
- import jdk.internal.util.concurrent.AbstractClassLoaderValue;
- import jdk.internal.util.concurrent.ClassLoaderValue;
- import sun.util.locale.BaseLocale;
- import sun.util.locale.LocaleObjectCache;
- import sun.util.locale.provider.ResourceBundleProviderSupport;
-
import java.io.IOException;
import java.io.InputStream;
import java.lang.ref.ReferenceQueue;
--- 40,45 ----
***************
*** 69,74 ****
--- 59,73 ----
import java.util.spi.ResourceBundleControlProvider;
import java.util.spi.ResourceBundleProvider;
+ import jdk.internal.misc.JavaUtilResourceBundleAccess;
+ import jdk.internal.misc.SharedSecrets;
+ import jdk.internal.reflect.CallerSensitive;
+ import jdk.internal.reflect.Reflection;
+ import jdk.internal.util.concurrent.AbstractClassLoaderValue;
+ import jdk.internal.util.concurrent.ClassLoaderValue;
+ import sun.util.locale.BaseLocale;
+ import sun.util.locale.LocaleObjectCache;
+ import sun.util.locale.provider.ResourceBundleProviderSupport;
import static
sun.security.util.SecurityConstants.GET_CLASSLOADER_PERMISSION;
***************
*** 346,354 ****
*/
public abstract class ResourceBundle {
- /** initial size of the bundle cache */
- private static final int INITIAL_CACHE_SIZE = 32;
-
static {
SharedSecrets.setJavaUtilResourceBundleAccess(
new JavaUtilResourceBundleAccess() {
--- 345,350 ----
***************
*** 386,392 ****
/**
! * The cache of BundleReference(s) per class loader, bundle
base name and locale.
*/
private static final ClassLoaderValue<BundleReference> cache
= new ClassLoaderValue<>();
--- 382,389 ----
/**
! * The cache of BundleReference(s) per class loader, module,
bundle base name
! * and locale.
*/
private static final ClassLoaderValue<BundleReference> cache
= new ClassLoaderValue<>();
***************
*** 419,430 ****
* The parent bundle is searched by {@link #getObject getObject}
* when this bundle does not contain a particular resource.
*/
! protected ResourceBundle parent = null;
/**
* The locale for this bundle.
*/
! private Locale locale = null;
/**
* The base bundle name for this bundle.
--- 416,427 ----
* The parent bundle is searched by {@link #getObject getObject}
* when this bundle does not contain a particular resource.
*/
! protected ResourceBundle parent;
/**
* The locale for this bundle.
*/
! private Locale locale;
/**
* The base bundle name for this bundle.
***************
*** 432,437 ****
--- 429,440 ----
private String name;
/**
+ * Bundle format which is necessary for calling
+ * Control.needsReload().
+ */
+ private String format;
+
+ /**
* The flag indicating this bundle has expired in the cache.
*/
private volatile boolean expired;
***************
*** 622,629 ****
/**
* A session object used during the {@link #getBundle} call.
! * The loader may be null, but the base name, the locale and
! * module must have a non-null value.
*/
private static class LoadSession {
// These four are the actual keys for lookup in cache.
--- 625,633 ----
/**
* A session object used during the {@link #getBundle} call.
! * The loader may be null (in which case it is considered to be
the
! * bootstrap class loader), but the module, base name, and
locale must have
! * a non-null value.
*/
private static class LoadSession {
// These four are the actual keys for lookup in cache.
***************
*** 642,651 ****
return key().get(loader);
}
- // bundle format which is necessary for calling
- // Control.needsReload().
- private String format;
-
// Placeholder for an error report by a Throwable
private Throwable cause;
--- 646,651 ----
***************
*** 699,712 ****
return callerHasProvider == Boolean.TRUE;
}
- String getFormat() {
- return format;
- }
-
- void setFormat(String format) {
- this.format = format;
- }
-
private void setCause(Throwable cause) {
if (this.cause == null) {
this.cause = cause;
--- 699,704 ----
***************
*** 734,740 ****
}
}
return "LookupSession[" + name + ", lc=" + l + ", ldr="
+ getLoader()
! + "(format=" + format + ")]";
}
}
--- 726,732 ----
}
}
return "LookupSession[" + name + ", lc=" + l + ", ldr="
+ getLoader()
! + "]";
}
}
***************
*** 1666,1672 ****
bundle = loadBundleFromProviders(baseName, targetLocale,
providers, loadSession);
if (bundle != null) {
! loadSession.setFormat(UNKNOWN_FORMAT);
}
}
// If none of providers returned a bundle and the caller
has no provider,
--- 1658,1664 ----
bundle = loadBundleFromProviders(baseName, targetLocale,
providers, loadSession);
if (bundle != null) {
! bundle.format = UNKNOWN_FORMAT;
}
}
// If none of providers returned a bundle and the caller
has no provider,
***************
*** 1690,1696 ****
}
if (bundle != null) {
! loadSession.setFormat(format);
break;
}
} catch (Exception e) {
--- 1682,1688 ----
}
if (bundle != null) {
! bundle.format = format;
break;
}
} catch (Exception e) {
***************
*** 1764,1770 ****
Iterable<ResourceBundleProvider> providers,
LoadSession loadSession)
{
! // assert cacheKey != null && providers != null;
return AccessController.doPrivileged(
new PrivilegedAction<>() {
public ResourceBundle run() {
--- 1756,1762 ----
Iterable<ResourceBundleProvider> providers,
LoadSession loadSession)
{
! // assert loadSession != null && providers != null;
return AccessController.doPrivileged(
new PrivilegedAction<>() {
public ResourceBundle run() {
***************
*** 1819,1825 ****
if (bundle != null) {
// Set the format in the cache key so that it can be
// used when calling needsReload later.
! loadSession.setFormat(format);
bundle.name = loadSession.getName();
bundle.locale = targetLocale;
// Bundle provider might reuse instances. So we
should make
--- 1811,1817 ----
if (bundle != null) {
// Set the format in the cache key so that it can be
// used when calling needsReload later.
! bundle.format = format;
bundle.name = loadSession.getName();
bundle.locale = targetLocale;
// Bundle provider might reuse instances. So we
should make
***************
*** 1877,1883 ****
* Finds a bundle in the cache. Any expired bundles are marked as
* `expired' and removed from the cache upon return.
*
! * @param loadSession the key to look up the cache
* @param control the Control to be used for the expiration
control
* @return the cached bundle, or null if the bundle is not
found in the
* cache or its parent has expired. <code>bundle.expire</code>
is true
--- 1869,1875 ----
* Finds a bundle in the cache. Any expired bundles are marked as
* `expired' and removed from the cache upon return.
*
! * @param loadSession the load session used look up the cache
* @param control the Control to be used for the expiration
control
* @return the cached bundle, or null if the bundle is not
found in the
* cache or its parent has expired. <code>bundle.expire</code>
is true
***************
*** 1946,1954 ****
if (!bundle.expired && expirationTime >= 0 &&
expirationTime <=
System.currentTimeMillis()) {
try {
! bundle.expired =
control.needsReload(loadSession.getName(),
! loadSession.getLocale(),
! loadSession.getFormat(),
loadSession.getLoader(),
bundle,
bundle.loadTime);
--- 1938,1946 ----
if (!bundle.expired && expirationTime >= 0 &&
expirationTime <=
System.currentTimeMillis()) {
try {
! bundle.expired =
control.needsReload(bundle.getBaseBundleName(),
! bundle.getLocale(),
! bundle.format,
loadSession.getLoader(),
bundle,
bundle.loadTime);
In my opinion, the change of the semantics of
ResourceBundle.clearCache(ClassLoader) could be tolerated if the
invocation of that method was not a requirement for application
containers to be able to unload the app's class loader. With my
proposed change to caching it is not a requirement any more.
Regards, Peter
On 07/25/2016 08:08 AM, Masayoshi Okutsu wrote:
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
