Review request for 6612680 (Remove classloader dependency on jkernel)

[Alan Bateman]

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.