Skip to content

Commit

Permalink
6670: Harmonize ~ Unclassifiable ~ across Flame View and Stacktrace View
Browse files Browse the repository at this point in the history 
cleaning, tooltip improvement
  • Loading branch information
@mirage22
mirage22 committed Jan 18, 2020
1 parent 60a1b66 commit 44d3e52
Show file tree
Hide file tree
Showing 10 changed files with 60 additions and 196 deletions.
58 changes: 0 additions & 58 deletions ...in/java/org/openjdk/jmc/flightrecorder/flameview/messages/internal/FlameViewMessages.java
View file

This file was deleted.

17 changes: 15 additions & 2 deletions ...rder.flameview/src/main/java/org/openjdk/jmc/flightrecorder/flameview/tree/TraceNode.java
View file
Expand Up @@ -40,12 +40,14 @@ public class TraceNode {
private final int value;
private final String name;
private final String packageName;
private final String desc;
private final List<TraceNode> children = new ArrayList<>();

public TraceNode(String name, int value, String packageName) {
public TraceNode(String name, int value, String packageName, String desc) {

This comment has been minimized.

Sign in to view

Copy link
@thegreystone

thegreystone Jan 19, 2020

Collaborator

Do we really need this? We can keep the data structure smaller by just setting name or packageName to UNCLASSIFIABLE_FRAME, and take care of it in the JSon generation.

This comment has been minimized.

Sign in to view

Copy link
@mirage22

mirage22 Jan 19, 2020 •
edited

Author Owner

I've been thinking about two options, it was the one. I've a change the code to the second. TraceNode stays without any changes.
this.name = name;
this.value = value;
this.packageName = packageName;
this.desc = desc;
}

public int getValue() {
Expand All @@ -59,6 +61,10 @@ public String getName() {
public String getPackageName() {
return packageName;
}

public String getDesc() {
return desc;
}

public List<TraceNode> getChildren() {
return children;
Expand All @@ -75,6 +81,7 @@ public int hashCode() {
result = prime * result + ((children == null) ? 0 : children.hashCode());
result = prime * result + ((name == null) ? 0 : name.hashCode());
result = prime * result + ((packageName == null) ? 0 : packageName.hashCode());
result = prime * result + ((desc == null) ? 0 : desc.hashCode());
result = prime * result + value;
return result;
}
Expand Down Expand Up @@ -104,13 +111,19 @@ public boolean equals(Object obj) {
}
} else if (!packageName.equals(other.packageName))
return false;
if (desc == null) {
if (other.desc != null) {
return false;
}
} else if (!desc.equals(other.desc))
return false;
if (value != other.value)
return false;
return true;
}

public String toString() {
return "TraceNode [name: " + name + ", value: " + value + ", packageName: " + packageName + ", children: "
return "TraceNode [name: " + name + ", value: " + value + ", packageName: " + packageName + ", desc: "+ desc + ", children: "
+ children.size() + "]";
}
}
52 changes: 26 additions & 26 deletions ...flameview/src/main/java/org/openjdk/jmc/flightrecorder/flameview/tree/TraceTreeUtils.java
View file
Expand Up @@ -38,18 +38,40 @@
import org.openjdk.jmc.common.IMCMethod;
import org.openjdk.jmc.common.item.IItemCollection;
import org.openjdk.jmc.common.util.FormatToolkit;
import org.openjdk.jmc.flightrecorder.flameview.messages.internal.FlameViewMessages;
import org.openjdk.jmc.flightrecorder.stacktrace.FrameSeparator;
import org.openjdk.jmc.flightrecorder.stacktrace.StacktraceModel;
import org.openjdk.jmc.flightrecorder.stacktrace.StacktraceModel.Branch;
import org.openjdk.jmc.flightrecorder.stacktrace.StacktraceModel.Fork;
import org.openjdk.jmc.flightrecorder.ui.messages.internal.Messages;
import org.openjdk.jmc.flightrecorder.stacktrace.FrameSeparator.FrameCategorization;
import org.openjdk.jmc.flightrecorder.stacktrace.StacktraceFrame;

public class TraceTreeUtils {
public final static String DEFAULT_ROOT_NAME = "__root";
public final static String DEFAULT_ROOT_PACKAGE_NAME = "";
public final static String DEFAULT_TRACE_NODE_DESC = "";
public final static FrameSeparator DEFAULT_FRAME_SEPARATOR = new FrameSeparator(FrameCategorization.METHOD, false);

private static class TraceNodeFactory {

private static TraceNode getRootTraceNode(String rootName, Fork rootFork) {
return new TraceNode(rootName == null ? DEFAULT_ROOT_NAME : rootName, rootFork.getItemsInFork(),
DEFAULT_ROOT_PACKAGE_NAME, DEFAULT_TRACE_NODE_DESC);
}

private static TraceNode getTraceNodeByStacktraceFrame(StacktraceFrame sFrame) {
IMCFrame frame = sFrame.getFrame();
IMCMethod method = frame.getMethod();
String packageName = FormatToolkit.getPackage(method.getType().getPackage());
if (frame == StacktraceModel.UNKNOWN_FRAME) {
return new TraceNode(Messages.Flameview_UNCLASSIFIABLE_FRAME, sFrame.getItemCount(),
packageName, Messages.Flameview_UNCLASSIFIABLE_FRAME_DESC);
} else {
String name = FormatToolkit.getHumanReadable(method, false, false, true, false, true, false);
return new TraceNode(name, sFrame.getItemCount(), packageName, DEFAULT_TRACE_NODE_DESC);
}
}
}

/**
* Traces a TraceTree from a {@link StacktraceModel}.
Expand All @@ -60,8 +82,7 @@ public class TraceTreeUtils {
*/
public static TraceNode createTree(StacktraceModel model, String rootName) {
Fork rootFork = model.getRootFork();
TraceNode root = new TraceNode(rootName == null ? DEFAULT_ROOT_NAME : rootName, rootFork.getItemsInFork(),
DEFAULT_ROOT_PACKAGE_NAME);
TraceNode root = TraceNodeFactory.getRootTraceNode(rootName, rootFork);
for (Branch branch : rootFork.getBranches()) {
addBranch(root, branch);
}
Expand All @@ -82,11 +103,10 @@ public static TraceNode createTree(

private static void addBranch(TraceNode root, Branch branch) {
StacktraceFrame firstFrame = branch.getFirstFrame();
TraceNode currentNode = new TraceNode(format(firstFrame), firstFrame.getItemCount(),
formatPackageName(firstFrame));
TraceNode currentNode = TraceNodeFactory.getTraceNodeByStacktraceFrame(firstFrame);
root.addChild(currentNode);
for (StacktraceFrame frame : branch.getTailFrames()) {
TraceNode newNode = new TraceNode(format(frame), frame.getItemCount(), formatPackageName(frame));
TraceNode newNode = TraceNodeFactory.getTraceNodeByStacktraceFrame(frame);
currentNode.addChild(newNode);
currentNode = newNode;
}
Expand All @@ -99,26 +119,6 @@ private static void addFork(TraceNode node, Fork fork) {
}
}

private static String format(StacktraceFrame sFrame) {
IMCFrame frame = sFrame.getFrame();
if (frame == StacktraceModel.UNKNOWN_FRAME) {
return FlameViewMessages.getString(FlameViewMessages.FLAMEVIEW_UNCLASSIFIABLE_FRAME);
} else {
IMCMethod method = frame.getMethod();
return FormatToolkit.getHumanReadable(method, false, false, true, false, true, false);
}
}

private static String formatPackageName(StacktraceFrame sFrame) {
IMCFrame frame = sFrame.getFrame();
if (frame == StacktraceModel.UNKNOWN_FRAME) {
return FlameViewMessages.getString(FlameViewMessages.FLAMEVIEW_UNCLASSIFIABLE_FRAME_PACKAGE);
} else {
IMCMethod method = frame.getMethod();
return FormatToolkit.getPackage(method.getType().getPackage());
}
}

public static String printTree(TraceNode node) {
StringBuilder builder = new StringBuilder();
builder.append("=== Tree Printout ===");
Expand Down
5 changes: 3 additions & 2 deletions ...lameview/src/main/java/org/openjdk/jmc/flightrecorder/flameview/views/FlameGraphView.java
View file
Expand Up @@ -253,8 +253,9 @@ private static String render(TraceNode root) {
}

private static void render(StringBuilder builder, TraceNode node) {
String start = String.format("{%s,%s,%s, \"c\": [ ", toJSonKeyValue("n", node.getName()),
toJSonKeyValue("p", node.getPackageName()), toJSonKeyValue("v", String.valueOf(node.getValue())));
String start = String.format("{%s,%s,%s,%s, \"c\": [ ", toJSonKeyValue("n", node.getName()),

This comment has been minimized.

Sign in to view

Copy link
@thegreystone

thegreystone Jan 19, 2020

Collaborator

Shouldn't we just add a tooltip override when needed (pretty much once), instead of adding empty d nodes all over?

This comment has been minimized.

Sign in to view

Copy link
@mirage22

mirage22 Jan 19, 2020

Author Owner

this part is in my focus due to unnecessary content generation.

This comment has been minimized.

Sign in to view

Copy link
@mirage22

mirage22 Jan 19, 2020 •
edited

Author Owner

I've add different rendering strategies. the field d is present only in case when description is available otherwise the Json element doesn't contain it. The Tooltip is properly generated based on the Json element
toJSonKeyValue("p", node.getPackageName()), toJSonKeyValue("d", node.getDesc()),
toJSonKeyValue("v", String.valueOf(node.getValue())));
builder.append(start);
for (int i = 0; i < node.getChildren().size(); i++) {
render(builder, node.getChildren().get(i));
Expand Down
13 changes: 11 additions & 2 deletions ...rg.openjdk.jmc.flightrecorder.flameview/src/main/resources/jsjmclibs/flameviewColoring.js
View file
Expand Up @@ -43,6 +43,8 @@ String.prototype.hashCode = function () {
return hash;
};

const emptyString = "";
const htmlTagBr = "\u003Cbr\u002F\u003E";
const rootPackageColor = "darkred";
const invalidPackageColor = "snow";
const packageJavaColorLightGray = "lightgray";
Expand All @@ -58,7 +60,7 @@ const packageMarkerComSunAndJdk = "comSunAndJdk";
const packageMarkerRest = "rest";
const packagesIdentifierMap = new Map().set("java.", packageMarkerJava).set("sun.", packageMarkerSun)
.set("com.sun.", packageMarkerComSunAndJdk).set("jdk.", packageMarkerComSunAndJdk);
const packageColorMap = new Map().set("", rootPackageColor);
const packageColorMap = new Map().set(emptyString, rootPackageColor);

const colorByPackage = function (p) {
if (p === undefined) {
Expand Down Expand Up @@ -120,5 +122,12 @@ const colorCell = function (d) {
};

const adjustTip = function (d) {
return d.data.n + "\u003Cbr\u002F\u003Epackage: " + d.data.p + "\u003Cbr\u002F\u003Esamples: " + d.data.v;
var tipMessage = d.data.n + htmlTagBr;
if( d.data.d !== emptyString) {
tipMessage += "description: " + d.data.d + htmlTagBr;
} else {
tipMessage += "package: " + d.data.p + htmlTagBr;
}
tipMessage += "samples: " + d.data.v;
return tipMessage;
};
35 changes: 0 additions & 35 deletions .../resources/org/openjdk/jmc/flightrecorder/flameview/messages/internal/messages.properties
View file

This file was deleted.

35 changes: 0 additions & 35 deletions ...sources/org/openjdk/jmc/flightrecorder/flameview/messages/internal/messages_ja.properties
View file

This file was deleted.

35 changes: 0 additions & 35 deletions ...rces/org/openjdk/jmc/flightrecorder/flameview/messages/internal/messages_zh_CN.properties
View file

This file was deleted.

4 changes: 3 additions & 1 deletion ...corder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/messages/internal/Messages.java
View file
Expand Up @@ -523,7 +523,9 @@ public class Messages extends NLS {
public static String VMOperationPage_PAGE_NAME;
public static String VMOperationPage_ROW_VM_OPERATIONS;
public static String VMOperationPage_TIMELINE_SELECTION;

public static String Flameview_UNCLASSIFIABLE_FRAME;

This comment has been minimized.

Sign in to view

Copy link
@thegreystone

thegreystone Jan 19, 2020

Collaborator

I think we want to use STACKTRACE_UNCLASSIFIABLE_FRAME for both the stacktrace view and the flame view, instead of having a copy in two places. Find a better home for it.

This comment has been minimized.

Sign in to view

Copy link
@mirage22

mirage22 Jan 19, 2020

Author Owner

I've corrected the code there is only one STACKTRACE_UNCLASSIFIABLE_FRAME used
public static String Flameview_UNCLASSIFIABLE_FRAME_DESC;

static {
NLS.initializeMessages(BUNDLE_NAME, Messages.class);
}
Expand Down
2 changes: 2 additions & 0 deletions ...rc/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties
View file
Expand Up @@ -612,3 +612,5 @@ MemoryLeakPage_STEPS_SKIPPED={0} {1} skipped steps from parent
MethodProfilingPage_CLASS_HISTOGRAM_SELECTION=Method Profiling Class Histogram Selection
MethodProfilingPage_PACKAGE_HISTOGRAM_SELECTION=Method Profiling Package Histogram Selection
MethodProfilingPage_PAGE_NAME=Method Profiling
Flameview_UNCLASSIFIABLE_FRAME=~ UNCLASSIFIABLE ~
Flameview_UNCLASSIFIABLE_FRAME_DESC=Unclassified means the stacktrace reached the stackdepth limit

0 comments on commit 44d3e52

Please sign in to comment.