Remove pseudo-commands for mid-execution capture. #1527

AWoloszyn · 2018-01-02T19:18:34Z

Instead of serializing pseudo-commands for mid-execution capture "eg: RecreateInstance", serialize the state block as-is.

Only re-create the state block when we need to on the server side.

TODO in a future MR:

Remove the cases where we generate the commands when not necessary
1. Generating the dependency graph
1. Resolving resources

Qining · 2018-01-04T21:40:42Z

cmd/linearize_trace/CMakeBuild.cmake

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+go_install()


should this be go_installl(DESTINATION ${TARGET_INSTALL_PATH})?

Qining · 2018-01-04T21:59:16Z

cmd/linearize_trace/main.go

+	}
+
+	initialCmds := capt.GetInitialCommands(ctx)
+	_ = initialCmds


Qining · 2018-01-04T22:03:28Z

cmd/linearize_trace/main.go

+	if err != nil {
+		return err
+	}
+


defer f.Close() ?

Qining · 2018-01-04T22:07:07Z

gapii/cc/chunk_writer.cpp

@@ -69,7 +69,9 @@ bool ChunkWriterImpl::write(std::string& s) {
 }

 void ChunkWriterImpl::flush() {
-    mStreamGood = mWriter->write(mBuffer.data(), mBuffer.size());
+    size_t bufferSize = mBuffer.size();
+    // If we flush en empty buffer, this will return false, which is incorrect.


I guess this comment is describing the stale code? Probably not necessary or should be changed for the new code here?

Good question - Andrew?

That is correct, removed the comment.

Qining · 2018-01-08T16:12:44Z

gapii/cc/spy_base.h

@@ -48,6 +48,7 @@
 #include <unordered_map>

 namespace gapii {
+const uint8_t kAllAPIs = 0xFF;


Probably we can change the getEncoder to make is accept a bit mask, instead the shift value, so we can avoid using this 'mask' like thing with the 'shift value' like thing in should_trace()? Hmm, probably not in this CL anyway.

Yeah, I would prefer to leave this for a future CL.

Qining · 2018-01-08T19:34:43Z

gapis/api/vulkan/command_buffer_rebuilder.go

@@ -927,13 +1069,12 @@ func rebuildVkCmdDebugMarkerInsertEXT(
 			markerNameData.Free()
 			markerInfoData.Free()
 		}, cb.VkCmdDebugMarkerInsertEXT(commandBuffer, markerInfoData.Ptr()).AddRead(
-			markerNameData.Data()).AddRead(markerInfoData.Data())
+			markerNameData.Data()).AddRead(markerInfoData.Data()), nil
 }

 func GetCommandArgs(ctx context.Context,


Probably we should add comment for it?

Qining · 2018-01-08T19:39:47Z

gapis/api/vulkan/externs.go

@@ -79,6 +80,10 @@ func (e externs) callSub(ctx context.Context, cmd api.Cmd, id api.CmdID, s *api.
 	})
 }

+func (e externs) callSub(ctx context.Context, cmd api.Cmd, id api.CmdID, s *api.GlobalState, b *rb.Builder, sub, data interface{}) {


Drop it? It is only used once.

Qining · 2018-01-08T19:40:02Z

gapis/api/vulkan/command_buffer_rebuilder.go

@@ -1035,6 +1176,108 @@ func GetCommandArgs(ctx context.Context,
 	}
 }

+func GetCommandFunction(cr CommandReference) interface{} {


Qining · 2018-01-08T20:02:03Z

gapis/api/vulkan/state_rebuilder.go

+
+// TODO: wherever possible, use old resources instead of doing full reads on the old pools.
+//       This is especially useful for things that are internal pools, (Shader words for example)
+func (s *State) RebuildState(ctx context.Context, oldState *api.GlobalState) []api.Cmd {


Qining · 2018-01-08T20:14:20Z

gapis/api/vulkan/state_rebuilder.go

+		if numHandled == 0 {
+			// There is a cycle in the basePipeline indices.
+			// Or the no base pipelines does exist.
+			// Create the rest without base pipelines


Is it allowed to by cyclical? Probably we should emit an error in the report view for this?

I am not sure if it is technically allowed, but I want to at least continue to work if it is.
Hopefully the validation layers can catch this.

Qining · 2018-01-08T20:14:58Z

gapis/api/vulkan/state_rebuilder.go

+	}
+
+	numHandled := 0
+	for len(unhandledPipelines) != 0 {


Looks like not optimal? Anyway, we can optimize it later if it matters.

Qining

Just had a fast read, Still reading the details of the code...

Qining · 2018-01-08T20:35:49Z

gapis/api/vulkan/state_rebuilder.go

+			VkMemoryAllocateInfo{
+				VkStructureType_VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO,
+				NewVoidᶜᵖ(memory.Nullptr),
+				size * 2, // Overallocate by a factor of 2.


Any reason for the overallocate?

Discussed offline.

Would be nice to be documented in the code. 😉

Qining · 2018-01-08T20:39:38Z

gapis/api/vulkan/state_rebuilder.go

+func (sb *stateBuilder) getSparseQueueFor(lastBoundQueue *QueueObject, device VkDevice, queueFamilyIndices *map[uint32]uint32) *QueueObject {
+	hasQueueFamilyIndices := queueFamilyIndices != nil
+
+	queueProperties := sb.s.PhysicalDevices.Get(sb.s.Devices.Get(lastBoundQueue.Device).PhysicalDevice).QueueFamilyProperties


I saw lastBoundQueue != nil in the next line. So if it can be nil here, lastBoundQueue.Device will crash.

Qining · 2018-01-16T16:35:32Z

gapis/api/vulkan/state_rebuilder.go

+			size,
+		}).Ptr(),
+		VkResult_VK_SUCCESS,
+	).AddWrite(dat.Data()))


This should be AddRead(dat.Data())

ben-clayton

Please address comments in follow up CLs once bazel branch is merged.

ben-clayton · 2018-02-05T18:40:51Z

gapii/cc/chunk_writer.cpp

@@ -69,7 +69,9 @@ bool ChunkWriterImpl::write(std::string& s) {
 }

 void ChunkWriterImpl::flush() {
-    mStreamGood = mWriter->write(mBuffer.data(), mBuffer.size());
+    size_t bufferSize = mBuffer.size();
+    // If we flush en empty buffer, this will return false, which is incorrect.


Good question - Andrew?

ben-clayton · 2018-02-05T18:41:50Z

gapii/cc/pool.h

@@ -25,6 +25,9 @@ namespace gapii {
 class Pool {
 public:
    static std::shared_ptr<Pool> create(uint32_t id, uint64_t size);
+// This creates a pool that can be serialized, but has no actual


Nit: Indent?

ben-clayton · 2018-02-05T18:43:08Z

gapii/cc/slice.h

@@ -90,7 +90,10 @@ inline Slice<T>::Slice() : mBase(nullptr), mCount(0) {}
 template<typename T>
 inline Slice<T>::Slice(T* base, uint64_t count, const std::shared_ptr<Pool>& pool)
    : mBase(base), mCount(count), mPool(pool) {
-    GAPID_ASSERT(mBase != nullptr || count == 0 /* Slice: null pointer */);
+
+    if (!mPool || !mPool->is_virtual()) {


When is mPool null?

IIRC mPool is nullptr, if this is the application pool. I might be wrong, but I think that is the reasoning for this logic.

ben-clayton · 2018-02-05T18:47:28Z

gapii/cc/spy.cpp

@@ -483,14 +510,14 @@ void Spy::onPostFrameBoundary(bool isStartOfFrame) {
            exit();
            set_suspended(false);
            saveInitialSate();
-            set_recording_state(true);
+            //set_recording_state(true);


That should all be cleaned up and gone now. Not sure why it shows in this CR.

ben-clayton · 2018-02-05T19:03:14Z

gapis/api/vulkan/state_rebuilder.go

+			// There is a cycle in the basePipeline indices.
+			// Or the no base pipelines does exist.
+			// Create the rest without base pipelines
+			for k, _ := range unhandledPipelines {


Superdooperübernit:

for k := range unhandledPipelines {

Is more idiomatic

ben-clayton · 2018-02-05T19:32:38Z

gapis/api/vulkan/state_rebuilder.go

+			VkMemoryAllocateInfo{
+				VkStructureType_VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO,
+				NewVoidᶜᵖ(memory.Nullptr),
+				size * 2, // Overallocate by a factor of 2.


Would be nice to be documented in the code. 😉

ben-clayton · 2018-02-05T19:32:49Z

gapis/api/vulkan/state_rebuilder.go

+			size,
+		}).Ptr(),
+		VkResult_VK_SUCCESS,
+	).AddWrite(dat.Data()))


ben-clayton · 2018-02-05T19:33:02Z

gapis/api/vulkan/state_rebuilder.go

+func (sb *stateBuilder) getSparseQueueFor(lastBoundQueue *QueueObject, device VkDevice, queueFamilyIndices *map[uint32]uint32) *QueueObject {
+	hasQueueFamilyIndices := queueFamilyIndices != nil
+
+	queueProperties := sb.s.PhysicalDevices.Get(sb.s.Devices.Get(lastBoundQueue.Device).PhysicalDevice).QueueFamilyProperties


ben-clayton · 2018-02-05T19:34:25Z

gapis/api/vulkan/vulkan.go

-func (*State) SetupInitialState(ctx context.Context) {}
-
-func (*State) RebuildState(ctx context.Context, s *api.GlobalState) []api.Cmd { return nil }
+func (c *State) SetupInitialState(ctx context.Context) {


Docs please

Instead of passing across synthetic commands, pass across the entire state block. This allows us to more efficiently handle the state block on the server side.

We were accidentally removing all buffer descriptor sets. This was causing replays to fail.

This re-writes the capture so that the state block at the start is replaced with the commands needed to generate the state block.

This will create a virtual command when a subcommand is requested.

Now that we serialize state, we have to let the early-terminator know that is it running with extra commands. This is so that the sync data matches the actual command list.

This will instruct the allocator to not allocate over the memory ranges needed by the initial commands.

This fixes textures in the UI.

Also work around a validation warning in read_framebuffer.go

When we re-create pools we do it through the observations. Therefore add a single 0-byte observation to each pool.

Also, recover the inheritance info of command buffers and bring back the missing layout transition for images that have mutiple samples or not-color aspect.

When the queue submission does not contain any commands, the signaled semaphore label still should be written. And for the wait semaphores, the submission will unsignal the semaphore, so it is a modify to the label, not just a read.

vkQueueSubmit more correct.

non-dedicately allocated handle.

…amically

AWoloszyn requested review from Qining and ben-clayton January 2, 2018 19:19

AWoloszyn force-pushed the move-mec branch from d689db1 to 29ed96e Compare January 3, 2018 15:12

Qining reviewed Jan 5, 2018

View reviewed changes

Qining reviewed Jan 8, 2018

View reviewed changes

AWoloszyn force-pushed the move-mec branch from 29ed96e to 8c2154b Compare January 10, 2018 14:42

Qining reviewed Jan 16, 2018

View reviewed changes

AWoloszyn force-pushed the move-mec branch from 80fb4a5 to afe4902 Compare January 16, 2018 18:13

AWoloszyn changed the title ~~WIP: Remove pseudo-commands for mid-execution capture.~~ Remove pseudo-commands for mid-execution capture. Jan 30, 2018

ben-clayton approved these changes Feb 5, 2018

View reviewed changes

AWoloszyn added 13 commits February 5, 2018 14:46

Serialize/deserialize state for Mid-Execution capture.

3d9a452

Instead of passing across synthetic commands, pass across the entire state block. This allows us to more efficiently handle the state block on the server side.

Updated replay/framebuffer observations to handle the new DCE.

eb454ea

Fixed the re-creation of descriptor sets.

22a0c10

We were accidentally removing all buffer descriptor sets. This was causing replays to fail.

A couple of cleanups for spy

b20aa2f

Fixed dead-code eliminiation with the new state serialization.

1db158a

Removed unused function in the vulkan.api file.

9617416

Added command-line tool to linearize a mid-execution capture.

c988c36

This re-writes the capture so that the state block at the start is replaced with the commands needed to generate the state block.

Fix the command-tree for commands that are recorded pre-MEC.

a996347

This will create a virtual command when a subcommand is requested.

Let the vulkan early-terminator understand about MEC.

eb8c499

Now that we serialize state, we have to let the early-terminator know that is it running with extra commands. This is so that the sync data matches the actual command list.

temp

cece40b

Reserve the memory ranges needed by the initial commands.

d69172a

This will instruct the allocator to not allocate over the memory ranges needed by the initial commands.

Remove some no longer needed code

bbd73ba

Get the fence values from the driver before we serialize state.

1016393

AWoloszyn and others added 16 commits February 5, 2018 14:46

Fix image offset calculations.

6f4306c

This fixes textures in the UI.

Format vulkan_mid_execution.cpp

5ca50b2

Actually track coherent memory in the new mid-execution capture.

bcd8419

Also work around a validation warning in read_framebuffer.go

Cache generated commands, and fix a bug with pools.

874b6cd

When we re-create pools we do it through the observations. Therefore add a single 0-byte observation to each pool.

update cmakeproto.cmake to fix build

3b5c2c9

read flush memory command data, instead of write

c163ae4

Fix missing semaphore signal

46f17e7

Also, recover the inheritance info of command buffers and bring back the missing layout transition for images that have mutiple samples or not-color aspect.

Removed unistd, which isn't on windows.

ba0ad0c

Handle the semaphore/fence signal/unsigal operation tirgger by

21567af

vkQueueSubmit more correct.

Work around app bug that dedicatedly allocated handle bind to

d05cea8

non-dedicately allocated handle.

Remove some error msg in the log view

cb75983

Skip creating descriptorsets whose pools are not valid

b279130

Add some comments and minor fixes

ba00f00

Truly delete descriptor sets when destroying descriptor pools

60e913e

ViewportCount and ScissorCount should be set even viewport is set dyn…

6137e46

…amically

AWoloszyn force-pushed the move-mec branch from 9280461 to efe51a5 Compare February 5, 2018 19:58

Update from some comments

b2d72ac

AWoloszyn force-pushed the move-mec branch from efe51a5 to b2d72ac Compare February 5, 2018 19:59

AWoloszyn merged commit 8d3d93d into google:master Feb 5, 2018

AWoloszyn deleted the move-mec branch February 5, 2018 20:01

Remove pseudo-commands for mid-execution capture. #1527

Remove pseudo-commands for mid-execution capture. #1527

Conversation

AWoloszyn commented Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qining left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-clayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AWoloszyn commented Jan 2, 2018 •

edited

Loading