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

[Henrik Dafgård]

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
> > > > >
>