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

[David Holmes]

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