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

[Jie Kang]

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