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
>