RFR: JDK-8171084: heapdump/JMapHeapCore fails with java.lang.RuntimeException: Heap segment size overflow

[David Holmes]

Updates looks good to me too Jini!

Thanks,
David

On 27/01/2017 2:49 PM, Jini George wrote:
> Thank you very much, Dmitry, for the review. I am addressing all your
> comments, except for the @ignore in the test.
>
> -jini
>
> On 1/26/2017 2:49 PM, Dmitry Samersoff wrote:
>> Jini,
>>
>> The changes looks good to me besides some nits (below) don't need to
>> re-review.
>>
>> HeapHprofBinWriter.java
>>
>>    494 - missed space before bracket
>>
>>    536, 810 - could you add something like "Unknown type " to "shouldn't
>> reach here" message.
>>
>>    751, 769 - it might be better to create a separate int headerSize()
>> function.
>>
>> heapDumper.cpp:1077
>>
>>    I understand the reason of removing do-nothing code, but it's the only
>> changes outside of SA and it clearly out of scope of this CR.
>>
>>   So it might be better to don't change hotspot heapdumper.
>>
>> TestHeapDumpForLargeArray.java:
>>
>>    I'm not sure we should add long and resource consuming test to
>> every-night test pile.
>>
>>    It might be better to add @ignore to this test with a proper comments
>> and run it manually if we see a problem with SA dumper (but it's just an
>> opinion - I'm OK to leave it as is).
>>
>> -Dmitry
>>
>>
>> On 2017-01-26 11:26, Jini George wrote:
>>> Modified webrev:
>>> <http://cr.openjdk.java.net/%7Ejgeorge/8171084/webrev.01/index.html>http://cr.openjdk.java.net/~jgeorge/8171084/webrev.01/index.html
>>>
>>>
>>> Requesting one more Reviewer also to take a look at this.
>>>
>>> Thank you,
>>> Jini.
>>>
>>> On 1/23/2017 2:10 PM, Jini George wrote:
>>>> Thanks much for the review, David. My answers inline:
>>>>
>>>>>    870     protected void writeInstance(Instance instance, int length)
>>>>> throws IOException {
>>>>>
>>>>> seems incorrect. It adds a length parameter that is unused and also
>>>>> creates an overload, rather than override of the inherited version.
>>>>> As a result this code:
>>>> Thank you for this very good catch! This was an accidental cut-n-paste
>>>> error. I have corrected this.
>>>>> Minor nit:
>>>>>
>>>>> 379     private static final long MAX_U4_VALUE = 4294967295L;
>>>>>
>>>>> would be clearer as:
>>>>>
>>>>> 379     private static final long MAX_U4_VALUE = 0xFFFFFFFFL;
>>>>>
>>>> Makes sense. Done.
>>>>> test/serviceability/sa/LingeredAppWithLargeArray.java
>>>>>
>>>>> Style nit:
>>>>>
>>>>>   27     public int hugeArray[];
>>>>>
>>>>> should be
>>>>>
>>>>>   27     public int[] hugeArray;
>>>>>
>>>>> but why public ??
>>>> No particular reason. Changed it.
>>>>> I don't know how LingeredApp works, but this looks odd:
>>>>>
>>>>> 32     public static void main(String args[]) {
>>>>> 33         LingeredAppWithLargeArray appObject = new
>>>>> LingeredAppWithLargeArray();
>>>>> 34         LingeredApp.main(args);
>>>>> 35     }
>>>>>
>>>>> as the appObject is never used. ??
>>>> I have changed the test case now to have the array allocation done in
>>>> main() and not in the constructor.
>>>>>    test/serviceability/sa/TestHeapDumpForLargeArray.java
>>>>>
>>>>> Why is the test excluded on Mac?
>>>> There used to be SA related failures on Mac a while back, but it seems
>>>> those have gotten fixed with 8169638
>>>> <https://bugs.openjdk.java.net/browse/JDK-8169638> and probably
>>>> 8160376 <https://bugs.openjdk.java.net/browse/JDK-8160376>. I am
>>>> removing the restriction and will be running the tests. Planning on
>>>> sending out a new webrev once the testing comes clean.
>>>>
>>>> Thanks,
>>>> Jini.
>>
>