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

[Ioi Lam]

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