On Wed, 17 Mar 2021 06:40:33 GMT, Serguei Spitsyn <sspit...@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
