RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 2)

[Ioi Lam]

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