[mtl] Borrowed commands #2164

kvark · 2018-06-21T01:49:37Z

PR checklist:

make succeeds (on *nix)
make reftests succeeds
tested examples with the following backends:

r? @gfx-rs/metallists

This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass).

My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code.

kvark · 2018-06-21T03:45:31Z

It might be the power management playing tricks with me. When plugged in (as opposed to 50% battery), I see stable 60-70fps. Just in case, I double-checked the version before my PR, and it doesn't appear faster. It's roughly the same (which is also kinda concerning), maybe a few frames per second less. With this in mind, removing the WIP tag, open for reviews.

grovesNL · 2018-06-22T00:14:17Z

src/backend/metal/src/command.rs

-        if aspects.intersects(Aspects::DEPTH | Aspects::STENCIL) {
-            commands.extend(com.map(soft::RenderCommand::SetDepthStencilDesc));
-        }
+        let render_resources = Some(&self.resources_vs).into_iter().chain(Some(&self.resources_fs));


Why not just iter::once instead of the Option here?

grovesNL · 2018-06-22T00:24:49Z

src/backend/metal/src/command.rs

+        let com_buffers = RENDER_STAGES
+            .iter()
+            .zip(render_resources.clone())
+            .flat_map(|(&stage, resources)| {


I would guess there is some extra overhead caused by repeating these identical RENDER_STAGES.iter().zip().flat_map(). Could we chain inside instead?

grovesNL · 2018-06-22T00:38:39Z

src/backend/metal/src/command.rs

+            width: vp.rect.w as _,
+            height: vp.rect.h as _,
+            znear: vp.depth.start as _,
+            zfar: if disabilities.broken_viewport_near_depth {


I wonder if this has anything to do with the z-fighting?

you can check it locally, I suppose, since this flag is only enabled on Intel, and you got AMD IIRC

I'm on Intel HD Graphics 6000 1536 MB

grovesNL · 2018-06-22T00:40:11Z

src/backend/metal/src/command.rs

-        );
+    }
+
+    fn set_stencil_mask_values<'a>(


The GitHub diff falls apart for a lot of these functions – let me know if there are any significant changes here, for now I'll assume the code has just been relocated

grovesNL · 2018-06-22T01:07:41Z

src/backend/metal/src/command.rs

            match stage {
                pso::Stage::Vertex =>
-                    encoder.set_vertex_bytes(index as _, bytes.len() as _, bytes.as_ptr() as _),
+                    encoder.set_vertex_bytes(index as _, words.len() as u64 * 4, words.as_ptr() as _),


nit: 4 -> WORD_SIZE or similar? We use this a few times

grovesNL · 2018-06-22T01:37:43Z

src/backend/metal/src/soft.rs

        offset: hal::buffer::Offset,
    },
    BindBufferData {
        stage: hal::pso::Stage,
        index: usize,
-        bytes: Vec<u8>,
+        words: R::Data,


In general I don't think 4-byte alignment is necessary in most places for iOS and tvOS, so we may want to reconsider eventually, or set a WORD_SIZE based on the OS

grovesNL · 2018-06-22T01:44:39Z

src/backend/metal/src/command.rs

-                            mem::size_of::<u32>()
-                        ).to_owned()
+                            &(r.size as u32) as *const u32,
+                            1,


nit: mem::size_of::<u32>() / WORD_SIZE if we ever change this (as mentioned in other comment)

grovesNL · 2018-06-22T01:50:59Z

src/backend/metal/src/command.rs

-                .borrow_mut()
-                .sink()
-                .pre_render_commands(commands);
+            if stages.contains(pso::ShaderStageFlags::VERTEX) {


The old approach avoids two calls to pre_render_commands here – not sure how the Option compares to this trade-off

I had to go through quite a few hoops with the borrow checker...

grovesNL · 2018-06-22T01:53:57Z

src/backend/metal/src/internal.rs

+}
+
+
+//#[derive(Clone)]


nit: comment and extra newline

grovesNL · 2018-06-22T01:55:13Z

src/backend/metal/src/native.rs

    Texture(&'a metal::TextureRef),
    Frame {
        swapchain: RwLockReadGuard<'a, SwapchainInner>,
-        index: hal::FrameImage,
+        index: hal::FrameImage


nit: comma removed

kvark · 2018-06-22T03:43:58Z

Thank you for a detailed review! ❤️
Concerns are addressed, let's proceed.
bors r=grovesNL

2164: [mtl] Borrowed commands r=grovesNL a=kvark PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: r? @gfx-rs/metallists This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass). My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code. Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2018-06-22T04:13:12Z

Build failed

continuous-integration/travis-ci/push

grovesNL

CI failure is unrelated:

info: downloading component 'rustc'
error: failed to parse url: 1.28.0-nightly (662c70a59 2018-06-21)/2018-06-22/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz
info: caused by: relative URL without a base
The command "curl -sSf https://sh.rustup.rs | sh -s -- --default-toolchain=$TRAVIS_RUST_VERSION -y" failed and exited with 1 during .

kvark · 2018-06-22T11:13:46Z

Bors retry

…

On Jun 22, 2018, at 00:20, Josh Groves ***@***.***> wrote: @grovesNL approved this pull request. CI failure is unrelated: info: downloading component 'rustc' error: failed to parse url: 1.28.0-nightly (662c70a59 2018-06-21)/2018-06-22/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz info: caused by: relative URL without a base The command "curl -sSf https://sh.rustup.rs | sh -s -- --default-toolchain=$TRAVIS_RUST_VERSION -y" failed and exited with 1 during . — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

2164: [mtl] Borrowed commands r=grovesNL a=kvark PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: r? @gfx-rs/metallists This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass). My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code. Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2018-06-22T11:29:10Z

Build failed

continuous-integration/travis-ci/push

kvark · 2018-06-22T17:53:49Z

bors retry

kvark · 2018-06-22T17:54:10Z

bors r=grovesNL

bors · 2018-06-22T17:54:11Z

Not awaiting review

2071: Remaping descriptor sets in the gl backend r=kvark a=ZeGentzy I'll rebase off master when I'm done. Uniforms in gl only have a bindings field, not a set one. This means that for shaders that use multiple sets to work, we must change where we are binding them. See page 14 for what I mean: https://www.khronos.org/assets/uploads/developers/library/2016-vulkan-devday-uk/4-Using-spir-v-with-spirv-cross.pdf PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [ ] tested examples with the following backends: 2164: [mtl] Borrowed commands r=grovesNL a=kvark PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: r? @gfx-rs/metallists This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass). My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code. 2166: Update example instructions in README.md r=kvark a=king6cong Co-authored-by: Hal Gentz <[email protected]> Co-authored-by: Dzmitry Malyshau <[email protected]> Co-authored-by: king6cong <[email protected]>

bors · 2018-06-22T18:24:20Z

Build failed (retrying...)

continuous-integration/travis-ci/push

2164: [mtl] Borrowed commands r=grovesNL a=kvark PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: r? @gfx-rs/metallists This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass). My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code. 2166: Update example instructions in README.md r=kvark a=king6cong Co-authored-by: Dzmitry Malyshau <[email protected]> Co-authored-by: king6cong <[email protected]>

bors · 2018-06-22T18:57:22Z

Build failed (retrying...)

continuous-integration/travis-ci/push

2164: [mtl] Borrowed commands r=grovesNL a=kvark PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: r? @gfx-rs/metallists This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass). My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code. Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2018-06-22T19:15:28Z

Build failed

continuous-integration/travis-ci/push

kvark · 2018-06-23T00:17:58Z

Bors r=grovesNL

…

On Jun 22, 2018, at 15:15, bors[bot] ***@***.***> wrote: Build failed continuous-integration/travis-ci/push — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

kvark · 2018-06-23T00:28:04Z

Bors retry

…

On Jun 22, 2018, at 15:15, bors[bot] ***@***.***> wrote: Build failed continuous-integration/travis-ci/push — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

bors · 2018-06-23T00:28:05Z

Not awaiting review

2164: [mtl] Borrowed commands r=grovesNL a=kvark PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: r? @gfx-rs/metallists This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass). My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code. Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2018-06-23T00:52:06Z

Build failed

continuous-integration/travis-ci/push

kvark · 2018-06-23T02:44:29Z

bors retry

kvark · 2018-06-23T02:44:44Z

bors r=grovesNL

bors · 2018-06-23T02:44:45Z

Not awaiting review

2164: [mtl] Borrowed commands r=grovesNL a=kvark PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: r? @gfx-rs/metallists This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass). My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code. Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2018-06-23T03:13:45Z

Build succeeded

2169: Persistent Metal swapchain r=grovesNL a=kvark Includes #2164 Fixes #2168 Fixes #2143 Removes deferred image resolution (yay!), and also renames some of the outdated concepts. We have no flickering, and proper frame sync now. PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: Co-authored-by: Dzmitry Malyshau <[email protected]>

kvark changed the title ~~[wip][mtl] Borrowed commands~~ [mtl] Borrowed commands Jun 21, 2018

kvark mentioned this pull request Jun 21, 2018

Persistent Metal swapchain #2169

Merged

3 tasks

grovesNL reviewed Jun 22, 2018

View reviewed changes

[mtl] Borrowed commands

90fd193

kvark force-pushed the mtl-copy branch from b0e954b to 90fd193 Compare June 22, 2018 03:43

grovesNL approved these changes Jun 22, 2018

View reviewed changes

bors bot merged commit 90fd193 into gfx-rs:master Jun 23, 2018

kvark deleted the mtl-copy branch June 23, 2018 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mtl] Borrowed commands #2164

[mtl] Borrowed commands #2164

kvark commented Jun 21, 2018

kvark commented Jun 21, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

kvark Jun 22, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

kvark Jun 22, 2018

grovesNL Jun 22, 2018

grovesNL Jun 22, 2018

kvark commented Jun 22, 2018

bors bot commented Jun 22, 2018

grovesNL left a comment

kvark commented Jun 22, 2018 via email

bors bot commented Jun 22, 2018

kvark commented Jun 22, 2018

kvark commented Jun 22, 2018

bors bot commented Jun 22, 2018

bors bot commented Jun 22, 2018

bors bot commented Jun 22, 2018

bors bot commented Jun 22, 2018

kvark commented Jun 23, 2018 via email

kvark commented Jun 23, 2018 via email

bors bot commented Jun 23, 2018

bors bot commented Jun 23, 2018

kvark commented Jun 23, 2018

kvark commented Jun 23, 2018

bors bot commented Jun 23, 2018

bors bot commented Jun 23, 2018

		}


		//#[derive(Clone)]

[mtl] Borrowed commands #2164

[mtl] Borrowed commands #2164

Conversation

kvark commented Jun 21, 2018

kvark commented Jun 21, 2018

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

kvark commented Jun 22, 2018

bors bot commented Jun 22, 2018

Build failed

grovesNL left a comment

Choose a reason for hiding this comment

kvark commented Jun 22, 2018 via email

bors bot commented Jun 22, 2018

Build failed

kvark commented Jun 22, 2018

kvark commented Jun 22, 2018

bors bot commented Jun 22, 2018

bors bot commented Jun 22, 2018

Build failed (retrying...)

bors bot commented Jun 22, 2018

Build failed (retrying...)

bors bot commented Jun 22, 2018

Build failed

kvark commented Jun 23, 2018 via email

kvark commented Jun 23, 2018 via email

bors bot commented Jun 23, 2018

bors bot commented Jun 23, 2018

Build failed

kvark commented Jun 23, 2018

kvark commented Jun 23, 2018

bors bot commented Jun 23, 2018

bors bot commented Jun 23, 2018

Build succeeded