RFR: 8161203: ResourceBundle.getBundle performance regression
Masayoshi Okutsu
masayoshi.okutsu at oracle.com
Tue Aug 2 15:30:42 UTC 2016
Hi Peter,
Thanks for bringing up the semantics issue of clearCache. You are right.
There's some semantics change in clearCache(ClassLoader). Let me look
into it.
The format field-related changes look good to me.
Thanks,
Masayoshi
On 8/1/2016 11:10 PM, Peter Levart wrote:
> 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 jigsaw-dev
mailing list