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?
Most methods referencinglookupCacheURLs and lookupCacheLoader
are synchronized except initLookupCache and knowToNotExist.
Should they? Or volatile?
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;
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.
Mandy
