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

Make AllResourceData resolvable based on resource type. #2512

Merged
merged 1 commit into from
Jan 17, 2019

Conversation

AWoloszyn
Copy link
Contributor

This means that dump_resources does not have to actually
kick off replays. It also means we don't have to store
as much data if we do not need all of it.

Fixes #2435

Copy link
Contributor

@Qining Qining left a comment

Choose a reason for hiding this comment

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

lgtm.

@@ -845,6 +845,7 @@ message RenderSettings {
// Resources contains the full list of resources used by a capture.
message Resources {
repeated ResourcesByType types = 1;
map<string, api.ResourceType> resourceTypes = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should have a better name for resourceTypes to indicate this is a map from resource id to resource types.

Copy link
Member

Choose a reason for hiding this comment

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

why would we need this map? The resources are already returned by type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so we can answer the question:
I have resourceID what type is that. We could get the same information out of the proto, but its backwards, given resourceID

  1. loop through all types
  2. loop through all ids, to see if Foo is found.
  3. Profit? (n^2 if we have to do this lookup repeatedly)

The reason it went into this message at all, was because it is free to compute along with the rest of the resources, otherwise we would have to re-mutate the entire world to generate this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used above
gapis/resolve/resource_data.go:141

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: The only other "efficient" way I could see of doing this was forcing the Client to add the type in the request.
I.e. have the path be

// Copy of resource_type due to who includes whom
// Or move it entirely into path.proto but that seems worse
enum ResourceType {
  // UnknownResource represents an unknown resource type
  UnknownResource = 0;
  // Texture represents the Texture resource type
  TextureResource = 1;
  // ShaderResource represents the Shader resource type
  ShaderResource = 2;
  // ProgramResource represents the Program resource type
  ProgramResource = 3;
  // PipelineResource respresents the Pipeline resource type
  PipelineResource = 4;
}

message ResourceData {
  ID ID = 1;
  Command after = 2;
  ResourceType type = 3;
}

Copy link
Member

Choose a reason for hiding this comment

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

OK. Alright, I think I can live with it :). Here are my concerns:

  • this is serialized to the client and stays resident in memory there
  • not a big fan of the ID.String() part as keys

I think the only way around it is to have an intermediate value stored in the DB and convert from it for both calls, but I'm not sure it's worth it...

This means that dump_resources does not have to actually
kick off replays. It also means we don't have to store
as much data if we do not need all of it.
@AWoloszyn AWoloszyn merged commit 2e6ae01 into google:master Jan 17, 2019
@AWoloszyn AWoloszyn deleted the all-res-data branch January 17, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants