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

[Ioi Lam]

On 8/6/14, 8:25 PM, David Holmes wrote:
> 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'll leave the memset there for now. It happens only once for a small 
region and doesn't seem to hurt.

>>> ---
>>> 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".
>
I have changed it to call strncpy.
>> +     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.
>

I added comments like this:

       EXCEPTION_MARK; // The following call should never throw, but 
would exit VM on error.
       Array<u8>* arr = MetadataFactory::new_array<u8>(loader_data, 
(bytes + 7)/8, THREAD);

Thanks
- Ioi

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