All suggestions have been implemented. Please find new webrevs here:
webrev root: http://cr.openjdk.java.net/~ykantser/8059047/webrev.02
webrev jdk: http://cr.openjdk.java.net/~ykantser/8059047.jdk/webrev.02
webrev hotspot:
http://cr.openjdk.java.net/~ykantser/8059047.hotspot/webrev.01
// Katja
On 04/24/2015 12:10 PM, Staffan Larsen wrote:
On 24 apr 2015, at 11:34, Yekaterina Kantserova
<[email protected]
<mailto:[email protected]>> wrote:
Hi,
Here comes the updated version.
bug: https://bugs.openjdk.java.net/browse/JDK-8059047
webrev root: http://cr.openjdk.java.net/~ykantser/8059047/webrev.01/
<http://cr.openjdk.java.net/%7Eykantser/8059047/webrev.01/>
webrev jdk:
http://cr.openjdk.java.net/~ykantser/8059047.jdk/webrev.01/
<http://cr.openjdk.java.net/%7Eykantser/8059047.jdk/webrev.01/>
test/com/sun/management/HotSpotDiagnosticMXBean/DumpHeap.java:
In main() I think you should change the deleteOnExit() to just
delete(). There is no reason to wait with it.
Perhaps we should also verify that the file does not already exists
before we try to write to it. If it exists, we can delete it.
webrev hotspot:
http://cr.openjdk.java.net/~ykantser/8059047.hotspot/webrev.00/
<http://cr.openjdk.java.net/%7Eykantser/8059047.hotspot/webrev.00/>
test/serviceability/dcmd/gc/HeapDumpTest.java:
Same thing: call delete() instead of deleteOnExit().
One comment about changes in hotspot part. The refactored version of
serviceability/dcmd/gc/HeapDumpTest.java doesn't contain check:
70 /*
71 * Some hprof dumps of all objects contain
constantPoolOop references that cannot be resolved, so we ignore
72 * failures about resolving constantPoolOop fields
using a negative lookahead
73 */
74 output.shouldNotMatch(".*WARNING(?!.*Failed to
resolve object.*constantPoolOop.*).*");
It depends on that the current version of jdk.test.lib.hprof parser
simply write down warnings to stdout. As a result the test needs to
invent own logic to parse it.
I suggest instead to improve jdk.test.lib.hprof parser as a separate
RFE. The parser will collect such information and provide a new
method for getting it, e.g.
jdk.test.lib.hprof.model.Snapshot.getWarnings(). The
serviceability/dcmd/gc/HeapDumpTest.java will be changed accordingly
when RFE is implemented.
Sounds right. A quick, hacky solution is to redirect System.out to a
stream or file while running the parser…
/Staffan
Thanks,
Katja
On 04/22/2015 03:09 PM, Staffan Larsen wrote:
On 22 apr 2015, at 11:17, Yekaterina
Kantserova<[email protected]
<mailto:[email protected]>> wrote:
>>>>>
>>>>>Hi,
>>>>>
>>>>>Could I please have a review of this fix.
>>>>>
>>>>>bug:https://bugs.openjdk.java.net/browse/JDK-8059047
<http://bugs.openjdk.java.net/browse/JDK-8059047>
>>>>>webrev:http://cr.openjdk.java.net/~ykantser/8059047/webrev.00/
<http://cr.openjdk.java.net/%7Eykantser/8059047/webrev.00/>
>>>>>
>>>>>This fix is a part of JEP 241: Remove the jhat Tool
(https://bugs.openjdk.java.net/browse/JDK-8059039). I suggest to put
parser/validator into common test library since the functionality
can be useful not only for SVC tools tests but even for some future
GC tests.
>>>>>
>>>>>The old jhat packages have been moved as follows:
>>>>>com.sun.tools.hat.internal.model -> jdk.test.lib.hprof.model
>>>>>com.sun.tools.hat.internal.parser -> jdk.test.lib.hprof.parser
>>>>>com.sun.tools.hat.internal.util -> jdk.test.lib.hprof.util
>>>>>
>>>>>The source has not been changed except Copyrights year.
>>>>>
>>>>>Thanks,
>>>>>Katja
