[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8171084: heapdump/JMapHeapCore fails with java.lang.RuntimeException: Heap segment size
From:       Jini George <jini.george () oracle ! com>
Date:       2017-01-27 5:59:30
Message-ID: 588AE191.5080702 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]