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

[Lin Zang]

On Fri, 19 Mar 2021 20:21:12 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> 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
>
>> 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.
> 
> I guess I don't understand why you would want write-through for small arrays but not large objects. I also have to admit I don't fully grasp the purpose of "segment mode". I see the docs say:
> 
>  301  * A heap dump can optionally be generated as a sequence of heap dump
>  302  * segments. This sequence is terminated by an end record. The additional
>  303  * tags allowed by format "JAVA PROFILE 1.0.2" are:
>  304  *
>  305  * HPROF_HEAP_DUMP_SEGMENT  denote a heap dump segment
>  306  *
>  307  *               [heap dump sub-records]*
>  308  *               The same sub-record types allowed by HPROF_HEAP_DUMP
>  309  *
>  310  * HPROF_HEAP_DUMP_END      denotes the end of a heap dump
> 
> But all this seems to be doing is grouping the HPROF_HEAP_DUMP records into an array rather than having them interspersed with other types of records. How does this help, and why would this mode not always be enabled?
> 
> Also I'm not so sure the above documentation is fully accurate. It looks like both HPROF_HEAP_DUMP_SEGMENT and HPROF_HEAP_DUMP_END are followed by a 4-byte size and 4-byte timestamp, although the timestamp seems to always be 0, and the size for END is also always 0.

Dear Chris，

> I guess I don't understand why you would want write-through for small arrays but not large objects.

I think it is because that the current implementation does not have code which could calculate the object size before scan it. but it has the logic for calculate array length (`calculateArrayMaxLength()`) ahead of scaning.  ( BTW, I am planing to add the logic for object, and then rewrite the whole heap dump impl to use writeThrough also for compressed dump, I think that should be in a separate PR)

> But all this seems to be doing is grouping the HPROF_HEAP_DUMP records into an array rather than having them interspersed with other types of records. How does this help, and why would this mode not always be enabled?

I think the original pupose of SEGMENT heap dump is to handle the case for large heap. In the hprof format spec, the size slot of the heap dump is 32bit, which means it has limitation of dump 4GB used heap.  so use segmental dump could help resolve the problem. And IMO the reason for not always enable it is because every segment has a header and tail, which may introduce extra memory, althogh it is not much.

> It looks like both HPROF_HEAP_DUMP_SEGMENT and HPROF_HEAP_DUMP_END are followed by a 4-byte size and 4-byte timestamp, although the timestamp seems to always be 0, and the size for END is also always 0.

My understand  from code is that 4-byte size and 4-byte timestamp only exist after HPROF_HEAP_DUMP_SEGMENT, and the HPROF_HEAP_DUMP_END actually is the end of the segment.  So I think the specification is not accurate.

Thanks,
Lin

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

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