Improve command tree grouping #884
Conversation
| @@ -448,7 +448,7 @@ func TestAddAtomsWithSplitting(t *testing.T) { | |||
| ctx := log.Testing(t) | |||
| got := buildTestGroup(700) | |||
|
|
|||
| got.AddAtoms(func(CmdID) bool { return true }, 45) | |||
| got.AddAtoms(func(CmdID) bool { return true }, 45, 0) | |||
There was a problem hiding this comment.
Can we please have a test for maxNeighbours?
gapis/resolve/command_tree.go
Outdated
| return log.Errf(ctx, err, "Couldn't get events") | ||
| } | ||
|
|
||
| func addDrawEvents(ctx context.Context, events *service.Events, p *path.CommandTree, t *commandTree, last api.CmdID) { |
There was a problem hiding this comment.
What's the fix for the "silently ignored error" of which you speak?
There was a problem hiding this comment.
return log.Errf(ctx, err, "Couldn't get events") was always ignored before
gapis/resolve/command_tree.go
Outdated
| @@ -372,20 +372,31 @@ func addDrawEvents(ctx context.Context, events *service.Events, p *path.CommandT | |||
| } | |||
|
|
|||
| func addFrameEvents(ctx context.Context, events *service.Events, p *path.CommandTree, t *commandTree, last api.CmdID) { | |||
| frameCount, frameStart, frameEnd := 0, api.CmdID(0), api.CmdID(0) | |||
| frameCount, frameStart := 0, api.CmdID(0) | |||
| for _, e := range events.List { | |||
| i := api.CmdID(e.Command.Indices[0]) | |||
| switch e.Kind { | |||
There was a problem hiding this comment.
I'm not sure about removing the use of FirstInFrame. Don't we want to use this information if it's provided by the API? For example eglSwapBuffers is now annotated with @StartOfFrame, so we're actually deriving our start-of-frame here from the end-of-frame event, which is actually derived from the @StartOfFrame annotation. Ewww.
gapis/api/cmd_id_group.go
Outdated
| Name string // Name of this group. | ||
| Range CmdIDRange // The range of commands this group (and items) represents. | ||
| Spans Spans // All sub-groups and sub-ranges of this group. | ||
| Thumbnail CmdID // The command which shall be used to make thumbnail for this group |
There was a problem hiding this comment.
I really don't think this information belongs here. The types in the gapis/api package are supposed to be core and generic data structures relating to APIs, Commands and basic resource types. Thumbnail is a very specific usage.
It's unfortunate that the resolve.commandTree structure doesn't have a separate node type for its representation of the command tree.
Given that you're just doing a binary search on the events list to get the thumbnail for the group, you could just do this on demand in resolve.CommandTreeNode()
I want to be about to easily distinguish what is user-defined group, and what is gapis-generated group (heuristics).
For pure-heuristic based grouping, this has no effect as we do not generate this pattern. For captures with a lot of user defined groups, this makes the user defined groups easier to find and navigate.
Just to make the code easier to read/modify Also fix silently ignored error
This does not have effect on the heuristic based grouping, but it ensures that we still have frames in the presence of user-defined debug groups.
This ensures we create the draw groups even in the presence of user-defined groups.
This behaviour fixes my current frame issue, and I do not believe we care in other cases.
|
PTAL |
This avoids a common problem where the framebuffer is unbound at the end of the group.
|
PS: I added the missing local changes to the last commit. |
No description provided.