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

[Daniel Fuchs]

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