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

Wed Apr 21 02:01:17 UTC 2021

On Wed, 14 Apr 2021 12:35:27 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> 8252842: Extend jmap to support parallel heap dump
>
> Lin Zang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits:
> 
>  - Merge branch 'master' into par-dump
>  - Merge branch 'master' into par-dump
>  - Typo fix
>  - remove parallel option and dumpheapext command
>  - Revert "hide the dumpheapext error message"
>    
>    This reverts commit 1af0e1e2bfab4f5d079c03ff0cb25067acacdac4.
>  - resize large object threshold
>  - Merge branch 'master' into par-dump
>  - Merge branch 'master' into par-dump
>  - code clean
>  - fix trailing white space issue
>  - ... and 15 more: https://git.openjdk.java.net/jdk/compare/e2106d5a...997784b1

Ok made a closer pass this time. Overall looks good except for some suggested edits, but I couldn't review all of it in detail. What type of testing are you doing?

src/hotspot/share/services/attachListener.cpp line 252:

> 250:     // parallel thread number for heap dump, initialize based on active processor count.
> 251:     // Note the real number of threads used is also determined by active workers and compression
> 252:     // backend thread number, see heapDumper.cpp.

The commas should really be periods to start new sentences.

src/hotspot/share/services/heapDumper.cpp line 579:

> 577:     // this is already the correct length, since we don't add more sub-records.
> 578:     write_u4(len);
> 579:     assert (Bytes::get_Java_u4((address)(buffer() + 5)) == len, "Inconsitent size!");

Remove space.

src/hotspot/share/services/heapDumper.cpp line 701:

> 699:   size_t _internal_buffer_used;
> 700:   char* _buffer_base;
> 701:   bool _splited_data;

`_split_data`. Split is one of those english verbs that doesn't follow the rules. "I split the log yesterday" is correct, not "splited".

src/hotspot/share/services/heapDumper.cpp line 721:

> 719: 
> 720:  public:
> 721:   ParDumpWriter(DumpWriter* dw) :

The DumpWriter constructor has the following comment, which you should add here also:
`// Check for error after constructing the object and destroy it in case of an error.`

src/hotspot/share/services/heapDumper.cpp line 737:

> 735:      if (_buffer_base != NULL) {
> 736:        os::free(_buffer_base);
> 737:         _buffer_base = NULL;

remove extra indentation space

src/hotspot/share/services/heapDumper.cpp line 853:

> 851: 
> 852:   void flush_to_backend(bool force) {
> 853:     // Guarantee there is only one writer update backend buffers.

"updating the backend buffers"?

src/hotspot/share/services/heapDumper.cpp line 862:

> 860:       entry = NULL;
> 861:     }
> 862:     assert(_pos == 0, "available buffer must be clean before flush");

"empty" instead of "clean"?

src/hotspot/share/services/heapDumper.cpp line 1665:

> 1663: // To avoid memory consumption, when dumping large objects such as huge array and
> 1664: // large objects whose size are larger than LARGE_OBJECT_DUMP_THRESHOLD, the scanned
> 1665: // partial object/array data will be send to the backend directly instead of caching

"sent"

src/hotspot/share/services/heapDumper.cpp line 1668:

> 1666: // the whole object/array in the internal buffer.
> 1667: // The HeapDumpLargeObjectList is used to save the large object when dumper scans
> 1668: // the heap. The large objects could be added (push) parallelly by multiple dumpers.

"dumpers,"

src/hotspot/share/services/heapDumper.cpp line 1669:

> 1667: // The HeapDumpLargeObjectList is used to save the large object when dumper scans
> 1668: // the heap. The large objects could be added (push) parallelly by multiple dumpers.
> 1669: // But they will be removed (pop) serially only by the VM thread.

"popped"

src/hotspot/share/services/heapDumper.cpp line 1690:

> 1688:     HeapDumpLargeObjectListElem* entry = new HeapDumpLargeObjectListElem(obj);
> 1689:     if (entry == NULL) {
> 1690:       warning("Fail to allocate element for large object list");

"failed"

src/hotspot/share/services/heapDumper.cpp line 1774:

> 1772:   }
> 1773: 
> 1774:   // If large object list exist and it is large object/array,

"exists"

src/hotspot/share/services/heapDumper.cpp line 1775:

> 1773: 
> 1774:   // If large object list exist and it is large object/array,
> 1775:   // add oop into the list and skip scan, VM thread will process it later.

"...scan. VM thread..."

src/hotspot/share/services/heapDumper.cpp line 1842:

> 1840:        ml.wait();
> 1841:      }
> 1842:      assert(_started == true,  "dumper is waken up with wrong state");

"dumper work up"

src/hotspot/share/services/heapDumper.cpp line 1918:

> 1916:     // Calculate dumper and writer threads number.
> 1917:     _num_writer_threads = num_total - _num_dumper_threads;
> 1918:     // If dumper threads number is zero, there is only VMThread work as a dumper.

"If dumper threads number is zero, only the VMThread works as a dumper."

src/hotspot/share/services/heapDumper.cpp line 1919:

> 1917:     _num_writer_threads = num_total - _num_dumper_threads;
> 1918:     // If dumper threads number is zero, there is only VMThread work as a dumper.
> 1919:     // If dumper threads number is equal to active workers, need at lest one thread work as writer.

"...at least one worker thread..."?

src/hotspot/share/services/heapDumperCompression.cpp line 240:

> 238: }
> 239: 
> 240: void CompressionBackend::flush_buffer() {

This method looks to be indentical to `CompressionBackend::deactivate()` except `CompressionBackend::deactivate()` does a couple more tasks at the end. Perhaps `CompressionBackend::deactivate()` should be calling this now.

src/hotspot/share/services/heapDumperCompression.cpp line 404:

> 402:   assert(buffer != NULL && used != 0 && max != 0, "Invalid data send to compression backend");
> 403:   assert(_active == true, "Backend must be active when flushing external buffer");
> 404:   // force flush buffered data and get new buffer.

This is an odd location for this comment. I think i should go before the method and also match the (modified) comment found in the .hpp file.

src/hotspot/share/services/heapDumperCompression.cpp line 410:

> 408: 
> 409:   MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
> 410:   // first try current work. if it is clean

Do you mean "First try current work buffer. Use it if empty."?

src/hotspot/share/services/heapDumperCompression.cpp line 417:

> 415:     get_new_buffer(&buf, &tmp_used, &tmp_max, true);
> 416:   }
> 417:   assert (_current != NULL && _current->_in != NULL && _current->_in_max >= max &&

`_current` can't possibly be NULL here since you referenced `_current->_in_used` a few lines above.

src/hotspot/share/services/heapDumperCompression.hpp line 221:

> 219:   char const* error() const { return _err; }
> 220: 
> 221:   // sets up a internal buffer, fill with external buffer and send to compressor

// Sets up a internal buffer, fills with external buffer, and sends to compressor.

src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java line 232:

> 230:                 if (compress_level.length() == 0) {
> 231:                      System.err.println("Fail: no number provided in option: '" + subopt + "'");
> 232:                      usage(1);

I think lines 231 and 232 have an extra indentation of 1 space.

Since this change is unrelated to the CR being fixed, perhaps it would be best to do these changes with the new CR you are going to create for the help output changes below.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2261