RFR (XL) 8046070 - Class Data Sharing clean up and refactoring, round #2
Ioi Lam
ioi.lam at oracle.com
Thu Aug 7 01:25:51 UTC 2014
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
More information about the core-libs-dev
mailing list