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

[David Holmes]

A couple of responses inline ...

Cheers,
David

On 7/08/2014 11:25 AM, Ioi Lam wrote:
> 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 would not expect the memory to be zeroed. The fields of the object 
should be initialized as appropriate.

>> ---
>> 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?

No I think static analysis tools may flag this as a "bad usage".

> +     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.

Use of EXCEPTION_MARK is always a bit unclear to me.

Thanks,
David
-----------------

>> ---
>>
>> 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
>