Style nit: all the
int cache[]
should be
int[] cache
Also not clear if 8061651-lookup-index-open-v2 is latest webrev ??
Thanks,
David
On 25/10/2014 9:38 AM, Ioi Lam wrote:
Hi Mandy,
Thanks for the review. please see comments in-line
On 10/24/14, 2:33 PM, Mandy Chung wrote:
On 10/23/2014 9:34 PM, Ioi Lam wrote:
Hi Mandy,
Thanks for the review. I have updated the patch:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v2/
Thanks for removing the LoaderType enum. Two questions -
do you need the new hasLookupCacheLoader variable? Should
lookupCAcheURLs be always be non-null if it has the lookup
cache?
I removed hasLookupCacheLoader and changed the condition check to
(lookupCacheURLs != null)
Most methods referencinglookupCacheURLs and lookupCacheLoader
are synchronized except initLookupCache and knowToNotExist.
Should they? Or volatile?
I changed both to synchronized.
On 10/21/14, 12:56 PM, Mandy Chung wrote:
line 398: what happens if loaders.size() > maxindex? Shouldn't
it return null?
In this case, all the loaders that's needed by the cache[] have been
opened. So we should return cache[].
I forget about that, sorry. I'm thinking how to make it more
obvious to those who doesn't know much of the details. Every time I
read this and I need to reload what I know about and miss something.
I changed the code to this. What do you think?
It seems to me that it would help if you refactor and add a method
"ensureLoadersInited" that will make it clear what it attempts to do.
The side effect invalidating the cache can be checked after it's
called and no need to worry about lookupCacheEnabled must be checked
in any order. Something like this
if (cache != null && cache.length > 0) {
int maxindex = cache[cache.length - 1];
ensureLoaderOpened(maxindex);
if (loaders.get(maxindex) == null || !lookupCacheEnabled) {
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders FAILED " +
loaders.size() + " for maxindex=" +
maxindex);
}
return null;
}
...
}
return cache;
I changed the code to
private synchronized int[] getLookupCache(String name) {
if (lookupCacheURLs == null || !lookupCacheEnabled) {
return null;
}
int cache[] = getLookupCacheForClassLoader(lookupCacheLoader,
name);
if (cache != null && cache.length > 0) {
int maxindex = cache[cache.length - 1]; // cache[] is
strictly ascending.
if (!ensureLoaderOpened(maxindex)) {
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders FAILED " +
loaders.size() + " for
maxindex=" + maxindex);
}
return null;
}
}
return cache;
}
private boolean ensureLoaderOpened(int index) {
if (loaders.size() <= index) {
// Open all Loaders up to, and including, index
if (getLoader(index) == null) {
return false;
}
if (!lookupCacheEnabled) {
// cache was invalidated as the result of the above call.
return false;
}
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders " + loaders.size() +
" to index=" + index);
}
}
return true;
}
The code is getting further complicated with this lookup cache and
bear with me for being picky.
if (loaders.size() <= maxindex) {
boolean failed = false;
// Call getNextLoader() with a null cache to open all
// Loaders up to, and including, maxindex.
if (getNextLoader(null, maxindex) == null) {
Can this call getLoader(maxindex)? They are essentially the same.
I think getLoader(maxindex) makes it explicit that it forces to
open the loader.
Changed.
Mandy