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.
