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

[Chris Plummer]

On Fri, 2 Apr 2021 09:22:26 GMT, Lin Zang <lzang at openjdk.org> wrote:

>>> > 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)
>> 
>> Can't you just call `Oop.getObjectSize()`?
>> 
>>> > 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.
>> 
>> Ok. So `HPROF_HEAP_DUMP` is just a record, and records have a 32-bit size limit. I assume that previously only one such record was allowed. So `HPROF_HEAP_DUMP_SEGMENT` was created, and the only difference between it and `HPROF_HEAP_DUMP` is that you can have more than one `HPROF_HEAP_DUMP_SEGMENT`. Am I understanding it correctly?
>
> Dear @plummercj,
> I am planing to rewrite the whole implementation with Oop.getObjectSize(), by which most of the object dump code could be write to underlying outputstream directly.  And then there is no need to use segmental dump for gziped heap dump. And hence this issue could be fixed at all.
> Do you think it is OK to include the re-write change in this PR?
> 
> BRs,
> Lin

If your rewrite has the side affect of fixing this issue, what I would suggest is a new CR and PR that just address the rewrite, especially since this PR has a lot of history that I think you are saying will pretty much become obsolete, and therefore is just noise to anyone wanting to review your changes. In the new PR you can note that the changes will also address 8262386, and you can include an `/issue 8262386` comment so it will be closed out with new PR.

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

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