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

[Ioi Lam]

Hi Mandy,

Thanks for the review. I have updated the patch:

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

Please see comments in-line.

On 10/21/14, 12:56 PM, Mandy Chung wrote:
> Hi Ioi,
>
> On 10/21/14 10:27 AM, Ioi Lam wrote:
>> Please review this fix:
>> http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v1/
> This looks better than the preliminary webrev you sent me offline
> earlier.  I only review the jdk repo change.
>
>
>
> Launcher.java
> line 290: it can be made a final field.
> line 297: this.ucp = ....
Done
> line 317-319: I think they are meant to be two sentences.
> I have the following suggested text:
>
>    // The class of the given name is not found in the parent
>    // class loader as well as its local URLClassPath.
>    // Check if this class has already been defined dynamically;
>    // if so, return the loaded class; otherwise, skip the parent
>    // delegation and findClass.
I have used your comment and removed the old one.
> URLClassPath.java
> line 76: this long line should be broken into multiple line.
>    Maybe you can addsun.security.action.GetPropertyAction in the
>    import and update line 72, 74, 76, 78.
Done
> line 319-326: Is this LoaderType enum necessary?  I think it can
>    simply pass the ClassLoader instance to VM rather than having
>    this enum type just for this purpose.  I suggest to get rid
>    of the LoaderType enum type.

I changed the API to pass the ClassLoader instance and removed the 
LoaderType.
> 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[].
> 404   if (getNextLoader(null, maxindex) == null ||
> 405       !lookupCacheEnabled) {
>
> line 405 should indent to the right for better readability.
>
> The comment helps as I was initially confused why lookupCacheEnabled
> is not checked first.  Am I right to say line 398-415 is equivalent
> to (excluding the debugging statements):
>     if (loaders.size() <= maxindex &&getLoader(maxindex) != null) {
>        returnlookupCacheEnabled ? cache : null;
>     }
>     return null;
> I think that is easier to understand.
I changed the code to this. What do you think?

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) {
                     failed = true;
                 } else if (!lookupCacheEnabled) {
                     // As a side effect of the getNextLoader call,
                     // The cache has been invalidated.
                     failed = true;
                 }
                 if (failed) {
                     if (DEBUG_LOOKUP_CACHE) {
                         System.out.println("Expanded loaders FAILED " +
                                loaders.size() + " for maxindex=" + 
maxindex);
                     }
                     return null;
                 }
                 if (DEBUG_LOOKUP_CACHE) {
                     System.out.println("Expanded loaders " + 
loaders.size() +
                                " to maxindex=" + maxindex);
                 }
             }
>
> 432   urlNoFragString.equals(
> 433          URLUtil.urlNoFragString(lookupCacheURLs[index]))) {
>
>    Should you store URLUtil.urlNoFragString form of URL in
>    lookupCacheURLs such that they can be compared with equals?
urlNoFragString is called exactly once for every URL in the 
lookupCacheURLs so they live very shortly. In contrast, the URLs stored 
in the lookupCacheURLs live forever (HotSpot has an internal reference 
to them and use them for other purposes). So for memory footprint, it's 
better to not cache the urlNoFragString.
>    line 423-426: formatting nit: these lines look a little long
>    and would be good to rewrap them.
Fixed
> jvm.h
>     1394: typo s/Retruns/Returns
>     As suggested above, pass the classloader instance instead of
>     defining a new loader_type interface
Fixed. Now the class loader instance is passed.
> URLClassPath.c
>     line 42: nit: align the parameters
Fixed
> There are several lines calling
>    49JNU_ThrowNullPointerException(env, 0).
>    55        JNU_ThrowOutOfMemoryError(env, NULL);
>
> The msg argument probably better to be consistent and pass NULL.
> ClassLoader.c is a bit inconsistent.
Fixed.
> Can you add regression tests in the jdk repo?  Sanity tests
> like with the lookup cache enabled but invalidated at startup
> or due to addURL call.
Added.
> Mandy
>