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.
