+1 I’m happy with the new reachabilityFence calls.
Mandy > On Jan 17, 2017, at 10:23 AM, Naoto Sato <naoto.s...@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 >>> >>
