Stand-alone passes #440

kvark · 2020-01-07T22:33:53Z

Fixes #438

kvark · 2020-01-08T19:44:51Z

Judging purely by the number of lines, you can see how there is something crazy going on here. The last commits integrates peek-poke for serializing the command stream of a (compute) pass.

The main driver for this (as opposed to an enum Command that has everything) is taking care of commands that have variable input count, such as set_vertex_buffers and set_bind_group. By serializing the commands, we can just append the extra data to the byte stream. This may also result in smaller data size for the recorded pass, and improved data cache locality for decoding it (less gaps).

The cost of this is more unsafe code, which is unfortunate. Note: WebRender also uses peek-poke, so it's not completely crazy for us either.

Another interesting bit is RawPass, which is a repr(C) structure (works somewhat well with raw nature of peek-poke). The idea is that Gecko C++ code would use it directly on the stack, while only wgpu-native would have to box it or put into a storage (to hide behind an immutable ID). According to the proposal in gfx-rs/wgpu-rs#156, wgpu-rs should also be able to place it directly, avoiding indirection on every access to the pass.

kvark · 2020-01-10T17:25:11Z

I'm seeing something that looks like a compiler bug when running the compute example (e.g. with make run-example-compute) from rev 5ae0623.

This is the offending piece:

    WGPURawComputePassId command_pass = //temp name
        wgpu_command_encoder_begin_compute_pass(encoder, NULL);

What I see is that wgpu_command_encoder_begin_compute_pass returns a proper 64 bit pointer. But it gets truncated to 32bits (see movslq instruction in the assembly screenshot) before assigning to command_pass (which is also 64 bit):

~~@jrmuizel any ideas what I'd be doing wrong here? I might be too quick to call it a compiler bug :)~~
Edit: this was tested on macOS (screenshot is from XCode) so far
Edit2: turns out there was a mashup between the header and the module, hence the broken assembly.

kvark · 2020-01-11T05:50:01Z

Hitting this in cbindgen (cc @emilio):

thread 'main' panicked at 'Unable to mangle generic parameter ConstPtr(Path(GenericPath { path: Path { name: "TextureViewId" }, export_name: "TextureViewId", generics: [], ctype: None })) for 'RenderPassColorAttachmentDescriptorBase'', /Users/dmalyshau/.cargo/registry/src/github.com-1ecc6299db9ec823/cbindgen-0.12.1/src/bindgen/mangle.rs:52:17

Will probably be able to work around, but still annoying...

kvark · 2020-01-11T14:47:25Z

Alright, at this stage raw compute and render passes work for the examples of this repo 🎉

emilio · 2020-01-11T14:54:48Z

If you have a test-case for that I'd be happy to take a look.

kvark · 2020-01-12T05:53:20Z

Tested on wgpu-rs examples (see gfx-rs/wgpu-rs#155), no regressions.
@grovesNL this is ready for merging, if you have time to review it.

I'm going to miss the immediate pass recording, but at this point it appears to me that having a single consistent code path for native and non-native is a bigger win. We can re-explore this direction later.

kvark · 2020-01-12T05:56:37Z

Interestingly, this change can also be seen as a first step towards de-storaging some of the objects. I.e. going from id of Storage<T> to just Box<T>. This makes sense for objects that are few and/or temporary, such as passes, command buffers, devices, but not for resources.

kvark · 2020-01-12T06:04:31Z

Found an issue - since we are soft recording with this PR, we aren't keeping the objects alive after they are used in the recording.

grovesNL · 2020-01-14T02:21:03Z

ffi/wgpu.h

                                         uint32_t start_slot,
-                                         const WGPUBufferId *buffers,
+                                         const WGPUBufferId *buffer_ids,
                                         const WGPUBufferAddress *offsets,
                                         uintptr_t length);


Not directly relevant to this PR but many of these uintptr_t are uint64_t in webgpu-headers

moved to #452

grovesNL · 2020-01-14T03:48:56Z

wgpu-core/src/command/compute.rs

-            trackers,
-        }
-    }
+#[derive(Clone, Copy, Debug, PeekCopy, Poke)]


Is this supposed to be repr(C)?

These don't need to be repr(C) because we never lay them out in memory that C code works with.

I thought peek_poke expects to operate on repr(C) types to have certain guarantees about the data layout, or at least the peek_poke examples seem to do that

hmm, I'll double check!

The webrender examples uses repr(C) because it was written for Gecko C++ code to see the types and copied verbatim into peek-poke.

That is to say, I consulted with @Gankra and it turns out there can be a slightly bigger chance of the compiler optimization (turning the struct poking into a memcopy) if it's repr(C). Still not a guarantee, still not a correctness concern, just a bit more likely.

So we should make these types repr(C) as you suggest, at least the Rect thing.

grovesNL · 2020-01-14T03:49:23Z

wgpu-core/src/command/render.rs

+}
+
+#[derive(Clone, Copy, Debug, PeekCopy, Poke)]
+enum RenderCommand {


Is this supposed to be repr(C)?

grovesNL · 2020-01-14T03:49:27Z

wgpu-core/src/command/render.rs

+}
+
+#[derive(Clone, Copy, Debug, PeekCopy, Poke)]
+pub struct Rect<T> {


Is this supposed to be repr(C)?

grovesNL · 2020-01-14T04:14:32Z

wgpu-core/src/command/render.rs

+        index: u8,
+        num_dynamic_offsets: u8,
+        bind_group_id: id::BindGroupId,
+        phantom_offsets: PhantomSlice<BufferAddress>,


I found it a bit confusing to add PhantomSlice to the end of these variants because I wasn't sure if they were markers for peek_poke to know to treat them specially somehow.

Instead of using this marker type, could we replace it by comments to indicate that there is some dynamically sized array at the end of these variants and have decode as a free function?

I think having at least some type-level semantic of the data following commands is helpful. having the decode strictly associated with a PhantomSlice makes it harder to fail when writing and maintaining this code. Comments wouldn't be as helpful.

I guess one interesting (future) option would be for peek_poke to provide a marker type to declare the size of variable length arrays, i.e.

#[derive(PeekCopy, Poke)] enum RenderCommand { SetBindGroup { index: u8, num_dynamic_offsets: u8, #[peek_poke(array_size = "num_dynamic_offsets")] dynamic_offsets: peek_poke::VariableLengthArray<BufferAddress>, }, /* ... */ }

kvark · 2020-01-14T15:19:59Z

@grovesNL thank you for having a look! I think we can proceed, no major concerns here.
bors r=me,grovesNL

440: Stand-alone passes r=me,grovesNL a=kvark Fixes #438 Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2020-01-14T15:56:11Z

Build succeeded

continuous-integration/travis-ci/push

kvark requested a review from grovesNL January 7, 2020 22:33

kvark changed the title ~~Stand-alone compute passes~~ Stand-alone passes Jan 8, 2020

kvark changed the title ~~Stand-alone passes~~ [WIP] Stand-alone passes Jan 8, 2020

kvark mentioned this pull request Jan 8, 2020

Standalone pass recording #438

Closed

kvark mentioned this pull request Jan 9, 2020

Refactor usage tracking to be truly sparse #439

Merged

kvark force-pushed the client-pass branch from 8504a94 to 0db2719 Compare January 10, 2020 02:34

kvark force-pushed the client-pass branch from 5ae0623 to 9464a6f Compare January 11, 2020 05:48

kvark force-pushed the client-pass branch from 9464a6f to 911180d Compare January 11, 2020 14:46

kvark mentioned this pull request Jan 12, 2020

Add a test for generic pointer parameter mozilla/cbindgen#453

Closed

kvark changed the title ~~[WIP] Stand-alone passes~~ Stand-alone passes Jan 12, 2020

kvark changed the title ~~Stand-alone passes~~ [WIP] Stand-alone passes Jan 12, 2020

kvark added 9 commits January 13, 2020 16:52

Stand-alone compute passes

7808a4d

Standalone render passes

20cd803

Use peek-poke for compute pass encoding

c01a7c6

Raw render pass encoding

941fcca

Switch the examples to use the new raw passes

40ac14e

Remove the old render and compute passes

e0574ee

Add missing render pass methods

f798f7c

Fix missing transitions before the render pass

92441c3

Switch to a custom fork of peek-poke

a510197

kvark force-pushed the client-pass branch from d1da970 to a510197 Compare January 13, 2020 21:53

kvark changed the title ~~[WIP] Stand-alone passes~~ Stand-alone passes Jan 14, 2020

grovesNL reviewed Jan 14, 2020

View reviewed changes

bors bot added a commit that referenced this pull request Jan 14, 2020

Merge #440

a72574a

440: Stand-alone passes r=me,grovesNL a=kvark Fixes #438 Co-authored-by: Dzmitry Malyshau <[email protected]>

bors bot merged commit a510197 into gfx-rs:master Jan 14, 2020

kvark mentioned this pull request Jan 15, 2020

Encode render target information into RawPass #454

Closed

kvark deleted the client-pass branch August 24, 2021 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stand-alone passes #440

Stand-alone passes #440

kvark commented Jan 7, 2020 •

edited

Loading

kvark commented Jan 8, 2020

kvark commented Jan 10, 2020 •

edited

Loading

kvark commented Jan 11, 2020

kvark commented Jan 11, 2020

emilio commented Jan 11, 2020

kvark commented Jan 12, 2020

kvark commented Jan 12, 2020

kvark commented Jan 12, 2020

grovesNL Jan 14, 2020

kvark Jan 14, 2020

grovesNL Jan 14, 2020

kvark Jan 14, 2020

grovesNL Jan 14, 2020

kvark Jan 14, 2020

kvark Jan 14, 2020

grovesNL Jan 14, 2020

grovesNL Jan 14, 2020

grovesNL Jan 14, 2020

kvark Jan 14, 2020

grovesNL Jan 14, 2020 •

edited

Loading

kvark commented Jan 14, 2020

bors bot commented Jan 14, 2020

Stand-alone passes #440

Stand-alone passes #440

Conversation

kvark commented Jan 7, 2020 • edited Loading

kvark commented Jan 8, 2020

kvark commented Jan 10, 2020 • edited Loading

kvark commented Jan 11, 2020

kvark commented Jan 11, 2020

emilio commented Jan 11, 2020

kvark commented Jan 12, 2020

kvark commented Jan 12, 2020

kvark commented Jan 12, 2020

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

grovesNL Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

kvark commented Jan 14, 2020

bors bot commented Jan 14, 2020

Build succeeded

kvark commented Jan 7, 2020 •

edited

Loading

kvark commented Jan 10, 2020 •

edited

Loading

grovesNL Jan 14, 2020 •

edited

Loading