Review request for 6612680 (Remove classloader dependency on jkernel)
Alan.Bateman at Sun.COM
Mon Oct 5 14:24:41 UTC 2009
Mandy Chung wrote:
> Alan, Karen,
> Can you review the fix for:
> 6612680: Remove classloader dependency on jkernel
> Webrev at:
> 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)
- 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.
More information about the core-libs-dev