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

[Mandy Chung]

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 = ....

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.

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.

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.

line 398: what happens if loaders.size() > maxindex?  Shouldn't
   it return null?

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.

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?

   line 423-426: formatting nit: these lines look a little long
   and would be good to rewrap them.

jvm.h
    1394: typo s/Retruns/Returns
    As suggested above, pass the classloader instance instead of
    defining a new loader_type interface

URLClassPath.c
    line 42: nit: align the parameters

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.

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.

Mandy