Vulkan memory breakdown by sean-purcell · Pull Request #1954 · google/gapid

sean-purcell · 2018-06-04T16:44:23Z

This creates a GAPIS service endpoint to get memory layout information at a given point in a trace, and implement it for Vulkan. It also adds a gapit verb to print out this information textually, along with information about aliased regions. No visual representation exists for now, just a text report.

sean-purcell · 2018-06-04T16:46:22Z

gapis/resolve/metrics.go

+
+}
+
+// FIXME DO NOT MERGE: Not sure where the best spot for this is, it can't go in gapis/api because api can't depend on service


This is likely not the best spot for this interface, maybe it should go in gapis/service along with the MemoryLayout proto definition?

It most likely belongs in gapis/api. This is similar to the Mesh API: gapis/api/mesh.go.

Ok, and then the Metrics proto definition should be moved to gapis/api/service.proto?

apazylbe · 2018-06-04T19:01:03Z

cmd/gapit/memory.go

+
+type bindingSlice []*service.MemoryBinding
+
+func (bindings bindingSlice) BindingLess(i, j int) bool {


Does it need to be public? If so, consider adding documenting.

apazylbe · 2018-06-04T19:02:48Z

cmd/gapit/memory.go

+
+func (bindings bindingSlice) BindingLess(i, j int) bool {
+	if bindings[i].Offset < bindings[j].Offset {
+		return true


Could simply, up to you
if bindings[i].Offset != bindings[j].Offset {
return bindings[i].Offset < bindings[j].Offset
}

apazylbe · 2018-06-04T19:20:34Z

cmd/gapit/memory.go

+			active[handle] = struct{}{}
+		}
+
+		if len(active) > 1 && i < len(points)-1 {


Could also do
for _, p := range points[:len(points)-1] above

apazylbe · 2018-06-04T19:25:19Z

gapis/api/vulkan/memory_layout.go

+// allocations, and which resources are bound to which locations in memory.
+func (a API) MemoryLayout(st *api.GlobalState) (*service.MemoryLayout, error) {
+	s := GetState(st)
+	allocations := []*service.MemoryAllocation{}


Consider pre-allocating with an appropriate length if it is known

qining · 2018-06-04T18:40:50Z

cmd/gapit/flags.go

 	}
+	MemoryFlags struct {
+		Gapis GapisFlags
+		At    flags.U64Slice `help:"command/subcommand index to get the state after. Empty for last"`


Looks like this is about memory layout not the state block?

qining · 2018-06-04T18:59:03Z

cmd/gapit/memory.go

+
+type bindingSlice []*service.MemoryBinding
+
+func (bindings bindingSlice) BindingLess(i, j int) bool {


Document or blurb?

qining · 2018-06-04T19:06:04Z

gapis/api/vulkan/memory_layout.go

+	s := GetState(st)
+	allocations := []*service.MemoryAllocation{}
+	// Serialize data on all allocations into protobufs
+	for handle, info := range *(s.DeviceMemories().Map) {


Prefer to s.DeviceMemories().All()

qining · 2018-06-04T19:10:07Z

gapis/api/vulkan/memory_layout.go

+			typ = service.MemoryBinding_IMAGE
+			size = image.MemoryRequirements().Size()
+		} else {
+			return nil, fmt.Errorf("Bound object %v is not a buffer or an image", handle)


I remember for sparse image and buffers, we didn't register the binding info on the memory object, only the buffer/image object. So we probably want to have a TODO or Note here to say some cases are not handled yet.

qining · 2018-06-04T19:15:09Z

gapis/service/service.proto

+}
+
+// The description of the memory layout of the API state
+message MemoryLayout {


Feel MemoryLayout name is a little bit confusing in GAPID, as we use this name for 'alignment, data width' things also. May be something like: MemoryUsage?

@AWoloszyn @ben-clayton @pmuetschard Any better idea or just using MemoryLayout is fine?

What about 'MemoryBreakdown'?

That is a fair point, overloading terms may not serve us well.
MemoryBreakdown is fine with me. Even a MemoryUsage (although people may expect a single number out of that).

qining · 2018-06-04T19:16:31Z

gapis/service/service.proto

+  // The device this allocation was made on
+  uint64 device = 1;
+  // The memory type this allocation is from
+  uint32 memory_type = 2;


Looks like this is more like memory_type_index? But index also sounds too Vulkan specific, I'm not sure if in GL world there is a thing like type index.

Yes, it is memory type index in Vulkan, I didn't want to make it overspecific to Vulkan though.

GL works a bit differently, since all memory is implicit. The allocations will be 1:1 with the resources.
I would expect something like "memory_type" to be "GL_IMAGE or GL_BUFFER" most likely.

qining · 2018-06-04T19:27:13Z

gapis/service/service.proto

+  // Constant index for looking up the flag values with the path.ConstantSet endpoint
+  uint32 constant_set = 4;
+  // The API specific ID for this allocation
+  uint64 id = 5;


Is this Id used? And should we name it 'handle'?

@ben-clayton suggested separated id and name for bindings, wasn't sure if it would also be useful for allocations. Open to using either id or handle, not sure which is a more descriptive name.

I have a small preference for Handle. It more closely maps to how I view things. (That is not to say my view is right).

Would you prefer to also keep name or is just handle good enough?

I think keeping the name is fine. It means we can use debug names if assigned.

AWoloszyn · 2018-06-06T15:45:01Z

gapis/service/service.proto

+  // The offset of the mapping into the source allocation, in bytes
+  uint64 offset = 2;
+  // The address on the host that this memory is bound to
+  uint64 host_address = 3;


I would prefer mapped_address rather than host_address since the host address doesn't have to necessarily be fixed for the entire applications lifetime (The driver is free to return different addresses for different mappings).

AWoloszyn · 2018-06-06T15:46:21Z

gapis/service/service.proto

+}
+
+// A buffer/image that is bound to part of a memory allocation
+message MemoryBinding {


Before we finalize this API we should figure out how this should look for sparse binding, since a single image can be bound to multiple locations.

This might mean another binding type of 'SparseImageBlock', which will define
x, y, w, h, mip_level, array_layer
Then we would also need SparseImageMetadata and SparseImageMipTail
Lastly we would need SparseBufferBlock

We also need some way to describe opaque sparse image binding blocks. Could probably do that by setting x, y, w, h = (~0, ~0, 0, 0).

SparseImageBlock also needs an aspect flags field

Hmm, I think it's actually better to have a separate type for opaque image block bindings

sean-purcell · 2018-06-07T19:07:17Z

Added proto definition for sparse types, implementation isn't done yet

AWoloszyn · 2018-06-08T14:54:28Z

gapis/api/service.proto

+  uint32 array_layer = 6;
+
+  // The aspect(s) of the image this block is from (colour, depth, etc.)
+  uint32 aspects = 7;


Can we use something other than a uint32_t here. Preferably a repeated field so we can have multiple aspects that have a proper name.

Something like repeated string or do we still want to keep the values? repeated Aspect; message Aspect { uint32 value; string name; }

I think? this will work (we might be using an old version of proto but

enum AspectType {
COLOR,
DEPTH,
STENCIL
}

repeated AspectType

COLOR, DEPTH, STENCIL are pretty standard across all graphics APIs so I dont mind having them in the proto.

Alright, metadata is technically also a vulkan aspect but I suppose it shouldn't show up except in SparseImageMetadataMipTail, where we've already distinguished it from regular mip tail.

Sounds reasonable.

AWoloszyn · 2018-06-08T14:57:52Z

gapis/api/service.proto

+
+  // Which type of object is using this memory, and any extra associated data
+  // with that.
+  oneof type {


I am not sure if we can do this in Proto, but for consistency (and forward compatibility) can we make a BufferBinding and ImageBinding message, even if they are empty.

AWoloszyn

Only a couple minor nits, other than that looks good.

AWoloszyn · 2018-06-12T12:51:37Z

Thanks!

- Endpoint allows for different types of metrics to be requested at a certain command index - Only memory layout metrics are supported for now

Prints reported data out as well as computes memory aliasing and displays aliased regions

- Dumps info on allocations, mappings, and image/buffer/sparse resource bindings

sean-purcell · 2018-06-12T15:10:51Z

Example outputs of gapit memory:

$ gapit memory -at 382 alias.gfxtrace
4 memory allocations
Name: 37942576
        Device:       38013824
        Memory Type:  9
        Size:         256
        Flags:
                VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT
                VK_MEMORY_PROPERTY_HOST_COHERENT_BIT
        1 bindings:
        Buffer: 42453424
                Offset: 0
                Size:   192
        No aliased regions
Name: 42427504
        Device:       38013824
        Memory Type:  9
        Size:         2048
        Flags:
                VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT
                VK_MEMORY_PROPERTY_HOST_COHERENT_BIT
        3 bindings:
        Buffer: 42600560
                Offset: 0
                Size:   1024
        Buffer: 42599920
                Offset: 256
                Size:   512
        Buffer: 42600208
                Offset: 512
                Size:   512
        3 aliased regions:
        0:
                Offset:  256
                Size:    256
                Shared by:
                        42599920
                        42600560
        1:
                Offset:  512
                Size:    256
                Shared by:
                        42599920
                        42600208
                        42600560
        2:
                Offset:  768
                Size:    256
                Shared by:
                        42600208
                        42600560
Name: 42454400
        Device:       38013824
        Memory Type:  7
        Size:         256
        Flags:
                VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
        1 bindings:
        Buffer: 42416976
                Offset: 0
                Size:   80
        No aliased regions
Name: 42565280
        Device:       38013824
        Memory Type:  7
        Size:         256
        Flags:
                VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
        1 bindings:
        Buffer: 37943008
                Offset: 0
                Size:   12
        No aliased regions

$ gapit memory -at 500 sparse.gfxtrace
10 memory allocations
Name: 94322983582016
	Device:       94322983530432
	Memory Type:  9
	Size:         256
	Flags:
		VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT
		VK_MEMORY_PROPERTY_HOST_COHERENT_BIT
	1 bindings:
	Buffer: 94322983621376
		Offset: 0
		Size:   148
	No aliased regions
Name: 94322983608800
	Device:       94322983530432
	Memory Type:  9
	Size:         256
	Flags:
		VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT
		VK_MEMORY_PROPERTY_HOST_COHERENT_BIT
	Mapped into host memory at 0x7f5c27549c00
		Offset: 0
		Size:   256
	1 bindings:
	Buffer: 94322983608384
		Offset: 0
		Size:   66
	No aliased regions
Name: 94322983634352
	Device:       94322983530432
	Memory Type:  9
	Size:         512
	Flags:
		VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT
		VK_MEMORY_PROPERTY_HOST_COHERENT_BIT
	Mapped into host memory at 0x7f5c27549e00
		Offset: 0
		Size:   512
	1 bindings:
	Buffer: 94322988450096
		Offset: 0
		Size:   280
	No aliased regions
Name: 94322988016288
	Device:       94322983530432
	Memory Type:  7
	Size:         8102784
	Flags:
		VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
	1 bindings:
	Image: 94322988011232
		Offset: 0
		Size:   8102784
	No aliased regions
Name: 94322988084640
	Device:       94322983530432
	Memory Type:  7
	Size:         131072
	Flags:
		VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
	1 bindings:
	Image: 94322988090272
		Offset: 0
		Size:   131072
	No aliased regions
Name: 94322988408688
	Device:       94322983530432
	Memory Type:  7
	Size:         351744
	Flags:
		VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
	1 bindings:
	Image: 94322983617152
		Offset: 0
		Size:   351744
	No aliased regions
Name: 94322988424912
	Device:       94322983530432
	Memory Type:  7
	Size:         65536
	Flags:
		VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
	1 bindings:
	Sparse Image Mip Tail: 94322983582384
		Offset: 0
		Size:   65536
		Array Layer:     0
		Mip Tail Offset: 0
		Aspects:      Color
	No aliased regions
Name: 94322988429376
	Device:       94322983530432
	Memory Type:  7
	Size:         65536
	Flags:
		VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
	1 bindings:
	Sparse Image Block: 94322983582384
		Offset: 0
		Size:   65536
		Block Offset: (0, 0)
		Block Extent: (128, 128)
		Mip Level:    6
		Array Layer:  0
		Aspects:      Color
	No aliased regions
Name: 94322988432800
	Device:       94322983530432
	Memory Type:  7
	Size:         2097152
	Flags:
		VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
	1 bindings:
	Buffer: 94322988432448
		Offset: 0
		Size:   2097152
	No aliased regions
Name: 94322988436272
	Device:       94322983530432
	Memory Type:  7
	Size:         387328
	Flags:
		VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT
	1 bindings:
	Buffer: 94322988435920
		Offset: 0
		Size:   387096
	No aliased regions

ben-clayton

Please feel free to follow up these late review comments in later CLs.

ben-clayton · 2018-06-12T15:27:46Z

gapis/api/service.proto

 }
+
+message Metrics {
+  oneof metrics {


Instead of having this as a oneof, I'd suggest having memory_breakdown as a regular field.
Then instead of having an enum for the single metrics you want, the interface can be a list of bool flags for the bits of info the client wants. That way the client can fetch everything it cares about in a single RPC.

ben-clayton · 2018-06-12T15:30:52Z

gapis/api/service.proto

+  uint32 memory_type = 2;
+  // Flags describing properties of the allocation, e.g. whether it's device
+  // local, whether it's host visible, etc.
+  uint32 flags = 3;


Part of the goal with this RPC interface is that the client should never have to have intimate knowledge of the actual graphics API. This breaks the abstraction.
I can see two ways of solving this:

Have a repeated string field of API flags as names.

Making another message with fields like IsHostVisible, etc. However this makes it hard to reproduce the true vulkan names.

It was originally a repeated string field of API flag names, but @AWoloszyn suggested using uint32 along with the allocation_flags_index field to fetch the flag names from ConstantSets

Ah okay, please can you document that the flags relate to bitfields in the constant sets then?
This was not obvious.

Good point, sure.

ben-clayton · 2018-06-12T15:34:12Z

cmd/gapit/memory.go

+					return ""
+				}
+				strs := make([]string, len(aspects))
+				for i, asp := range aspects {


You can add a Format() method on the type so that this automatically handled by the fmt package.
There are many examples of this here.

ben-clayton · 2018-06-12T15:37:15Z

cmd/gapit/memory.go

+				fmt.Printf("\t\tBlock Extent: (%v, %v)\n",
+					info.Width, info.Height)
+				fmt.Printf("\t\tMip Level:    %v\n", info.MipLevel)
+				fmt.Printf("\t\tArray Layer:  %v\n", info.ArrayLayer)


Nit: There is the text/tabwriter package that does automatic alignment of text into columns. It also deals with emitting whitespace instead of tab characters (which many terminals will struggle with).

ben-clayton · 2018-06-12T15:43:18Z

gapis/api/service.proto

+  // Flags describing properties of the allocation, e.g. whether it's device
+  // local, whether it's host visible, etc.
+  uint32 flags = 3;
+  // Constant index for looking up the flag values with the path.ConstantSet endpoint


Of course, I now see it is mentioned here. I of course didn't look here. :)

Although now that you point this out I think this field is duplicating the work of allocation_flags_index in MemoryBreakdown. That should be fixed.

@ben-clayton

Recommended changes by @ben-clayton in google#1954 - Change Metrics proto to not be oneof, and change path.Metrics to have a flag per type instead of an enum - Remove redudancy on flags ConstantSet index, and document better - Add aspectList type with Format method - Use text/tabwriter in cmd/gapit/memory

@ben-clayton

Recommended changes by @ben-clayton in #1954 - Change Metrics proto to not be oneof, and change path.Metrics to have a flag per type instead of an enum - Remove redudancy on flags ConstantSet index, and document better - Add aspectList type with Format method - Use text/tabwriter in cmd/gapit/memory

sean-purcell commented Jun 4, 2018

View reviewed changes

apazylbe reviewed Jun 4, 2018

View reviewed changes

qining reviewed Jun 4, 2018

View reviewed changes

AWoloszyn reviewed Jun 6, 2018

View reviewed changes

sean-purcell force-pushed the vulkan-memory-breakdown branch from 8b87ec5 to 21390c6 Compare June 7, 2018 19:06

AWoloszyn reviewed Jun 8, 2018

View reviewed changes

AWoloszyn approved these changes Jun 12, 2018

View reviewed changes

sean-purcell added 4 commits June 12, 2018 10:14

Add extra to query and store VkPhysicalDeviceMemoryProperties

c47dba9

Add metrics endpoint to gapis/service

15596c2

- Endpoint allows for different types of metrics to be requested at a certain command index - Only memory layout metrics are supported for now

Add gapit memory, verb to get memory breakdown from a trace

60f6a07

Prints reported data out as well as computes memory aliasing and displays aliased regions

Add support for memory layout metrics to vulkan API

b8a7c30

- Dumps info on allocations, mappings, and image/buffer/sparse resource bindings

sean-purcell force-pushed the vulkan-memory-breakdown branch from af84cda to b8a7c30 Compare June 12, 2018 14:33

ben-clayton reviewed Jun 12, 2018

View reviewed changes

AWoloszyn merged commit dceb3e2 into google:master Jun 12, 2018

sean-purcell deleted the vulkan-memory-breakdown branch June 15, 2018 17:21

sean-purcell mentioned this pull request Jun 15, 2018

Small fixes to memory breakdown #1989

Merged


		}

		// FIXME DO NOT MERGE: Not sure where the best spot for this is, it can't go in gapis/api because api can't depend on service


		type bindingSlice []*service.MemoryBinding

		func (bindings bindingSlice) BindingLess(i, j int) bool {

Comments

Conversation

sean-purcell commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sean-purcell commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWoloszyn left a comment

Choose a reason for hiding this comment