[9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Mandy Chung]

+1

I’m happy with the new reachabilityFence calls.

Mandy

> On Jan 17, 2017, at 10:23 AM, Naoto Sato <naoto.sato at oracle.com> wrote:
> 
> Thanks, Peter & Daniel.
> 
> I modified the webrevs accordingly:
> 
> http://cr.openjdk.java.net/~naoto/8171139/webrev.06/
> http://cr.openjdk.java.net/~naoto/8171140/webrev.01/
> 
> The one for 8171139 is the same as Peter's latest one, and the latter one for 8171140 was modified following that change.
> 
> Naoto
> 
> On 1/17/17 4:24 AM, Daniel Fuchs wrote:
>> +1
>> 
>> With Peter's proposed changes if I'm not mistaken then all methods
>> that operate on the CacheKey (findBundle, findBundleInCache,
>> putBundleInCache, loadBundle, loadBundleFromProvider) are all
>> called from a point originating from within the block now protected
>> by the reachability fences, so there should be no opportunity for the
>> referents of our KeyElementReference to become null while the
>> CacheKey is used.
>> 
>> Thanks Peter!
>> 
>> -- daniel
>> 
>> On 17/01/17 11:53, Peter Levart wrote:
>>> Hi Naoto,
>>> 
>>> Sorry for joining late for review. Thanks for taking this on. I think it
>>> is shaping well...
>>> 
>>> On 01/14/2017 01:54 AM, Naoto Sato wrote:
>>>> 
>>>> 
>>>> http://cr.openjdk.java.net/~naoto/8171139/webrev.05/
>>>> 
>>> 
>>> Tho things...
>>> 
>>> 1. As said, the "cloning" of CacheKey has no purpose in the following
>>> section of code (lines 1685...1709):
>>> 
>>>            CacheKey constKey = new CacheKey(cacheKey);
>>>            trace("findBundle: %d %s %s formats: %s%n", index,
>>> candidateLocales, cacheKey, formats);
>>>            try {
>>>                if (module.isNamed()) {
>>>                    bundle = loadBundle(cacheKey, formats, control,
>>> module, callerModule);
>>>                } else {
>>>                    bundle = loadBundle(cacheKey, formats, control,
>>> expiredBundle);
>>>                }
>>>                if (bundle != null) {
>>>                    if (bundle.parent == null) {
>>>                        bundle.setParent(parent);
>>>                    }
>>>                    bundle.locale = targetLocale;
>>>                    bundle = putBundleInCache(cacheKey, bundle, control);
>>>                    return bundle;
>>>                }
>>> 
>>>                // Put NONEXISTENT_BUNDLE in the cache as a mark that
>>> there's no bundle
>>>                // instance for the locale.
>>>                putBundleInCache(cacheKey, NONEXISTENT_BUNDLE, control);
>>>            } finally {
>>>                if (constKey.getCause() instanceof
>>> InterruptedException) {
>>>                    Thread.currentThread().interrupt();
>>>                }
>>>            }
>>> 
>>> The 'constKey' is copied of the 'cacheKey' and is not used anywhere
>>> except in finally block where it is asked for .getCause() which will
>>> always be null since copying of CacheKey never copies the cause...
>>> 
>>> 2. We can make sure that CacheKey copy constructor always copies a
>>> CacheKey that has non-cleared moduleRef and callerRef. The original
>>> cacheKey is created in getBundleImpl() from nun-null module and
>>> callerModule.  getBundleImpl() then calls down to findModule() with this
>>> cacheKey. If we keep a reference to module and callerModule in
>>> getBundleImpl() alive until the end of this method, we guarantee that
>>> moduleRef and callerRef will not be cleared while using such cacheKey to
>>> construct copies of it.
>>> 
>>> Here's those two changes applied to your webrev.05 and also a race in
>>> clearCacheImpl() fixed (as reported by Daniel Fuchs):
>>> 
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/8170772_ResourceBundle.caching/webrev.06/
>>> 
>>> 
>>> 
>>> What do you think?
>>> 
>>> Regards, Peter
>>> 
>>