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

[Jini George]

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.
>