<i18n dev> RFR: 8161203: ResourceBundle.getBundle performance regression
Peter Levart
peter.levart at gmail.com
Thu Aug 4 16:47:22 UTC 2016
Hi Masayoshi,
Just a thought. What if RB.clearCache(classLoader) was specified as
equivalent to RB.clearCache(classLoader.getUnnamedModule()) ?
I think that would be backwards compatible...
Regards, Peter
On 08/02/2016 05:30 PM, Masayoshi Okutsu wrote:
> 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 i18n-dev
mailing list