Review request for 6612680 (Remove classloader dependency on jkernel) (original) (raw)
Alan Bateman Alan.Bateman at Sun.COM
Mon Oct 5 14:24:41 UTC 2009
- Previous message: Review request for 6612680 (Remove classloader dependency on jkernel)
- Next message: Review request for 6612680 (Remove classloader dependency on jkernel)
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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.
- Previous message: Review request for 6612680 (Remove classloader dependency on jkernel)
- Next message: Review request for 6612680 (Remove classloader dependency on jkernel)
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]