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

Mandy Chung mandy.chung at oracle.com
Tue Oct 21 19:56:27 UTC 2014

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.

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.

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.

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

    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.


More information about the core-libs-dev mailing list