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
