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.
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
More information about the core-libs-dev
mailing list