Mandy Chung wrote:
Alan, Karen,
Can you review the fix for:
6612680: Remove classloader dependency on jkernel
Webrev at:
http://cr.openjdk.java.net/~mchung/6612680/
java.lang.ClassLoader and sun.misc.Launcher have explicit dependencies
on the jkernel code. While the performance impact of this is minimal
(the calls basically amount to nops when the JRE is complete), it's
still undesirable.
To eliminate the static dependency on jkernel, this adds a boot
classloader hook interface that is invoked in the ClassLoader
findClass, getSystemResource, and other methods. The hook is null by
default. The jkernel VM will call the static
DownloadManager.setBootClassLoaderHook() method and only the jkernel
environment will have a non-null boot class loader hook. Since
jkernel is a separate build including the bundles and VM, as we
discussed, the jkernel VM is a reasonable place to inject the
DownloadManager as the boot class loader hook.
I'm hoping to make TL b74 which is code freeze by 10 pm PT Monday.
Good to see this dependency going away. A couple of comments:
- In thread.cpp, do you need to check if klass is NULL
(sun.jkernel.DownloadManager not present).
- In thread.cpp, you set the hook after the system classes have been
initialized. Do I have this right? (looks like you double check in
setHook). You might want to check with the deployment folks to make sure
that they understand the implications of this - for example it might
have change the contents of the core bundle.
- Do these changes mean we can remove sun.misc.VM.isBootedKernelVM?
- Is the removal of CLASSDESTDIR from make/sun/jkernel/Makefile related
or just clean-up (as it doesn't seem to be used)? Minor nit but you've
add a blank line at line 46.
- ZipEntry: the idea that a class loader hook overrides the zip file
time is a bit strange. I see you have a few FIXME comments in the code
to revisit but maybe for this push it might be neater to just have
BootClassLoaderHook define a isCurrentThreadPrefetching or some such
method that returns a boolean to indicate if the current thread is
fetching and if so, set it to some pre-defined value (I don't know if
there is any significance to the value of KERNEL_STATIC_MODTIME).
- In ICC_Profile would it be neater to do:
BootClassLoaderHook hook = BootClassLoaderHook.getHook();
if (hook != null)
hook.prefetchFile(...)
- Minor nit but you've added a few blank lines to DownloadManager.
- I agree with Rémi comment that DownloadManager.instance doesn't seem
to be needed.
- In BootClassLoaderHook's class description they may be a typo - I
think you meant to say "into the bootstrap class loader". Also, I assume
you meant to say "In other JDK builds ...".
Otherwise, I think this is okay. I'll do another pass on it today as I
know you want to get this over the finish line by tonight.
-Alan.
