RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, round #2
karen.kinnear at oracle.com
Wed Aug 6 19:52:20 UTC 2014
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
Would it make sense to move the assertion from skip_over_field_name for DumpSharedSpaces
to JavaCalls::call* ?
"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
line 258: could you fix mmaped -> mmapped
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?
- what does CDS before these changes do if you have an empty path in the archived classpath or a non-empty directory?
- 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?
line 587 calls check_shared_classpath on if DumpSharedSpaces?
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)
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
comment lines 1221-1224
alst-> also (spelling)
Why do you put this in metaspace if it is deallocated after initialization? I would have expected a CHeapObj not in metaspace?
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?
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?
FIXME comment 129 - what is the current state of this? Is this a planned future change?
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"
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?
line 682: could you possibly change the comment from "repeat" to "iterate"?
_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?
I have to go to a meeting, so more later.
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
> 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.
> UTE (vm.runtime.testlist, vm.quick.testlist, vm.parallel_class_loading.testlist)
> Various adhoc SQE tests on Aurora
> - Ioi
More information about the core-libs-dev