RFR: 8262386: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java timed out [v14]

[Lin Zang]

On Wed, 25 Aug 2021 09:27:54 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Lin Zang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into sadump-fix
>>  - Merge branch 'master' into sadump-fix
>>  - Merge branch 'master' into s-fix
>>  - add comment for the timestamp value
>>  - Merge branch 'master' into s-fix
>>  - Fix typo and add comment
>>  - Merge branch 'master' into s-fix
>>  - fix typo in comments
>>  - Merge branch 'master' into s-fix
>>  - Merge branch 'master'
>>  - ... and 8 more: https://git.openjdk.java.net/jdk/compare/b7eb86a6...a87793c3
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1367:
> 
>> 1365:         @Override
>> 1366:         public synchronized void write(int b) throws IOException {
>> 1367:            if (segmentMode && !unbufferedMode) {
> 
> It is not clear why the condition` !unbufferedMode` is additionally checked here and several places below. The `SegmentedOutputStream` constructor always sets `false` to the`unbufferedMode` field at L1344. Is it possible that `segmentMode` can be also unbuffered?

The `unbufferedMode` is set to true at L1498, and it is use when writing array data. The purpose is to write the data directly to the file instead of buffering them first, it can avoid memory copy and also avoid timeout when dumping large array (because the buffer must contains the whole segment before it is written to file, to avoid data pollution for compressed dump, and if the buffer is not enough, a memory copy is conducted)

> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1496:
> 
>> 1494:             }
>> 1495:             // buffer must be empty now.
>> 1496:             assert (segmentMode && (segmentWritten == 0) && (unbufferedMode == false)) : "Wrong Status";
> 
> This assert repeats checks in asserts at lines L1489 and L1490.
> It is not clear why this duplication is needed.

You are right, it is unnecessary, I will fix it .

> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1580:
> 
>> 1578:         private boolean allowSegmented;
>> 1579:         // Write data directly to underlying stream. Don't use internal buffer.
>> 1580:         private boolean unbufferedMode;
> 
> I would suggest replace `unbufferedMode` with `bufferedMode` field. It will simplify logic a little bit.
> Then you will need to rename the method `fillSegmentSizeAndEnableUnbufferedMode` to be `fillSegmentSizeAndDisableBufferedMode`. 
> There are several places where `!unbufferedMode` will be replaced with `bufferedMode` .

Good idea, I will made the change. Thanks.

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

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