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

[Mandy Chung]

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