Graph Visualization: Added the data structure for graph visualization, Dot and Pbtxt formats, super-nodes and unit testing for data structure. #2507

ecapia · 2019-01-02T19:47:26Z

No description provided.

Dot and Pbtxt formats from the graph, super-nodes (hierarchies) based on command type and graph properties. Added unit testing for graph data structure. Data Structure: The data structure for the graph visualization supports efficiently operations such as: addNode, addEdge, deleteNode, deleteEdge, etc. Get Dot and Pbtxt formats: The dot format allows visualize static graphs (using Graphviz and Gephi) and the Pbtxt allows visualize dynamic graphs using TensorBoard. Super-nodes (hierarchies): Due limits on human and TensorBoard visualization, creating super-nodes is necessary to improve performance. Initial super-nodes defined on command type (CommandBuffer, CommandRenderPass and CommandSubPass) and Strongly connected components.

AWoloszyn · 2019-01-02T19:52:23Z

gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go

+	VK_CMD_DRAW_INDEXED        = "vkCmdDrawIndexed"
+	VK_CMD_DRAW                = "vkCmdDraw"
+	COMMAND_BUFFER             = "commandBuffer"
+	MAXIMUM_LEVEL_IN_HIERARCHY = 10


These shouldn't be outside of the Vulkan package.
In general we keep the knowledge of an API confined to the package where at all possible.

Yes, I think it makes sense to abstract away this hierarchy logic into something like an interface providing getCommandLabel, which could be implemented by vulkan.API.

Okay, I moved this into //gapis/api and //gapis/api/vulkan. I put the functions and structures such that the main code (//gapis/resolve/dependencygraph2/graph_visualization/..) depends only from //gapis/api considering future changes in the next commits.

gapis/resolve/dependencygraph2/graph_visualization/graph_structure.go

bjoeris · 2019-01-02T20:30:15Z

gapis/resolve/dependencygraph2/graph_visualization/graph_structure.go

+	return newGraph
+}
+
+func (g *Graph) addNodeByDefault() int {


There are a lot of addNodeBy... variants that all seem to do almost the same thing. Could you get away with just one, or maybe 2 of these?

Alternatively, you could separate this out into something like newNode(id int, label string) *Node, then set the fields you care about, and then call something like (g *Graph) addNode(*Node) bool that actually updates g.

Yes. I have reduced this into two functions getNewNode(int,string) *Node and addNode(*Node), I think is the best option considering the future modifications in the next commits as well.

gapis/resolve/dependencygraph2/graph_visualization/graph_structure.go

gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go

bjoeris · 2019-01-02T21:01:18Z

gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go

+	VK_CMD_DRAW_INDEXED        = "vkCmdDrawIndexed"
+	VK_CMD_DRAW                = "vkCmdDraw"
+	COMMAND_BUFFER             = "commandBuffer"
+	MAXIMUM_LEVEL_IN_HIERARCHY = 10


Yes, I think it makes sense to abstract away this hierarchy logic into something like an interface providing getCommandLabel, which could be implemented by vulkan.API.

gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go

AWoloszyn · 2019-01-03T14:27:04Z

gapis/resolve/dependencygraph2/graph_visualization/graph_structure.go

+	return true
+}
+
+func (g *Graph) removeNodePreservingEdges(idNode int) bool {


Is this bool supposed to be success/failure?
If so, in Go it is more typical to return an error (or nil).

Yes, it is. I am using error now to handle that situation. In addition, I removed unnecessary returned errors from functions, keeping only the most relevant to detect bugs.

ecapia · 2019-01-04T01:37:12Z

I have pushed a new commit considering the recommendations and feedback

AWoloszyn · 2019-01-04T14:20:46Z

gapis/resolve/dependencygraph2/graph_visualization/graph_structure.go

+	return output
+}
+
+func (g *graph) getGraphInPbtxtFormat() string {


This looks like it is trying to write JSON:

I may suggest creating a golang object that matches the json object you are trying to write, and using the json package to output the Golang object directly.

This isn't quite JSON. It's TensorFlow's particular flavor of protobuf text format (which is different than the protobuf text format that Go's protobuf package produces).

Ahh I apologize, also yay new formats for everyone to write!

bjoeris · 2019-01-04T15:07:19Z

gapis/api/graph_visualization.go

+)
+
+type Hierarchy struct {
+	LevelId      [MAXIMUM_LEVEL_IN_HIERARCHY]int


Why is this a statically sized array, rather than a slice? Is 10 significant in some way, or just big enough that you think it's unlikely to ever need more than 10 levels? I also don't see any runtime checks to make sure the current level never exceeds 10.

I changed this in slice, also I added functions setValue and GetValue for this slice, because its length could increment dynamically.

I added functions to manage the Hierarchy in a better and simpler way. In this commit for the trace "multithreading" is obtained unexpected results because multiple threads running , but this problem is addressed in a later commit.

gapis/api/vulkan/graph_visualization.go

gapis/api/graph_visualization.go

gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go

gapis/resolve/dependencygraph2/graph_visualization/graph_structure_test.go

gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go

Dot and Pbtxt formats from the graph, super-nodes (hierarchies) based on command type and graph properties. Added unit testing for graph data structure. Data Structure: The data structure for the graph visualization supports efficiently operations such as: addNode, addEdge, deleteNode, deleteEdge, etc. Get Dot and Pbtxt formats: The dot format allows visualize static graphs (using Graphviz and Gephi) and the Pbtxt allows visualize dynamic graphs using TensorBoard. Super-nodes (hierarchies): Due limits on human and TensorBoard visualization, creating super-nodes is necessary to improve performance. Initial super-nodes defined on command type (CommandBuffer, CommandRenderPass and CommandSubPass) and Strongly connected components.

bjoeris

lgtm

bjoeris · 2019-01-07T14:23:27Z

gapis/api/vulkan/graph_visualization.go

+	parameters := command.CmdParams()
+	for _, parameter := range parameters {
+		if parameter.Name == COMMAND_BUFFER {
+			commandBuffer := parameter.Name + fmt.Sprintf("%d", parameter.Get()) + "/"


Nit: if you're using sprintf anyway, you might as well let sprintf also handle the concatenations.

Okay, I will consider that in next commits, thank you.

googlebot added the cla: yes label Jan 2, 2019

ecapia force-pushed the graph_visualization_2 branch from 93d5edf to dc19fb7 Compare January 2, 2019 19:48

AWoloszyn reviewed Jan 2, 2019

View reviewed changes

bjoeris requested changes Jan 2, 2019

View reviewed changes

AWoloszyn reviewed Jan 3, 2019

View reviewed changes

AWoloszyn reviewed Jan 4, 2019

View reviewed changes

bjoeris requested changes Jan 4, 2019

View reviewed changes

ecapia force-pushed the graph_visualization_2 branch 2 times, most recently from ee27b60 to 3b6969f Compare January 4, 2019 21:27

bjoeris added the kokoro:run label Jan 4, 2019

kokoro-team removed the kokoro:run label Jan 4, 2019

ecapia force-pushed the graph_visualization_2 branch from 3b6969f to 1b486c3 Compare January 5, 2019 02:56

ecapia force-pushed the graph_visualization_2 branch from 1b486c3 to a100a72 Compare January 6, 2019 22:35

bjoeris added the kokoro:run label Jan 7, 2019

kokoro-team removed the kokoro:run label Jan 7, 2019

bjoeris approved these changes Jan 7, 2019

View reviewed changes

bjoeris merged commit 1236ebb into google:master Jan 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graph Visualization: Added the data structure for graph visualization, Dot and Pbtxt formats, super-nodes and unit testing for data structure. #2507

Graph Visualization: Added the data structure for graph visualization, Dot and Pbtxt formats, super-nodes and unit testing for data structure. #2507

ecapia commented Jan 2, 2019

AWoloszyn Jan 2, 2019

bjoeris Jan 2, 2019

ecapia Jan 3, 2019

bjoeris Jan 2, 2019

ecapia Jan 3, 2019

bjoeris Jan 2, 2019

AWoloszyn Jan 3, 2019

ecapia Jan 4, 2019

ecapia commented Jan 4, 2019

AWoloszyn Jan 4, 2019

bjoeris Jan 4, 2019

AWoloszyn Jan 4, 2019

bjoeris Jan 4, 2019

ecapia Jan 4, 2019

ecapia Jan 5, 2019

bjoeris left a comment

bjoeris Jan 7, 2019

ecapia Jan 7, 2019

Graph Visualization: Added the data structure for graph visualization, Dot and Pbtxt formats, super-nodes and unit testing for data structure. #2507

Graph Visualization: Added the data structure for graph visualization, Dot and Pbtxt formats, super-nodes and unit testing for data structure. #2507

Conversation

ecapia commented Jan 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecapia commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjoeris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment