Review request for 6612680 (Remove classloader dependency on jkernel)
Mandy.Chung at Sun.COM
Mon Oct 5 23:39:20 UTC 2009
Thanks for the review. The revised webrev is at:
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
> - 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)
> - 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.
More information about the core-libs-dev