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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.