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

[Ioi Lam]

Hi David,

Thanks for the reviews. I will incorporate your suggestions. See 
additional comments below:

On 8/6/14, 3:20 AM, David Holmes wrote:
> Hi Ioi,
>
> Continuing ... just a few minor comments
>
> Thanks,
> David
> ------
>
> hotspot/src/share/vm/memory/filemap.cpp
>
> Nit: printing string literals doesn't need to use %s ie:
>
> +     tty->print("%s", "[");
> +     tty->vprint(msg, ap);
> +     tty->print_cr("%s", "]");
>
> Should just be:
>
> +     tty->print("[");
> +     tty->vprint(msg, ap);
> +     tty->print_cr("]");
>
> ---
> Why do we need memset here:
>
>  140 FileMapInfo::FileMapInfo() {
>  141   assert(_current_info == NULL, "must be singleton"); // not 
> thread safe
>  142   _current_info = this;
>  143   memset(this, 0, sizeof(FileMapInfo));
>
The FileMapInfo is a CHeapObj<mtClass>. Does the "new" operator zero the 
memory? I added the memset just for "extra safety". Maybe I should 
remove it (the original code didn't do memset)?
> ---
> I don't quite follow the name related logic here:
>
> 224         strcpy(strptr, name);
>
I am adding an assert like this. Do you think this is enough?

+     assert(strptr + strlen(name) + 1 <= strptr_max, "miscalculated 
buffer size");
       strcpy(strptr, name);
       strptr += name_bytes;
...
       EXCEPTION_MARK;
       Array<u8>* arr = MetadataFactory::new_array<u8>(loader_data, 
(bytes + 7)/8, THREAD);
       strptr = (char*)(arr->data());
+     strptr_max = strptr + bytes;

> but strcpy rather than strncpy raises a red-flag.
>
> ---
> What is the role of this:
>
>  230       EXCEPTION_MARK;
>
> Can new_array post exceptions? If so you need to deal with it else the 
> EXCEPTION_MARK will terminate the VM abruptly.
>
At dump time, new_array() will fail if SharedReadOnlySpace or 
SharedReadWriteSpace is too small. But instead of throwing an exception, 
it will print a message about SharedReadOnlySpace or 
SharedReadWriteSpace, and exit the VM.

The EXCEPTION_MARK just indicates we should never return back to this 
function with a pending exception.

> ---
>
> hotspot/src/share/vm/memory/metaspaceShared.cpp
>
>  779   // Rewrite and unlink classes.
>  780   tty->print_cr("Rewriting and linking classes ...");
>
> Is it linking or unlinking?
>
linking. I will fix comment.
> ---
>
> hotspot/src/share/vm/oops/instanceKlass.hpp
>
> +   // was loaded. For archived classes, this filed is either NULL 
> (for system
>
> Typo: filed
>
> +   // needed after afterwards.
>
> Typo: after after
>
> ----
>
>  test/testlibrary/com/oracle/java/testlibrary/BuildHelper.java
>
> File has the wrong copyright header.
>
>
> On 29/07/2014 9:09 AM, 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