RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, round #2

[Karen Kinnear]

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