On 17/01/17 18:23, Naoto Sato 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/
+1
best regards,
-- daniel
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
