On Mon, 6 Sep 2021 08:03:22 GMT, Lin Zang <[email protected]> wrote:
>> 8252842: Extend jmap to support parallel heap dump
>
> Lin Zang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix build error
The change seems OK so far, but I've found a few issues.
src/hotspot/share/services/attachListener.cpp line 255:
> 253: HeapDumper dumper(live_objects_only /* request GC */);
> 254: dumper.dump(path, out, (int)level, (uint)parallel_thread_num);
> 255: }
The call signature for dump has changed, since it now takes an overwrite bool
value after the compression level. This means that actually the number of
parallel threads is not set but remains at the default value of 1. So the
parallel case is never executed currently.
src/hotspot/share/services/heapDumper.cpp line 1681:
> 1679: };
> 1680:
> 1681: HeapDumpLargeObjectListElem* _head;
This should be volatile.
src/hotspot/share/services/heapDumper.cpp line 1682:
> 1680:
> 1681: HeapDumpLargeObjectListElem* _head;
> 1682: uint _length;
I don't think the length is needed at all. You just have to know if the list is
empty or not and that can be found out via the _head field.
src/hotspot/share/services/heapDumper.cpp line 1714:
> 1712: }
> 1713: HeapDumpLargeObjectListElem* entry = _head;
> 1714: if (_head->_next != NULL) {
Why the check? Wouldn't you want _head to be NULL when the last entry was
removed?
src/hotspot/share/services/heapDumper.cpp line 1717:
> 1715: _head = _head->_next;
> 1716: }
> 1717: entry->_next = NULL;
I don't see why you would NULL out the _next field. The entry is deleted
shortly after anyway.
src/hotspot/share/services/heapDumper.cpp line 1797:
> 1795: if (o->is_instance()) {
> 1796: InstanceKlass* ik = InstanceKlass::cast(o->klass());
> 1797: size = DumperSupport::instance_size(ik);
Getting the size of an instance can be surprisingly expensive for classes with
many static fields, since we iterate over all static fields of the class. So I
would avoid having to calculate the size twice (here and when we are actually
dump it). Maybe here it would just be enough to use o->size() * 8 as an upper
limit value of the real size in the heap dump. After all practically no
non-array object will ever we that large.
src/hotspot/share/services/heapDumper.cpp line 1822:
> 1820: Monitor* _lock;
> 1821: uint _dumper_number;
> 1822: uint _waiting_number;
The _waiting_number doesn't seem to really have a purpose. No decision depends
on it.
src/hotspot/share/services/heapDumper.cpp line 2448:
> 2446: // dump the large objects.
> 2447: void VM_HeapDumper::dump_large_objects(ObjectClosure* cl) {
> 2448: if (_large_object_list->is_empty()) {
drain() should be able to handle an empyt list, so this check should not be
needed.
src/hotspot/share/services/heapDumperCompression.cpp line 386:
> 384: size_t tmp_max = 0;
> 385:
> 386: MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
This is not thread safe if flush_external_buffer() or get_new_buffer() are
called concurrently. But it seems to be Ok since flush_external_buffer() is
only called with the _lock var of the parallel writer locked, so there is no
contention. Is this correct?
-------------
PR: https://git.openjdk.java.net/jdk/pull/2261
