Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre-resolve dependency graph #2181

Merged
merged 2 commits into from
Sep 11, 2018
Merged

Conversation

Qining
Copy link
Contributor

@Qining Qining commented Sep 5, 2018

Resolve the dependency graph in parallel immediately once the capture
data is resolve. This speed up the GAPIS side a bit (~30 sec for a
typical Vulkan app, 1.8G trace file) for buliding the instructions to
get the framebuffer data from the replay device.

based on #2172

@@ -230,6 +233,14 @@ func (s *server) LoadCapture(ctx context.Context, path string) (*path.Capture, e
if _, err = capture.ResolveFromPath(ctx, p); err != nil {
return nil, err
}
// Pre-resolve the dependency graph.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AWoloszyn I'm not sure if this is a proper place to insert the code. This makes the function name: 'LoadCapture' not that accurate. If so, we also need to do the same for 'ImportCapture'.

Another place is the capture.go:Import(), just after the path.Capture is built. Then we will need to resolve the capture there first, then resolve dependency graph.

Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using new context here because the context passed to LoadCapture is cancelled by gRPC? I'm not making any sort of judgement here - just curious.

As for your question - I think the most sensible place to pre-resolve would be at the end of capture.builder.build, but I'm fairly certain that you can't, as that would form a cyclic dependency between capture and dependencygraph. This seems reasonable for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly.

Cool, so I will just preresolve the graph here.

@@ -230,6 +233,14 @@ func (s *server) LoadCapture(ctx context.Context, path string) (*path.Capture, e
if _, err = capture.ResolveFromPath(ctx, p); err != nil {
return nil, err
}
// Pre-resolve the dependency graph.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using new context here because the context passed to LoadCapture is cancelled by gRPC? I'm not making any sort of judgement here - just curious.

As for your question - I think the most sensible place to pre-resolve would be at the end of capture.builder.build, but I'm fairly certain that you can't, as that would form a cyclic dependency between capture and dependencygraph. This seems reasonable for now.

@Qining
Copy link
Contributor Author

Qining commented Sep 11, 2018

Will drop the second commit: Do not cache capture file data 6402de1 then submit. The capture raw data is handled in #2179 and #2186

Resolve the dependency graph in parallel immediately once the capture
data is resolve. This speed up the GAPIS side a bit (~30 sec for a
typical Vulkan app, 1.8G trace file) for buliding the instructions to
get the framebuffer data from the replay device.
@Qining Qining force-pushed the pre-resolve-footprint branch from ffc9356 to d182024 Compare September 11, 2018 18:37
@Qining Qining merged commit 9871fcd into google:master Sep 11, 2018
@Qining Qining deleted the pre-resolve-footprint branch October 23, 2018 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants