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
