RFR: 8161203: ResourceBundle.getBundle performance regression

[Peter Levart]

Hi,

Unrelated to this issue, I think I found a bug which may cause thread 
interrupt to be swallowed. There's a 2nd place where CacheKey is cloned, 
but for a reason I cannot explain:

1760         if (bundle != NONEXISTENT_BUNDLE) {
1761             CacheKey constKey = (CacheKey) cacheKey.clone();
1762
1763             try {
1764                 if (module.isNamed()) {
1765                     bundle = loadBundle(cacheKey, formats, control, 
module);
1766                 } else {
1767                     bundle = loadBundle(cacheKey, formats, control, 
expiredBundle);
1768                 }
1769                 if (bundle != null) {
1770                     if (bundle.parent == null) {
1771                         bundle.setParent(parent);
1772                     }
1773                     bundle.locale = targetLocale;
1774                     bundle = putBundleInCache(cacheKey, bundle, 
control);
1775                     return bundle;
1776                 }
1777
1778                 // Put NONEXISTENT_BUNDLE in the cache as a mark 
that there's no bundle
1779                 // instance for the locale.
1780                 putBundleInCache(cacheKey, NONEXISTENT_BUNDLE, 
control);
1781             } finally {
1782                 if (constKey.getCause() instanceof 
InterruptedException) {
1783                     Thread.currentThread().interrupt();
1784                 }
1785             }

... the clone ('constKey') is only used in finally clause to check 
whether its 'cause' is an InterruptedException and set the thread 
interrupt status if it is. The trouble is, CacheKey.clone() clears the 
'cause' on the clone (line 795):

  782         @Override
  783         public Object clone() {
  784             try {
  785                 CacheKey clone = (CacheKey) super.clone();
  786                 if (loaderRef != null) {
  787                     clone.loaderRef = new 
KeyElementReference<>(getLoader(),
  788 referenceQueue, clone);
  789                 }
  790                 clone.moduleRef = new 
KeyElementReference<>(getModule(),
  791 referenceQueue, clone);
  792                 // Clear the reference to ResourceBundleProviders
  793                 clone.providers = null;
  794                 // Clear the reference to a Throwable
  795                 clone.cause = null;
  796                 // Clear callerHasProvider
  797                 clone.callerHasProvider = null;
  798                 return clone;
  799             } catch (CloneNotSupportedException e) {
  800                 //this should never happen
  801                 throw new InternalError(e);
  802             }
  803         }

...so the if statement in finally clause never executes its body. Now 
what was the programmers intent? Did (s)he want to check the cacheKey's 
cause as it was before executing loadBundle statements with the 
cacheKey? If so, then it would be better to just remember the cause and 
not clone the whole cacheKey...

Regards, Peter


On 07/21/2016 04:08 PM, Peter Levart wrote:
> Hi Claes,
>
>
> On 07/21/2016 03:21 PM, Claes Redestad wrote:
>>
>>
>> On 2016-07-21 15:13, 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.
>>
>> Seems to me that simply setting providersChecked = true; in clone() 
>> should keep the old behavior?
>>
>> Also, while not trying to make ResourceBundle thread-safe, perhaps 
>> getProviders() would be more idiomatically implemented as:
>>
>>     ServiceLoader<ResourceBundleProvider> getProviders() {
>>         ServiceLoader<ResourceBundleProvider> providers = 
>> this.providers;
>>         if (!providersChecked) {
>> this.providers = providers = getServiceLoader(getModule(), name);
>> providersChecked = true;
>> }
>>         return providers;
>>     }
>>
>> ... to guard against the method ever returning null on weakly-ordered 
>> CPUs.
>>
>> Thanks!
>>
>> /Claes
>
> I thought of that too, but then I checked where these methods are 
> called from and it seems that the CacheKey instance that is used to 
> obtain providers is freshly allocated at each call to static 
> getBundleImpl() and just passed down to other methods. So only a 
> single thread is ever accessing a particular CackeKey instance.
>
> A clone of CackeKey is then inserted into the shared cache, but 
> without a reference to providers.
>
> Regards, Peter
>
>>
>>>
>>> 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
>>>>
>>>
>>
>