Graph Visualization: Added hierarchy based on frames, faster function to get the 'pbtxt' and 'dot' formats, and implicit edges are added between commands and subcommands.

[ecapia]

No description provided.

[AWoloszyn]

It would be more canonical go with:

	currentGraph, err := createGraphFromDependencyGraph(ctx, dependencyGraph)
	if err != nil {
		return []byte{}, err
        }
	currentGraph.joinNodesByFrame()
	currentGraph.joinNodesWithZeroDegree()
	output := []byte{}
	if format == service.GraphFormat_PBTXT {
		output = currentGraph.getGraphInPbtxtFormat()
	} else if format == service.GraphFormat_DOT {
		output = currentGraph.getGraphInDotFormat()
	}

	return output, nil

[ecapia]

You are right, I added the error checking.

[bjoeris]

This recursive implementation makes me a bit nervous that a long chain of dependencies could cause a stack overflow.

[AWoloszyn]

Golang has an infinite stack. So unless we expect this to end up being a memory consumption issue, we should be ok there.

[bjoeris]

Ah, interesting!

[ecapia]

Thank you, I was not expecting an infinite stack. However, I implemented an iterative version (using a breadth first search). I think in some point it could end up a unnecessary memory consumption issue for huge graphs (millions of nodes and hundred of millions of edges).

[bjoeris]

Using an empty GlobalState actually works?

@AWoloszyn, is this safe in general? And if so, why does CmdFlags have a GlobalState argument?

[AWoloszyn]

This is not safe in general. I expect we would be ok in Vulkan, but it will likely segfault in GLES.

The way you would normally do this is get all of the EndOfFrame events from the trace (using a path.Events resolvable), and use those.
Alternatively you could insert those parameters into the dependency graph when creating it, which will have a valid global state.

gapis/resolve/events.go

[bjoeris]

I will store the CmdFlags in the dependency graph command nodes, and switch this to get the CmdFlags from the dependency graph in a separate PR.

[bjoeris]

Just an observation: Forward dependencies break this strategy. For example, if an image is created in frame 1 and destroyed in frame 2, the creation has a dependency to the destruction. Following that edge in the DFS would cause some of the frame 2 commands to be incorrectly placed in frame 1.

However, I think the issues from this should be pretty small, since nodes with forward dependencies (like resource destruction) probably don't have a lot of other dependencies. I also don't see an obviously more correct way to handle forward dependencies, since just ignoring them in the DFS will leave lots of orphan nodes not associated with any frame.

[bjoeris]

Do you need tail? It looks like tail should always be the end of visitedNodes.

Also, it looks like things will behave very strangely if visitedNodes were somehow not empty to start. I know this should not happen, but if you set head := len(*visitedNodes), it would be a non-issue.

[ecapia]

No, I do not . I was using tail for better understanding of the algorithm, but I think it is okay remove this.

I am assuming that always visitedNodes is empty at the beginning, but could be useful for some cases set head := len(*visitedNodes) such as get nodes from two frames, so I changed it. Thank you.

[bjoeris]

I don't doubt that individual WriteString calls are faster, but I think a single WriteString or fmt.Fprintf call per output line would be easier to read, and I suspect the performance difference is extremely small.

[ecapia]

Yes, the performance is almost the same considering small lengths of strings. Based on local becnhmarking and https://gist.github.com/dtjm/c6ebc86abe7515c988ec , using concatenation is a little faster than fmt.Fprintf. So I added concatenation by line.

[bjoeris]

lgtm