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

[Jini George]

Thank you, David.

-Jini.

On 1/27/2017 11:26 AM, David Holmes wrote:
> 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.
>>>
>>