[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(M): 8059047: Extract parser/validator from jhat for use in tests
From: Yekaterina Kantserova <yekaterina.kantserova () oracle ! com>
Date: 2015-04-28 12:11:38
Message-ID: 553F78FA.3090202 () oracle ! com
[Download RAW message or body]
Staffan,
Thanks for the review!
// Katja
On 04/28/2015 02:05 PM, Staffan Larsen wrote:
> Looks good!
>
> Thanks,
> /Staffan
>
> > On 24 apr 2015, at 16:17, Yekaterina Kantserova
> > <yekaterina.kantserova@oracle.com
> > <mailto:yekaterina.kantserova@oracle.com>> wrote:
> >
> > 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
> > > > <yekaterina.kantserova@oracle.com
> > > > <mailto:yekaterina.kantserova@oracle.com>> 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<yekaterina.kantserova@oracle.com
> > > > > <mailto:yekaterina.kantserova@oracle.com>> 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
> > > >
> > >
> >
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Staffan,<br>
<br>
Thanks for the review!<br>
<br>
// Katja<br>
<br>
<div class="moz-cite-prefix">On 04/28/2015 02:05 PM, Staffan Larsen
wrote:<br>
</div>
<blockquote
cite="mid:C863412D-A755-449C-B7E7-4F74A70EAB2B@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<div class="">Looks good!</div>
<div class=""><br class="">
</div>
<div class="">Thanks,</div>
<div class="">/Staffan</div>
<div class=""><br class="">
</div>
<div>
<blockquote type="cite" class="">
<div class="">On 24 apr 2015, at 16:17, Yekaterina Kantserova
<<a moz-do-not-send="true"
href="mailto:yekaterina.kantserova@oracle.com" \
class="">yekaterina.kantserova@oracle.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class=""> All
suggestions have been implemented. Please find new webrevs
here:<br class="">
<br class="">
webrev root: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eykantser/8059047/webrev.02">http://cr.openjdk.java.net/~ykantser/8059047/webrev.02</a><br
class="">
webrev jdk: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eykantser/8059047.jdk/webrev.02">http://cr.openjdk.java.net/~ykantser/8059047.jdk/webrev.02</a><br
class="">
webrev hotspot: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eykantser/8059047.hotspot/webrev.01">http://cr.openjdk.java.net/~ykantser/8059047.hotspot/webrev.01</a><br
class="">
<br class="">
// Katja<br class="">
<br class="">
<br class="">
<br class="">
<div class="moz-cite-prefix">On 04/24/2015 12:10 PM,
Staffan Larsen wrote:<br class="">
</div>
<blockquote
cite="mid:EE65F4CD-97F4-40A7-88CF-804FFEED408F@oracle.com"
type="cite" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252" class="">
<br class="">
<div class="">
<blockquote type="cite" class="">
<div class="">On 24 apr 2015, at 11:34, Yekaterina
Kantserova <<a moz-do-not-send="true"
href="mailto:yekaterina.kantserova@oracle.com"
class="">yekaterina.kantserova@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">Hi,<br class="">
<br class="">
Here comes the updated version.<br class="">
<br class="">
bug: <a moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8059047"
\
class="">https://bugs.openjdk.java.net/browse/JDK-8059047</a><br class="">
<br class="">
webrev root: <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Eykantser/8059047/webrev.01/"
\
class="">http://cr.openjdk.java.net/~ykantser/8059047/webrev.01/</a><br class="">
webrev jdk: <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Eykantser/8059047.jdk/webrev.01/"
\
class="">http://cr.openjdk.java.net/~ykantser/8059047.jdk/webrev.01/</a><br \
class=""> </div>
</blockquote>
<div class=""><br class="">
</div>
<div \
class="">test/com/sun/management/HotSpotDiagnosticMXBean/DumpHeap.java:</div> <div \
class=""><br class=""> </div>
<div class="">In main() I think you should change the
deleteOnExit() to just delete(). There is no reason
to wait with it.</div>
<div class=""><br class="">
</div>
<div class="">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.</div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<blockquote type="cite" class="">
<div class="">webrev hotspot: <a
moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Eykantser/8059047.hotspot/webrev.00/"
\
class="">http://cr.openjdk.java.net/~ykantser/8059047.hotspot/webrev.00/</a><br \
class=""> </div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">test/serviceability/dcmd/gc/HeapDumpTest.java:</div>
<div class=""><br class="">
</div>
<div class="">Same thing: call delete() instead of
deleteOnExit().</div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<blockquote type="cite" class="">
<div class=""><br class="">
<br class="">
One comment about changes in hotspot part. The
refactored version of
serviceability/dcmd/gc/HeapDumpTest.java doesn't
contain check:<br class="">
<br class="">
70 /*<br class="">
71 * Some hprof dumps of all objects
contain constantPoolOop references that cannot be
resolved, so we ignore<br class="">
72 * failures about resolving
constantPoolOop fields using a negative lookahead<br
class="">
73 */<br class="">
74
output.shouldNotMatch(".*WARNING(?!.*Failed
to resolve object.*constantPoolOop.*).*");<br
class="">
<br class="">
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.<br class="">
<br class="">
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.<br
class="">
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">Sounds right. A quick, hacky solution is
to redirect System.out to a stream or file while
running the parser…</div>
<div class=""><br class="">
</div>
<div class="">/Staffan</div>
<br class="">
<blockquote type="cite" class="">
<div class=""><br class="">
<br class="">
Thanks,<br class="">
Katja<br class="">
<br class="">
<br class="">
<br class="">
On 04/22/2015 03:09 PM, Staffan Larsen wrote:<br
class="">
<blockquote type="cite" class="">On 22 apr 2015,
at 11:17, Yekaterina Kantserova<<a
moz-do-not-send="true"
href="mailto:yekaterina.kantserova@oracle.com"
class="">yekaterina.kantserova@oracle.com</a>>
wrote:<br class="">
>>>>><br class="">
>>>>>Hi,<br class="">
>>>>><br class="">
>>>>>Could I please have a review
of this fix.<br class="">
>>>>><br class="">
>>>>>bug:<a
moz-do-not-send="true"
class="moz-txt-link-freetext" href="https:/">https://</a><a
moz-do-not-send="true"
href="http://bugs.openjdk.java.net/browse/JDK-8059047"
class="">bugs.openjdk.java.net/browse/JDK-8059047</a><br
class="">
>>>>>webrev:<a
moz-do-not-send="true"
class="moz-txt-link-freetext" href="http:/">http://</a><a
moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Eykantser/8059047/webrev.00/"
\
class="">cr.openjdk.java.net/~ykantser/8059047/webrev.00/</a><br class="">
>>>>><br class="">
>>>>>This fix is a part of JEP
241: Remove the jhat Tool (<a
moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8059039"
\
class="">https://bugs.openjdk.java.net/browse/JDK-8059039</a>).
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.<br class="">
>>>>><br class="">
>>>>>The old jhat packages have
been moved as follows:<br class="">
>>>>>com.sun.tools.hat.internal.model
-> jdk.test.lib.hprof.model<br class="">
>>>>>com.sun.tools.hat.internal.parser
-> jdk.test.lib.hprof.parser<br class="">
>>>>>com.sun.tools.hat.internal.util
-> jdk.test.lib.hprof.util<br class="">
>>>>><br class="">
>>>>>The source has not been
changed except Copyrights year.<br class="">
>>>>><br class="">
>>>>>Thanks,<br class="">
>>>>>Katja<br class="">
</blockquote>
<br class="">
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic