<i18n dev> RFR: 8161203: ResourceBundle.getBundle performance regression
Peter Levart
peter.levart at gmail.com
Mon Aug 1 14:10:26 UTC 2016
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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the i18n-dev
mailing list