Hi Karen,
Thanks for the review. Please see my comments in-line.
On 8/6/14, 12:52 PM, Karen Kinnear wrote:
Ioi,
Changes look really good. Just minor comments:
1. jdk: SecureClassLoader.java: getProtectionDomain
please add a comment that this assumes no signers and combine the first and
third lines
fixed.
2. classFileParser.cpp
Would it make sense to move the assertion from skip_over_field_name for
DumpSharedSpaces
to JavaCalls::call* ?
fixed.
3. classLoader.cpp
"VM internal error. Must not load .class file during dump time"
- I think what you are saying is that class data sharing only supports
loading .class files from .jar files - maybe
modify the error message
fixed
4. classLoader.cpp
line 258: could you fix mmaped -> mmapped
fixed
5. classLoader.cpp
lines 468, 519: I think this changes the TraceClassLoading behavior? I think
you want
if (TraceClassLoading || TraceClassPaths && Verbose) to print_meta_index (or maybe
TraceClassPaths || (TraceClassLoading && Verbose)
same with line 549- PrintSharedArchiveAndExit - really you mean same as
above?
Line 549 is intended: trace_class_path() internally checks for
TraceClassPaths.
if (!PrintSharedArchiveAndExit) {
trace_class_path("[Bootstrap loader class path=", sys_class_path);
}
I fixed the other cases you pointed out.
6. classLoader.cpp
check_shared_classpath
- what does CDS before these changes do if you have an empty path in the
archived classpath or a non-empty directory?
In JDK8,
If you have an empty bootclasspath for dumping -- it won't go very far :-)
$ java -Dsun.boot.class.path= -Xshare:dump
Error occurred during initialization of VM
java/lang/NoClassDefFoundError: java/lang/Object
If you have an non-empty directory:
$ java -Xbootclasspath/a:/tmp -Xshare:dump
Loading classes to share ... done.
....
An error has occurred while processing the shared archive file.
Boot classpath directory /tmp is not empty.
Error occurred during initialization of VM
Unable to use shared archive.
- what happens if the archive is created (DumpSharedSpaces) without an
empty path or with a non-empty directory
but used with an empty path element or a non-empty directory?
The archive will fail to load because the dump-time classpath does not
match the run-time classpath.
There's also a check to make sure that if you had an empty directory
during dump time, the directory must remain empty during run-time.
line 587 calls check_shared_classpath only if DumpSharedSpaces?
This is intentional. If you're not dumping, the boot classpath can have
non-empty dirs.
7. classLoader.cpp
line 1085: you have a ResourceMark when you created class_name, so you
don't need another one
just a note: the class_name is legally allowed to be null - if you had
succeeded in parseClassFile you would use
parsed_name for any printing of the result (your code is fine, just wanted
you to know)
fixed
8. dictionary.cpp
line 225 - I presume this is for a specific compiler? If you know what it is,
it would help to record it here
in case in future we could move this
David Holmes also noticed this. I removed the comment as it really
doesn't apply.
9. systemDictionary.cpp
comment lines 1221-1224
I removed these lines as they are out of date.
10. sharedPathsMiscInfo.hpp
alst-> also (spelling)
11. SharedPathsMiscInfo
Why do you put this in metaspace if it is deallocated after initialization?
I would have expected a CHeapObj not in metaspace?
SharedPathsMiscInfo are allocated from CHeap:
class SharedPathsMiscInfo : public CHeapObj<mtClass> {....}
hotspot/src/share/vm/classfile/sharedClassUtil.hpp:
static SharedPathsMiscInfo* allocate_shared_paths_misc_info() {
return new SharedPathsMiscInfo();
}
Or if metaspace makes sense, maybe ask Coleen how other metaspace information that is
freed gets recorded (did we need the new metaspace object type "Deallocated"?)
is this related to metadataFactory.hpp lines 82-87?
Unrelated. Those lines are describing cases where loading of a class
fails (missing super class, etc) and would have been freed in the normal
run-time, but I saw crashes during dump time. I will file a bug and fix
this FIXME after the initial push.
12. Just checking for documentation for use
If you created a CDS archive (version 1), and then try to use it with the
new code - will the archive continue to work as it
did before? Or do you have to recreated it as a version 2 archive? I
presume we need to document this change. Is it the
case that an archive is expected to match a given jdk (or hotspot) version?
So this is not a surprise to customers? Or is
this behavior new with 8u40?
The archive is specific to a JDK build version (has always been -- see
FileMapHeader::_jvm_ident[]), so this shouldn't be a surprise. I guess
the "version" field isn't strictly necessary, but I updated it from 1 to
2 because the FileMapHeader has been restructured a lot.
13. filemap.hpp
FIXME comment 129 - what is the current state of this? Is this a planned
future change?
this comment still applies. I will file a bug for a future fix
(performance only, not a functionality bug).
14. metaspaceShared.cpp:
line 861: "druing" -> "during"
not new: lines 779/780: Comment says "Rewrite and unlink classes", prints
"Rewriting and linking classes"
Maybe the comment could say: "Rewrite and link missing classes, then unlink all
classes as part of dump"
"Rewrite and link classes" would be the right comment here.
Klass::remove_unshareable_info() is called at a later stage.
Your comment says super interfaces may have been missed from the
classlist(s)
- I believe that loading any class requires preloading any supertypes -
superclasses and superinterfaces - so I would
not expect those to be missing at this time - or is that just in the
case of failed verification?
I will double check exactly what could be missing and rewrite the
comments accordingly.
15. metaspaceShared.cpp
line 682: could you possibly change the comment from "repeat" to "iterate"?
fixed
16. metaspaceShared.cpp
_check_one_shared_class: at this point you are checking supertypes for
failed verification
1) have you ever seen a class that passed verification but one of its
supertypes failed? I don't think that should be possible?
I'm not sure what this loop is for? Just for the
IgnoreUnverifiableClassesDuringDump?
If my vague memory is correct (I could be totally wrong), during
verification some constant pool entries are resolved, which leads to the
loading of unrelated classes. In a normal run (without CDS), these extra
classes are verified lazily when they are initialized. However, during
dumping, we want to verify these classes as well, so that everything in
the archive is verified. This is the reason for the iterations. Let me
double check.
I have to go to a meeting, so more later.
Thank you so much!
- Ioi
thanks,
Karen
On Jul 28, 2014, at 7:09 PM, Ioi Lam wrote:
Hi Folks,
Please review the following clean up and refactoring of the CDS code, for JDK9
http://cr.openjdk.java.net/~iklam/8046070-cds-cleanup-v2/
https://bugs.openjdk.java.net/browse/JDK-8046070
Summary of fix:
Clean up and refactor the Class Data Sharing (CDS) code, including:
+ Improve archive integrity checking
+ Support bytecode verification during archive dumping time
+ Allow the user to configure the CDS class list and archive file.
+ Allow future extension of the CDS class loading mechanism.
Tests:
JPRT
UTE (vm.runtime.testlist, vm.quick.testlist,
vm.parallel_class_loading.testlist)
Various adhoc SQE tests on Aurora
JCK
Thanks
- Ioi
