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.
