RFR(M): 8059047: Extract parser/validator from jhat for use in tests

[Staffan Larsen]

Looks good!

Thanks,
/Staffan

> On 24 apr 2015, at 16:17, Yekaterina Kantserova <yekaterina.kantserova at 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.kantserova at oracle.com <mailto:yekaterina.kantserova at 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.kantserova at oracle.com <mailto:yekaterina.kantserova at 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
>>> 
>> 
>