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.

Sign up for GitHub

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

Pre-resolve dependency graph #2181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8bd7d1a
add missing comments
Qining Sep 4, 2018
d182024
Pre-resolve dependency graph
Qining Sep 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gapis/api/vulkan/replay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ func (a API) Replay(
if opt {
// If we have not set up the dependency graph, do it now.
if dceInfo.ft == nil {
ft, err := dependencygraph.GetFootprint(ctx, intent.Device)
ft, err := dependencygraph.GetFootprint(ctx, intent.Capture)
if err != nil {
return 0, err
}
Expand Down
6 changes: 6 additions & 0 deletions gapis/replay/builder/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,9 @@ func (b *Builder) Build(ctx context.Context) (gapir.Payload, PostDataHandler, No
log.I(ctx, "Resource count: %d", len(payload.Resources))
}

// Make a copy of the reference of the finished decoder list to cut off the
// connection between the builder and furture uses of the decoders so that
// the builder do not need to be kept alive when using these decoders.
decoders := b.decoders
handlePost := func(pd *gapir.PostData) {
// TODO: should we skip it instead of return error?
Expand All @@ -652,6 +655,9 @@ func (b *Builder) Build(ctx context.Context) (gapir.Payload, PostDataHandler, No
})
}

// Make a copy of the reference of the finished notification reader list to
// cut off the connection between the builder and future uses of the readers
// so that the builder do not need to be kept alive when using these readers.
readers := b.notificationReaders
handleNotification := func(n *gapir.Notification) {
ctx = log.Enter(ctx, "NotificationHandler")
Expand Down
14 changes: 3 additions & 11 deletions gapis/resolve/dependencygraph/footprint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/google/gapid/gapis/api"
"github.com/google/gapid/gapis/capture"
"github.com/google/gapid/gapis/database"
"github.com/google/gapid/gapis/replay"
"github.com/google/gapid/gapis/resolve/initialcmds"
"github.com/google/gapid/gapis/service/path"
)
Expand Down Expand Up @@ -163,10 +162,9 @@ type FootprintBuilder interface {
}

// GetFootprint returns a pointer to the resolved Footprint.
func GetFootprint(ctx context.Context, device *path.Device) (*Footprint, error) {
func GetFootprint(ctx context.Context, c *path.Capture) (*Footprint, error) {
r, err := database.Build(ctx, &FootprintResolvable{
Capture: capture.Get(ctx),
Device: device,
Capture: c,
})
if err != nil {
return nil, fmt.Errorf("Counld not get execution foot print: %v", err)
Expand All @@ -176,18 +174,12 @@ func GetFootprint(ctx context.Context, device *path.Device) (*Footprint, error)

// Resolve implements the database.Resolver interface.
func (r *FootprintResolvable) Resolve(ctx context.Context) (interface{}, error) {
ctx = capture.Put(ctx, r.Capture)
if d := r.Device; d != nil {
ctx = replay.PutDevice(ctx, d)
}

c, err := capture.Resolve(ctx)
c, err := capture.ResolveFromPath(ctx, r.Capture)
if err != nil {
return nil, err
}
cmds := c.Commands
// If the capture contains initial state, prepend the commands to build the state.

initialCmds, ranges, err := initialcmds.InitialCommands(ctx, r.Capture)
if err != nil {
return nil, err
Expand Down
1 change: 1 addition & 0 deletions gapis/server/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ go_library(
"//gapis/replay:go_default_library",
"//gapis/replay/devices:go_default_library",
"//gapis/resolve:go_default_library",
"//gapis/resolve/dependencygraph:go_default_library",
"//gapis/service:go_default_library",
"//gapis/service/path:go_default_library",
"//gapis/stringtable:go_default_library",
Expand Down
11 changes: 11 additions & 0 deletions gapis/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ import (
"sync/atomic"
"time"

"github.com/google/gapid/core/app/crash"
"github.com/google/gapid/core/app/crash/reporting"
"github.com/google/gapid/core/context/keys"

"github.com/google/gapid/core/app"
"github.com/google/gapid/core/app/analytics"
Expand All @@ -43,6 +45,7 @@ import (
"github.com/google/gapid/gapis/replay"
"github.com/google/gapid/gapis/replay/devices"
"github.com/google/gapid/gapis/resolve"
"github.com/google/gapid/gapis/resolve/dependencygraph"
"github.com/google/gapid/gapis/service"
"github.com/google/gapid/gapis/service/path"
"github.com/google/gapid/gapis/stringtable"
Expand Down Expand Up @@ -239,6 +242,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.

crash.Go(func() {
newCtx := keys.Clone(context.Background(), ctx)
_, err = dependencygraph.GetFootprint(newCtx, p)
if err != nil {
log.E(newCtx, "Error resolve dependency graph: %v", err)
}
})
return p, nil
}

Expand Down