RFR: 8252842: Extend jmap to support parallel heap dump [v32]

[Ralf Schmelter]

On Mon, 6 Sep 2021 08:03:22 GMT, Lin Zang <lzang at openjdk.org> 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