Graph Visualization: Added a 'Label' structure, debug marks , order of commandBuffer and reestructuration of the code. #2541

ecapia · 2019-01-14T19:04:43Z

No description provided.

AWoloszyn · 2019-01-14T20:20:44Z

gapis/resolve/dependencygraph2/graph_visualization/graph_algorithms.go

+	UNUSED           = "UNUSED"
+)
+
+func (g *graph) traverseGraph(currentNode *node, visitTime, minVisitTime, idInStronglyConnectedComponents, visitedNodesId *[]int, currentId, currentTime *int) {


These are large enough functions, it would be good to get comments on exactly what the parameters are supposed to be.

I abstracted the parameters used to compute the Strongly Connected Components in a directed graph based on Tarjan algorithm. Thank you.

AWoloszyn · 2019-01-14T20:22:43Z

gapis/api/graph_visualization.go

+	LevelsID   []int
+}
+
+func (label *Label) GetSize() int {


Godocs needed for all public methods.

Thank you, I added the Godocs for all public methods.

AWoloszyn · 2019-01-14T20:22:49Z

gapis/api/graph_visualization.go

+	"bytes"
+	"fmt"
+)
+


Godocs here as well.

I added the Godocs here as well. Thanks.

bjoeris

lgtm, modulo adding the godocs, our offline discussion about debug markers, and a few nits.

bjoeris · 2019-01-15T01:39:10Z

gapis/api/vulkan/graph_visualization.go

+	level := builder.labelsInsideDebugMarkers[len(builder.labelsInsideDebugMarkers)-1].GetSize() - 1
+	builder.numberOfDebugMarker++
+	for i := from; i <= to; i++ {
+		builder.labelsInsideDebugMarkers[i].Insert(level, VK_CMD_DEBUG_MARKER, builder.numberOfDebugMarker)


I don't understand why adding a debug marker affects multiple entries in labelsInsideDebugMarkers.

In fact some labels could be affected by more than one debug marker (nested debug markers), in that case the algorithm add more than one time the debug marker level. All commands between debug_marker_begin and debug_marker_end are affected.

bjoeris · 2019-01-15T01:40:49Z

gapis/api/vulkan/graph_visualization.go

+
+	} else if len(builder.positionOfDebugMarkersBegin) > 0 {
+		if labelOfLastDebugMarkerBegin.GetSize() != currentLabel.GetSize() {
+			builder.positionOfDebugMarkersBegin = builder.positionOfDebugMarkersBegin[:0]


It looks like this aborts ALL open debug markers as soon as ONE doesn't nest correctly with the non-debug markers. Is that a correct understanding?

We have a ton of code in vulkan.go to handle nesting of debug markers and similar, (as used for displaying groups in the UI) It handles some of these cases (bad nestings etc).

Is there a chance we can leverage that?
It may be a follow-up, but it may save some time in handling some of these cases.

Now I am adding debug markers as long as it does not destroy any level of hierarchy defined in commands. So, if exists some command between debug_marker_begin and debug_marker_end which has smaller Label size , adding the debug marker here will destroy the levels of hierarchy and for this case I just ignore the debug marker. It means I am preserving Labels with no-decreasing size. I have tested with several captures and it is working very nice. In addition to this, I am analyzing the possibility to include code from vulkan.go to address this problem as well, but if it is the case , I will add this in the last commit. Thank you.

bjoeris · 2019-01-15T01:56:19Z

gapis/api/vulkan/graph_visualization.go

 			}
 		}
 	}
-	if _, ok := builder.labelToHierarchy[label]; !ok {
-		builder.labelToHierarchy[label] = &api.Hierarchy{}
+	temporalLabelAsAString := label.GetLabelAsAString()


Is the name "temporal" intentional? Or was it meant to be "temporary"?

It is intentional, but I figured out that 'temporal' is not the correct word to describe something provisional , thank you. I changed it to temporary.

bjoeris · 2019-01-15T02:02:10Z

gapis/api/vulkan/graph_visualization.go

 	}
-	hierarchy := builder.labelToHierarchy[label]
+	hierarchy := builder.labelAsAStringToHierarchy[temporalLabelAsAString]


In general this pattern of inserting a default value into the map when the key is missing can be re-expressed as

hierarchy, ok := builder.labelAsAStringToHierarchy[temporalLabelAsAString] if !ok { hierarchy = &api.Hierarchy{} builder.labelAsAStringToHierarchy[temporalLabelAsAString] = hierarchy }

This removes one of the map lookups.

Thank you. Yes , using this I am removing one look up in the map. I changed it for the two cases.

bjoeris · 2019-01-15T02:11:00Z

gapis/resolve/dependencygraph2/graph_visualization/graph_structure.go

@@ -197,122 +189,6 @@ func (input nodeSorter) Less(i, j int) bool {
 	return input[i].id < input[j].id
 }

-func (g *graph) traverseGraph(currentNode *node, visitTime, minVisitTime, idInStronglyConnectedComponents, visitedNodesId *[]int, currentId, currentTime *int) {


When moving big chunks of code like this, it's usually preferred to do the move in a separate commit, since it is hard to see what changed. Just a comment, no need to change this.

I have abstracted this part of code to simplify the understanding, because it is essentially the Tarjan algorithm to compute Strongly Connected Components in a directed graph. I will remove this in a later commit.

bjoeris

lgtm

bjoeris · 2019-01-16T02:12:08Z

gapis/api/vulkan/graph_visualization.go

 }

-func (builder *labelForVulkanCommands) GetCommandLabel(command api.Cmd, commandNodeId uint64) string {
+func (builder *labelForVulkanCommands) GetCommandLabel(command api.Cmd, cmdId uint64) *api.Label {


These functions that implement the interfaces are also public, and should also have godocs. Those godocs are usually exactly the same as the godocs from the interface method they are implementing.

Thank you, I will add the Godocs here as well.

commandBuffer and reestructuration of the code. 'Label' Structure: It improves the time and space complexity for algorithms on the graph and the output format. Debug markers: They are added whenever do not break any previous hierarchy already defined. It follows the idea of balance parenthesis. Reestructuration of the code: This is divided in graph_algorithms, graph_structure, graph_output and graph_visualization for better understanding and future work.

googlebot added the cla: yes label Jan 14, 2019

AWoloszyn reviewed Jan 14, 2019

View reviewed changes

bjoeris reviewed Jan 15, 2019

View reviewed changes

ecapia force-pushed the graph_visualization_5 branch from 5f232a1 to fce6646 Compare January 16, 2019 01:32

bjoeris added the kokoro:run label Jan 16, 2019

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

bjoeris approved these changes Jan 16, 2019

View reviewed changes

ecapia force-pushed the graph_visualization_5 branch 2 times, most recently from 10bdf15 to c895efd Compare January 16, 2019 02:37

ecapia force-pushed the graph_visualization_5 branch from c895efd to 54a9bbc Compare January 16, 2019 02:43

bjoeris added the kokoro:run label Jan 16, 2019

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

bjoeris merged commit fdfbfbc into google:master Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graph Visualization: Added a 'Label' structure, debug marks , order of commandBuffer and reestructuration of the code. #2541

Graph Visualization: Added a 'Label' structure, debug marks , order of commandBuffer and reestructuration of the code. #2541

ecapia commented Jan 14, 2019

AWoloszyn Jan 14, 2019

ecapia Jan 15, 2019 •

edited

Loading

AWoloszyn Jan 14, 2019

ecapia Jan 16, 2019

AWoloszyn Jan 14, 2019

ecapia Jan 16, 2019 •

edited

Loading

bjoeris left a comment

bjoeris Jan 15, 2019

ecapia Jan 16, 2019 •

edited

Loading

bjoeris Jan 15, 2019

AWoloszyn Jan 15, 2019

ecapia Jan 16, 2019 •

edited

Loading

bjoeris Jan 15, 2019

ecapia Jan 15, 2019 •

edited

Loading

bjoeris Jan 15, 2019

ecapia Jan 15, 2019

bjoeris Jan 15, 2019

ecapia Jan 16, 2019 •

edited

Loading

bjoeris left a comment

bjoeris Jan 16, 2019

ecapia Jan 16, 2019

Graph Visualization: Added a 'Label' structure, debug marks , order of commandBuffer and reestructuration of the code. #2541

Graph Visualization: Added a 'Label' structure, debug marks , order of commandBuffer and reestructuration of the code. #2541

Conversation

ecapia commented Jan 14, 2019

Choose a reason for hiding this comment

ecapia Jan 15, 2019 • edited Loading

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 Jan 16, 2019 • edited Loading

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

ecapia Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecapia Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecapia Jan 15, 2019 • edited Loading

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 Jan 16, 2019 • edited Loading

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

ecapia Jan 15, 2019 •

edited

Loading

ecapia Jan 16, 2019 •

edited

Loading

ecapia Jan 16, 2019 •

edited

Loading

ecapia Jan 16, 2019 •

edited

Loading

ecapia Jan 15, 2019 •

edited

Loading

ecapia Jan 16, 2019 •

edited

Loading