Review request for 6612680 (Remove classloader dependency on jkernel)

[Mandy Chung]

Alan, Rémi,

Thanks for the review.   The revised webrev is at:
   http://cr.openjdk.java.net/~mchung/6612680/jdk-webrev.01/
   http://cr.openjdk.java.net/~mchung/6612680/hotspot-webrev.01/

Alan Bateman wrote:
> 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).
>

Yes, we need that so that jkernel VM can run with other JDKs with no 
DownloadManager.  Thanks for catching it.

> - 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.

Yes.  I set the hook after the system classes have been initialized.  I 
added the check in the setHook() method to catch if a new hook would be 
added in the future before the system is initialized.
>
> - Do these changes mean we can remove sun.misc.VM.isBootedKernelVM?
>

I clean that up.  We no longer need this method that was put as a 
workaround.

> - 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.

This is to fix the jdk and deploy build failure when we eliminate the 
dependency to jkernel.   If the sun.jkernel.* classes were built in the 
tmp directory, javah fail to find the sun.jkernel.* classes to generate 
the .h files.  Usually we only set CLASSDESTDIR to TEMPDIR when we want 
to package classes in a different jar file.  This is not the case for 
sun.jkernel.* since they are included in rt.jar and that's a bug.   I 
guess why it worked in the past is because sun.jkernel.* get compiled 
when compiling the system classes that reference DownloadManager and the 
sun.jkernel.* classes were put in the usual class destination directory.

>
> - 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.
>

I clean up the code per your suggestion and ready to push the fix.

Mandy
> -Alan.
>
>
>
>
>
>
>
>
>
>
>
>