RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 2)
Ioi Lam
ioi.lam at oracle.com
Fri Oct 24 23:38:04 UTC 2014
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
More information about the core-libs-dev
mailing list