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

[Lin Zang]

On Wed, 17 Mar 2021 06:40:33 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Hi Lin,
>> 
>> One concern I have is the naming used in the fix.
>> First, the term write-through you use is unusual and confusing.
>> Would it better to say 'direct or unbuffered output' instead?
>> 
>>  612         // Now the total size of data to dump is known and can be filled to segment header.
>>  613         // Enable write-through mode to avoid internal buffer copies.
>>  614         if (useSegmentedHeapDump) {
>>  615             long longBytes = length * typeSize + headerSize;
>>  616             int bytesToWrite = (int) (longBytes);
>>  617             hprofBufferedOut.fillSegmentSizeAndEnableWriteThrough(bytesToWrite);
>>  618         }
>> 
>> Why 'longBytes' ? The scope of this local variable is very short, and it is clear that its type is long.
>> Why not something like 'size' or 'totalSize' ?
>> There is no need in parentheses around the 'longBytes' at L616.
>> Also, there is no need to for one more local variable 'bytesToWrite'.
>> Something like below is more clear:
>>   int size = (int)(length * typeSize + headerSize);
>> I agree with Chris, it is confusing why the decision about write-trough mode is made in this method.
>> The method fillSegmentSizeAndEnableWriteThrough() is called for any array if useSegmentedHeapDump == true,
>> so any segmented output for arrays is direct (in write-through mode).
>> Is this new dimension really needs to be introduced?
>> 
>> Thanks,
>> Serguei
>
> One more question.
> 1367         public synchronized void write(int b) throws IOException {
> 1368            if (segmentMode && !writeThrough) {
> 1369                if (segmentWritten == 0) {
> 1370                    // At the begining of the segment.
> 1371                    writeSegmentHeader();
> 1372                } else if (segmentWritten == segmentBuffer.length) {
> 1373                    // Internal buffer is full, extend a larger one.
> 1374                    int newSize = segmentBuffer.length + SEGMENT_BUFFER_INC_SIZE;
> 1375                    byte newBuf[] = new byte[newSize];
> 1376                    System.arraycopy(segmentBuffer, 0, newBuf, 0, segmentWritten);
> 1377                    segmentBuffer = newBuf;
> 1378                }
> 1379                segmentBuffer[segmentWritten++] = (byte)b;
> 1380                return;
> 1381            }
> 1382            super.write(b);
> 1383         }
> Does the check for "!writeThrough" at L1368 means there is no need to write the segment header (as at L1371)?

Dear Serguei, 
Thanks for review! I will refine it based on your comments.

> I agree with Chris, it is confusing why the decision about write-trough mode is made in this method 

The "write-through" mode requires filling the segment size in segment header before object contents are scanned and written, And it is only in method `calculateArrayMaxLength()` the array size could be known and filled, and therefore the `write-through` mode could be enabled.
(please be aware that current implementation of dumping non-array object does not calculate the object size first, which is 
the reason why internal buffer is required when compressed heap dump was introduced.)

> The method fillSegmentSizeAndEnableWriteThrough() is called for any array if useSegmentedHeapDump == true,
> so any segmented output for arrays is direct (in write-through mode).
> Is this new dimension really needs to be introduced?

IMO, it is better to dump all array object in `write-through` mode, because it could avoid memory consumption and performance issue when dumping large array. And the original implementation of object dump works like `write-through`, so this fix looks like sort of retrieve back to the original behavior.

On the other side, the specific issue described at JDK-8262386 could also be fixed if enable write-through mode only when array size is larger than maxBytes calculated in `calculateArrayMaxLength()`, because the test created an 4GB integer array.  But I suggest to `write-through` all array objects as this could help avoid similar issue that may happen when dump large array whose size is less than 4GB. 

> Does the check for "!writeThrough" at L1368 means there is no need to write the segment header (as at L1371)?
Yes, The `writeSegmentHeader(size)` is invoked in `fillSegmentSizeAndEnableWriteThrough()`, so there is no need to call it here.

Thanks!
Lin

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

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