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

[Marcus Hirt]

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