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