Looks good! Thanks, /Staffan
> On 24 apr 2015, at 16:17, Yekaterina Kantserova > <yekaterina.kantser...@oracle.com> wrote: > > All suggestions have been implemented. Please find new webrevs here: > > webrev root: http://cr.openjdk.java.net/~ykantser/8059047/webrev.02 > <http://cr.openjdk.java.net/~ykantser/8059047/webrev.02> > webrev jdk: http://cr.openjdk.java.net/~ykantser/8059047.jdk/webrev.02 > <http://cr.openjdk.java.net/~ykantser/8059047.jdk/webrev.02> > webrev hotspot: > http://cr.openjdk.java.net/~ykantser/8059047.hotspot/webrev.01 > <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.kantser...@oracle.com >>> <mailto:yekaterina.kantser...@oracle.com>> wrote: >>> >>> Hi, >>> >>> Here comes the updated version. >>> >>> bug: https://bugs.openjdk.java.net/browse/JDK-8059047 >>> <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.kantser...@oracle.com >>>> <mailto:yekaterina.kantser...@oracle.com>> wrote: >>>> >>>>> >>>> >>>>>Hi, >>>> >>>>> >>>> >>>>>Could I please have a review of this fix. >>>> >>>>> >>>> >>>>>bug:https:// <https:/>bugs.openjdk.java.net/browse/JDK-8059047 >>>> >>>>><http://bugs.openjdk.java.net/browse/JDK-8059047> >>>> >>>>>webrev:http:// >>>> >>>>><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 >>>> >>>>><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 >>> >> >
