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

[Ioi Lam]

Hi David, I have update the latest webrev at:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/

and fixed the "int cache[]" style you mentioned.

This version also contains the JDK test case that Mandy requested:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html

Thanks
- Ioi


On 10/26/14, 4:27 PM, David Holmes wrote:
> 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
>>