Heap-less descriptor sets in Metal #2183

kvark · 2018-06-26T16:49:04Z

Fixes the heap allocations in descriptor sets, to help #2161.
Edit: the improvement is there, but it's not the one we were looking for.

Adds real value semantics for the DescriptorPool, which now owns the allocation and has the actual descriptor sets pointing to it.
PR checklist:

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

Note: it doesn't work correctly yet, but I'd be happy to get the review comments and notes. Perhaps, you can spot the mistake? ;)
Sadly, my CTS is non-operational atm, so it's not easy to figure out a test case.
Note2: the individual commits are not build-able. Will squash once finished.

kvark · 2018-06-26T22:16:21Z

I'm thinking about these follow-ups:

make us not issuing any pre-render/compute commands if not inside a render pass at all
make us not re-set the resources to the same values - check against cache
change the API a little bit to avoid heaps and use iterators with move semantics

kvark · 2018-06-27T03:54:31Z

I've implemented the first two follow-ups here. The code looks much better, subjectively. Performance-wise we don't have hard numbers, just some rough estimation. FPS increased from 65 to 85, which is quite visible but still far from the target.

kvark · 2018-06-27T16:26:25Z

That difference of 20fps is for peak fps (with everything still on screen), but the averages differ less.
Anyhow, I'm hitting a failed assertion occasionally when running Dota now:

ERROR! VK call failed! result = VK_ERROR_FRAGMENTED_POOL ( vkAllocateDescriptorSets( m_pDeviceVulkan, &allocInfo, &pCurrentDescriptorSetPool->m_ppDescriptorSets[ pCurrentDescriptorSetPool->m_nCurrentDescriptorSet - 1 ] ) )
ERROR! VK call failed! result = VK_ERROR_FRAGMENTED_POOL ( vkAllocateDescriptorSets( m_pDeviceVulkan, &allocInfo, &pCurrentDescriptorSetPool->m_ppDescriptorSets[ pCurrentDescriptorSetPool->m_nCurrentDescriptorSet - 1 ] ) )
ERROR! VK call failed! result = VK_ERROR_FRAGMENTED_POOL ( vkAllocateDescriptorSets( m_pDeviceVulkan, &allocInfo, &pCurrentDescriptorSetPool->m_ppDescriptorSets[ pCurrentDescriptorSetPool->m_nCurrentDescriptorSet - 1 ] ) )
ERROR! VK call failed! result = VK_ERROR_FRAGMENTED_POOL ( vkAllocateDescriptorSets( m_pDeviceVulkan, &allocInfo, &pCurrentDescriptorSetPool->m_ppDescriptorSets[ pCurrentDescriptorSetPool->m_nCurrentDescriptorSet - 1 ] ) )
thread '<unnamed>' panicked at 'assertion failed: range.start < range.end', /Users/dmalyshau/Code/gfx/src/backend/metal/src/../../auxil/range_alloc.rs:63:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

First, we don't know how dota reacts on an allocation error (but we can ask). Second, the range allocator failure may signify a mistake in our logic. Need to double check that (cc @Xaeroxe ).

…n fail

kvark · 2018-06-27T16:39:13Z

@Xaeroxe nvm, it was my code's fault all along. Fixed now :)

grovesNL

Looks great 👍 looking forward to the performance improvement!

Dota 2 should have covered/validated most of these changes as well, so it seems safe to proceed.

grovesNL · 2018-06-27T18:48:35Z

src/backend/metal/src/command.rs

+                        native::DescriptorPool::count_bindings(layout.ty, layout.count,
+                            &mut sampler_base, &mut texture_base, &mut buffer_base);
+
+                        if buffer_base != bf_range.start {


nit: seems we could collapse this with the other if buffer_base != bf_range.start below

that's just github view playing tricks on us - the other one has different indentation (so executes at a different frequency)

grovesNL · 2018-06-27T19:07:26Z

src/backend/auxil/range_alloc.rs

        }
    }

    pub fn allocate_range(&mut self, length: T) -> Option<Range<T>> {
+        assert_ne!(length + length, length);


What's the purpose of this assertion? Seems it's just checking for zero?

yes, but T doesn't have a Zero bound :)

kvark

Thanks for taking a look!
I still suspect something is wonky with the code, but having so much stuff piling in doesn't help, and we'll need to investigate post-merging.
bors r=grovesNL

kvark · 2018-06-28T03:21:15Z

src/backend/metal/src/command.rs

+                        native::DescriptorPool::count_bindings(layout.ty, layout.count,
+                            &mut sampler_base, &mut texture_base, &mut buffer_base);
+
+                        if buffer_base != bf_range.start {


that's just github view playing tricks on us - the other one has different indentation (so executes at a different frequency)

2183: Heap-less descriptor sets in Metal r=grovesNL a=kvark Fixes the heap allocations in descriptor sets, to help #2161. Edit: the improvement is there, but it's not the one we were looking for. Adds real value semantics for the `DescriptorPool`, which now owns the allocation and has the actual descriptor sets pointing to it. PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: metal ~~Note: it doesn't work correctly yet, but I'd be happy to get the review comments and notes. Perhaps, you can spot the mistake? ;) Sadly, my CTS is non-operational atm, so it's not easy to figure out a test case. Note2: the individual commits are not build-able. Will squash once finished.~~ Co-authored-by: Dzmitry Malyshau <[email protected]>

bors · 2018-06-28T03:53:49Z

Build succeeded

kvark requested a review from grovesNL June 26, 2018 16:49

[mtl] descriptor management rework to get the real pool semantics

5340e09

kvark force-pushed the mtl-descriptors branch from 15901ef to 5340e09 Compare June 26, 2018 21:42

kvark changed the title ~~[WIP] heap-less descriptor sets~~ Heap-less descriptor sets in Metal Jun 26, 2018

kvark added 2 commits June 26, 2018 23:14

[mtl] only issue resource commands for changed bindings

61a2cef

[mtl] temporary scope for pre-pass commands

9bcc9f4

kvark mentioned this pull request Jun 27, 2018

[meta] Dota performance issues on Metal #2161

Closed

22 tasks

Avoid allocation in free_sets

cb4c48a

[mtl] fix range de-allocation of an empty during descriptor allocatio…

1f9b40e

…n fail

grovesNL approved these changes Jun 28, 2018

View reviewed changes

kvark commented Jun 28, 2018

View reviewed changes

bors bot merged commit 1f9b40e into gfx-rs:master Jun 28, 2018

kvark deleted the mtl-descriptors branch June 28, 2018 03:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heap-less descriptor sets in Metal #2183

Heap-less descriptor sets in Metal #2183

kvark commented Jun 26, 2018 •

edited

Loading

kvark commented Jun 26, 2018 •

edited

Loading

kvark commented Jun 27, 2018

kvark commented Jun 27, 2018

kvark commented Jun 27, 2018

grovesNL left a comment

grovesNL Jun 27, 2018

kvark Jun 28, 2018

grovesNL Jun 27, 2018

kvark Jun 28, 2018

kvark left a comment

kvark Jun 28, 2018

bors bot commented Jun 28, 2018

Heap-less descriptor sets in Metal #2183

Heap-less descriptor sets in Metal #2183

Conversation

kvark commented Jun 26, 2018 • edited Loading

kvark commented Jun 26, 2018 • edited Loading

kvark commented Jun 27, 2018

kvark commented Jun 27, 2018

kvark commented Jun 27, 2018

grovesNL left a comment

Choose a reason for hiding this comment

grovesNL Jun 27, 2018

Choose a reason for hiding this comment

kvark Jun 28, 2018

Choose a reason for hiding this comment

grovesNL Jun 27, 2018

Choose a reason for hiding this comment

kvark Jun 28, 2018

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

kvark Jun 28, 2018

Choose a reason for hiding this comment

bors bot commented Jun 28, 2018

Build succeeded

kvark commented Jun 26, 2018 •

edited

Loading

kvark commented Jun 26, 2018 •

edited

Loading