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

[Serguei Spitsyn]

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

>> As discussed in CR https://bugs.openjdk.java.net/browse/JDK-8262386, the byte[] list is much more like an optimization. Revert it in the PR and I will create a separate CR and PR for it. 
>> 
>> Thanks,
>> Lin
>
> 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)?

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

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