Review request for JMC-JMC-4057: Adding experimental flame graph view.

[Marcus Hirt]

Thanks Henrik!

/M

On Thu, Jul 11, 2019 at 6:47 PM Henrik Dafgård <hdafgard at gmail.com> wrote:
>
> I think this looks good, although in FlameGraphView.toJson returns an empty string for a null value, this wouldn't be valid JSON.
>
>
> Cheers,
> Henrik Dafgård
>
>
> On Thu, 11 Jul 2019 at 17:56, Marcus Hirt <marcus.hirt at datadoghq.com> wrote:
>>
>> Excellent! Henrik, can you also review?
>>
>> Kind regards,
>> Marcus
>>
>> On Thu, Jul 11, 2019 at 2:41 PM Jie Kang <jkang at redhat.com> wrote:
>> >
>> > On Tue, Jul 9, 2019 at 6:31 PM Marcus Hirt <marcus.hirt at datadoghq.com> wrote:
>> > >
>> > > Hi Jie,
>> > >
>> > > Thanks for taking a look!
>> > >
>> > > Here's an updated version:
>> > > http://cr.openjdk.java.net/~hirt/JMC-4057/webrev.1/
>> >
>> > Hi Marcus,
>> >
>> > I have one minor nit for:
>> >
>> > --- /dev/null 2019-07-09 22:26:24.682716100 +0200
>> > +++ new/application/org.openjdk.jmc.flightrecorder.ext.flameview/src/main/java/org/openjdk/jmc/flightrecorder/ext/flameview/tree/TraceTreeUtils.java
>> > 2019-07-09 23:56:58.542672600 +0200
>> > [...]
>> > + public static String printTree(TraceNode node) {
>> > + StringBuilder builder = new StringBuilder();
>> > + builder.append("=== Tree Printout ===%n");
>> >
>> > When I tried a reproducer in a static main:
>> >
>> > StringBuilder b = new StringBuilder();
>> > b.append("Hi%nBob!");
>> > System.out.println(b.toString());
>> >
>> > The result is
>> >
>> > Hi!%nBob!
>> >
>> > I think it would be more appropriate to use System.lineSeparator()
>> >
>> > The patch good to me; I think it's okay without further review from me!
>> >
>> > (Adding jmc-dev list to CC)
>> >
>> >
>> > Regards,
>> >
>> > >
>> > > Kind regards,
>> > > Marcus
>> > >
>> > > On Mon, Jul 8, 2019 at 4:04 PM Jie Kang <jkang at redhat.com> wrote:
>> > > >
>> > > > On Fri, Jun 21, 2019 at 11:19 AM Marcus Hirt <marcus at hirt.se> wrote:
>> > > > >
>> > > > > Hi all,
>> > > > >
>> > > > > Please review this change to add an experimental flame graph view.
>> > > > > See known issues in the Jira.
>> > > > >
>> > > > > Also cleaning up some imports, and excluding JOverflow from the
>> > > > > development launchers until JMC-6343 is fixed.
>> > > > >
>> > > > > Jira:   https://bugs.openjdk.java.net/browse/JMC-4057
>> > > > > Webrev: http://cr.openjdk.java.net/~hirt/JMC-4057/webrev.0/
>> > > >
>> > > > Hi Marcus,
>> > > >
>> > > > I have one comment below:
>> > > >
>> > > > *
>> > > > There are two spotbugs failures:
>> > > > VA_FORMAT_STRING_USES_NEWLINE: For TraceTreeUtils.printTree:110, the
>> > > > format string can use %n instead of /n for a platform-specific line
>> > > > separator. The use of \n can also be altered in
>> > > > TraceTreeUtils.printTree:104 to be platform-independent via
>> > > > System.lineSeparator().
>> > > > SBSC_USE_STRINGBUFFER_CONCATENATION: For TraceTreeUtils.indent:119,
>> > > > spotbugs would prefer a StringBuilder/StringBuffer for building the
>> > > > result.
>> > > >
>> > > > Apart from this, I think this patch as an initial commit of this work
>> > > > looks good! It worked for me on Fedora 30. For the record, I was not
>> > > > able to view the launcher changes due to the webrev issue. I have
>> > > > opened a JIRA issue [1] to track it and also supplied a potential fix
>> > > > through the mailing list [2].
>> > > >
>> > > > [1] https://bugs.openjdk.java.net/browse/JMC-6524
>> > > > [2] http://mail.openjdk.java.net/pipermail/jmc-dev/2019-July/001226.html
>> > > >
>> > > >
>> > > > Regards,
>> > > >
>> > > > >
>> > > > > Kind regards,
>> > > > > Marcus
>> > > > >