Remote command sink in Metal #2260

kvark · 2018-07-21T03:50:21Z

Fixes #2259
The results so far are not super promising - highly unstable (presumably, because of the dispatch), with performance around Immediate mark. We are still missing the most important follow-up here - to avoid any heap allocations when recording commands. Currently, it just goes with Vec::new() and grows it for each pass, which shows up in the profile quite a bit.

The PR also has a bunch of stuff in general optimizations:

HAL change in the descriptor allocation API to avoid the heap
lighten up Metal descriptor binding path (a bit) by making sure there is enough state slots in advance
simplification and refactoring of CommandSink implementations

PR checklist:

make succeeds (on *nix)
make reftests succeeds
tested examples with the following backends: Metal
rustfmt run on changed code

grovesNL · 2018-07-21T05:46:03Z

src/backend/vulkan/src/native.rs

+            .map(|sets| {
+                output.extend(sets
+                    .into_iter()
+                    .zip(layout_bindinds)


The change isn't part of this PR, but layout_bindinds is a typo here

grovesNL · 2018-07-21T06:09:46Z

src/hal/src/pso/descriptor.rs

    }

    /// Allocate one or multiple descriptor sets from the pool.
    ///
    /// Each descriptor set will be allocated from the pool according to the corresponding set layout.
    /// Descriptors will become invalid once the pool is reset. Usage of invalidated descriptor sets results
    /// in undefined behavior.
-    fn allocate_sets<I>(&mut self, layouts: I) -> Vec<Result<B::DescriptorSet, AllocationError>>
+    fn allocate_sets<I>(&mut self, layouts: I, sets: &mut Vec<B::DescriptorSet>) -> Result<(), AllocationError>


IMO this signature could be confusing. For example, it's unclear from the signature whether the sets's existing contents are overridden or extended. Would it be possible to somehow return impl Iterator and let the caller decide what to do with the newly allocated sets? Then I guess we could save a Vec allocation in allocate_set too

I took a few attempts to make this API nicer before, and failed. impl Iterator still has to be one concrete type, and different backends would not agree on it. Having it as an associated type of this trait then prevents any sort of default implementation... You are welcome to try it out :)

It would be cool to figure this out, because I think we could use that pattern in a few places. Anyway I don't think it's worth holding up this PR, we can change it later if we figure it out (or if impl Iterator widens type support)

grovesNL · 2018-07-21T06:13:01Z

src/backend/metal/src/command.rs

@@ -208,7 +208,7 @@ struct State {
    work_group_size: MTLSize,
    primitive_type: MTLPrimitiveType,
    resources_vs: StageResources,
-    resources_fs: StageResources,
+    resources_ps: StageResources,


nit: fs is probably the correct Vulkan terminology if we're trying to stay consistent between HAL/Vulkan still

yes, but there is a line we need to draw between Vulkan the API and Metal the implementation. I felt like these resource states are closer to the latter and don't need to stick to Vulkan terms.

grovesNL · 2018-07-21T06:15:16Z

src/backend/metal/src/native.rs

+}
+
+impl PipelineLayout {
+    /// Tet the first vertex buffer index to be used by attributes.


Tet -> Get

grovesNL · 2018-07-21T06:17:04Z

src/backend/metal/src/native.rs

+
+impl PipelineLayout {
+    /// Tet the first vertex buffer index to be used by attributes.
+    pub(crate) fn attribute_buffer_index(&self) -> u32 {


I guess we could inline this too

grovesNL · 2018-07-21T06:28:22Z

src/backend/metal/src/command.rs

-                assert!(is_encoding);
-                match journal.passes.last() {
-                    Some(&(soft::Pass::Render(_), _)) => {
-                        journal.render_commands.extend(commands.into_iter().map(soft::RenderCommand::own));


Could you explain how journal.{render_commands|compute_commands} are used in these cases now? We have a few state switches now between render passes/active encoders and it's a bit hard to see

The PR doesn't really change any journaling logic, just merges the code paths that were not DRY before.

For compute and render, we have 2 versions of the commands processing: one that ignores everything if we are not in the pass (because it's going to be stored in the internal state and still flushed when we start one, later on), and one that expects the current pass. What my cleanup does is implementing the latter on top of the former.

kvark · 2018-07-21T13:25:02Z

@grovesNL thanks for the review! I addressed the notes (some in code, some in comments). Please check out the answers and proceed if satisfied.

kvark · 2018-07-23T03:41:57Z

I've added some basic time tracking:

	Immediate	Deferred	Remote
record begin to submit	9.0ms	8.0ms	10.6ms
record finish to submit	0.02ms	0.03ms	0.02ms
frame wait	0.015ms	0.015ms	0.015ms
submission	0.05ms	2.9ms	0.5ms

From here it's fairly obvious why the Remote sink doesn't show any benefit: it is expected to save us some submission time comparing to Deferred, but that gain is overweight by the increased time to record. Trick is - that recording time wasn't supposed to be affected at all. It's only bad because we aren't yet able to re-use the heap allocated for the commands.

grovesNL

The remaining changes seem reasonable to me

grovesNL · 2018-07-24T01:21:02Z

bors r+

2260: Remote command sink in Metal r=grovesNL a=kvark Fixes #2259 The results so far are not super promising - highly unstable (presumably, because of the dispatch), with performance around `Immediate` mark. We are still missing the most important follow-up here - to avoid any heap allocations when recording commands. Currently, it just goes with `Vec::new()` and grows it for each pass, which shows up in the profile quite a bit. The PR also has a bunch of stuff in general optimizations: - HAL change in the descriptor allocation API to avoid the heap - lighten up Metal descriptor binding path (a bit) by making sure there is enough state slots in advance - simplification and refactoring of `CommandSink` implementations PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [x] tested examples with the following backends: Metal - [ ] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2018-07-24T01:54:49Z

Build succeeded

2264: Render pass descriptor cache for Metal r=grovesNL a=kvark ~~Includes #2260~~ Fixes two of the performance issues in #2161 (RP desc locking and copying costs). Immediate recording FPS doesn't seem to change, maybe slightly lower (touching 90 from below more than from above, as I recall it doing - need to confirm. Edit - confirmed to not be caused by the PR). Deferred recording FPS seem to go from lower 100s to higher, or even from 100 to 110, roughly speaking. There are barely any bottlenecks left for it, outside of the general architecture. Main thread now spends about 14.5% in our code, which is mostly covered by driver interaction. PR checklist: - [ ] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: Metal - [ ] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <[email protected]>

kvark added 3 commits July 20, 2018 22:11

[mtl] remote command sink

d159730

[mtl] important TODOs for future optimization

524d026

Change descriptor allocation API to avoid heap allocation

ecde19c

kvark requested a review from grovesNL July 21, 2018 03:50

grovesNL reviewed Jul 21, 2018

View reviewed changes

[mtl] pre-allocate space for resources in command buffer state

62d16de

kvark force-pushed the mtl-remote branch from a6c399b to 62d16de Compare July 21, 2018 13:20

[mtl] better sync and less allocation on the remote sink

b5266e7

grovesNL approved these changes Jul 24, 2018

View reviewed changes

kvark mentioned this pull request Jul 24, 2018

Render pass descriptor cache for Metal #2264

Merged

4 tasks

bors bot merged commit b5266e7 into gfx-rs:master Jul 24, 2018

kvark deleted the mtl-remote branch July 24, 2018 01:59

kvark mentioned this pull request Jul 24, 2018

[meta] Dota performance issues on Metal #2161

Closed

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote command sink in Metal #2260

Remote command sink in Metal #2260

kvark commented Jul 21, 2018

grovesNL Jul 21, 2018

grovesNL Jul 21, 2018

kvark Jul 21, 2018

grovesNL Jul 24, 2018

grovesNL Jul 21, 2018

kvark Jul 21, 2018

grovesNL Jul 21, 2018

grovesNL Jul 21, 2018

grovesNL Jul 21, 2018

kvark Jul 21, 2018

kvark commented Jul 21, 2018

kvark commented Jul 23, 2018 •

edited

Loading

grovesNL left a comment

grovesNL commented Jul 24, 2018

bors bot commented Jul 24, 2018

Remote command sink in Metal #2260

Remote command sink in Metal #2260

Conversation

kvark commented Jul 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

kvark commented Jul 21, 2018

kvark commented Jul 23, 2018 • edited Loading

grovesNL left a comment

Choose a reason for hiding this comment

grovesNL commented Jul 24, 2018

bors bot commented Jul 24, 2018

Build succeeded

kvark commented Jul 23, 2018 •

edited

Loading