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
2. classFileParser.cpp
Would it make sense to move the assertion from skip_over_field_name for
DumpSharedSpaces
to JavaCalls::call* ?
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
4. classLoader.cpp
line 258: could you fix mmaped -> mmapped
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?
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?
- 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?
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)
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
9. systemDictionary.cpp
comment lines 1221-1224
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?
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?
13. filemap.hpp
FIXME comment 129 - what is the current state of this? Is this a planned
future change?
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"
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?
15. metaspaceShared.cpp
line 682: could you possibly change the comment from "repeat" to "iterate"?
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?
I have to go to a meeting, so more later.
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
