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

[Naoto Sato]

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
>>
>