Validate bind group layouts by value #400

yanchith · 2019-12-06T11:50:26Z

This PR widens the comparisons done between two bind group layouts to also consider them equivalent if their vectors of BindGroupLayoutBindings are equal.

Tested on various modifications of the cube example from wgpu-rs. https://gist.github.com/yanchith/56e15fe35658b14587ff9bfcbd53116f

Fixes #335

kvark

Thank you for the PR!
Here is an idea on how we can implement this better.

kvark · 2019-12-09T21:14:08Z

wgpu-core/src/command/bind.rs

-                }) if layout_id == bind_group_layout_id => {
-                    LayoutChange::Match(group_id.value, &self.dynamic_offsets)
+                }) => {
+                    if layout_id == bind_group_layout_id {


I think we can do even better than this. What if we resolve the duplicate layouts earlier, in create_bind_group_layout? If we see that a layout with the same content already exists, we can just return the same ID. This means we'd always be able to compare by ID.

Nice! I'll give it a shot in the coming days 👍

Thanks! And sorry for coming up with this late. I had a feeling that we were missing something, but didn't realize what it was until now 😅

yanchith · 2019-12-12T15:06:02Z

Alright, the changes you requested have some really nice properties: all the work is done at init time and the code is much simpler too 👍

Couple of caveats:

The bind group layout storage is currently scanned linearly to see if any bind group layouts have the same bindings. Not sure how expensive the call is considered to be, but we could use an additional or better suited data structure to make the search faster.
I added the Backend to the storage's value type, because I had to produce it for the ID. Let's hope it doesn't pessimize the data layout much.
Now that the backend is available in the storage, I added a couple of asserts here and there: assert_eq!(backend, storage_backend), similar as was already done for epochs. Not sure if wgpu currently supports running on multiple backends simultaneously, so maybe these new asserts are totally redundant 😂
If there ever is a wgpu_bind_group_layout_destroy (not sure if it is planned), we will need to rething this approach so we don't drop the layout while still in use.

kvark · 2019-12-12T15:26:36Z

wgpu-core/src/device.rs

@@ -1182,6 +1182,15 @@ impl<F: IdentityFilter<BindGroupLayoutId>> Global<F> {
        let hub = B::hub(self);
        let bindings = unsafe { slice::from_raw_parts(desc.bindings, desc.bindings_length) };

+        {
+            let (bind_group_layout_guard, _) = hub.bind_group_layouts.read(&mut token);


let's add a //TODO: figure out if bindings order matters

kvark · 2019-12-12T16:28:04Z

wgpu-core/src/device.rs

@@ -1182,6 +1182,15 @@ impl<F: IdentityFilter<BindGroupLayoutId>> Global<F> {
        let hub = B::hub(self);
        let bindings = unsafe { slice::from_raw_parts(desc.bindings, desc.bindings_length) };

+        {
+            let (bind_group_layout_guard, _) = hub.bind_group_layouts.read(&mut token);
+            for (id, value) in bind_group_layout_guard.iter() {


let's use bind_group_layout_guard.iter().any(|| {..})

kvark · 2019-12-12T16:28:43Z

wgpu-core/src/hub.rs

@@ -89,55 +89,67 @@ impl IdentityManager {
 #[derive(Debug)]
 pub struct Storage<T, I: TypedId> {
    //TODO: consider concurrent hashmap?
-    map: VecMap<(T, Epoch)>,
+    map: VecMap<(T, Epoch, Backend)>,


this is unexpected, why do we need to add the backend here?
Each storage, conceptually, only stores things for the same backend. There can't be two different indices of that map with different backends.

Okay, I thought as much, but good to know! I will have to figure out a way to produce the TypedId for the iterator (need a backend to zip the TypedId), but maybe I can just pass the backend to iter()...

kvark

Thank you!
bors r+

400: Validate bind group layouts by value r=kvark a=yanchith This PR widens the comparisons done between two bind group layouts to also consider them equivalent if their vectors of `BindGroupLayoutBinding`s are equal. Tested on various modifications of the cube example from wgpu-rs. https://gist.github.com/yanchith/56e15fe35658b14587ff9bfcbd53116f Fixes #335 Co-authored-by: yanchith <yanchi.toth@gmail.com>

kvark · 2019-12-16T21:34:32Z

You are right that we'll have an issue with deleting the bind groups, and that's something we need to have sooner rather than later.

kvark · 2019-12-16T21:37:09Z

bors r-
sorry, just something I clarified only now

bors · 2019-12-16T21:37:11Z

Canceled

kvark · 2019-12-16T21:38:18Z

@yanchith can we please store the bindings in a map instead of a vec (i.e. binding number -> value)? we want to make sure the contents of the bindings match by value regardless of the order they are specified.
Also, please squash the commits.
Sorry about getting late to you!

yanchith · 2019-12-17T01:29:10Z

can we please store the bindings in a map instead of a vec (i.e. binding number -> value)? we want to make sure the contents of the bindings match by value regardless of the order they are specified.

No prob, I'll get to it tomorrow 👍

You are right that we'll have an issue with deleting the bind groups, and that's something we need to have sooner rather than later.

Do we want to tackle this now, or once we allow deleting bind group layouts?

kvark · 2019-12-17T02:07:03Z

Everything that can be deleted is supposed to be refcounted internally, including the bind group layouts.
I think what we'll be doing is just manually bumping the refcount when the ID is re-used, and decreasing it when the user requests to delete it.

yanchith · 2019-12-17T11:54:47Z

Done as you requested. There is a caveat now, documented in a TODO I added. We can easily avoid it at the cost of allocating a hashmap that we possibly might throw away right after.

yanchith · 2019-12-17T11:57:35Z

wgpu-core/src/device.rs

@@ -1203,7 +1230,7 @@ impl<F: IdentityFilter<BindGroupLayoutId>> Global<F> {

        let layout = binding_model::BindGroupLayout {
            raw,
-            bindings: bindings.to_vec(),
+            bindings: bindings.iter().cloned().map(|b| (b.binding, b)).collect(),


This is the allocation that we can move up to simplify the comparison and also make it more correct. But we may end up not using it.

kvark

Thank you! Let's go one more step, and we'll be golden

kvark · 2019-12-17T17:06:09Z

wgpu-core/src/device.rs

+                bind_group_layout_guard
+                    .iter(device_id.backend())
+                    .find(|(_, bgl)| {
+                        bindings.len() == bgl.bindings.len()


ok, I see what you did here. Let's move the hashmap creation early and just compare hashmaps here directly. The code would be simpler, and bind group layout creation is not a place that needs optimization - throwing away the hashmap is fine.

To make later bind group layout compatibility checks simpler (and cheaper), deduplicate them on creation. If two bind group layouts with same descriptors are requested, only one is created and the same id is returned for both.

kvark

Thank you!
bors r+

400: Validate bind group layouts by value r=kvark a=yanchith This PR widens the comparisons done between two bind group layouts to also consider them equivalent if their vectors of `BindGroupLayoutBinding`s are equal. Tested on various modifications of the cube example from wgpu-rs. https://gist.github.com/yanchith/56e15fe35658b14587ff9bfcbd53116f Fixes #335 Co-authored-by: yanchith <yanchi.toth@gmail.com>

bors · 2019-12-17T18:12:11Z

Build succeeded

continuous-integration/travis-ci/push

kvark · 2019-12-17T18:15:25Z

Sorry it took us long, and congrats on your contribution!

yanchith · 2019-12-17T18:22:23Z

No prob, happy to help again in the future!

400: Added test for "capture" example r=kvark a=bfrazho I've added a test to validate the capture example. I was planning on using the existing "screenshot.png" that was in there for the assertion for the test, but apparently was slightly different from the one that I was generating. I ended up replacing the screenshot.png with what the test was generating. I'm a little concerned that they were different from the start, but maybe the png generating library made some small changes over time. I did check to make sure that it generated the same png on Windows and Linux on the same computer. Let me know if there is anything that you would like me to change! Co-authored-by: Brian <brian@linux-ccip.lan>

yanchith force-pushed the validate-bind-group-layouts-by-value branch from d2b935f to 20bda02 Compare December 7, 2019 09:59

yanchith marked this pull request as ready for review December 7, 2019 10:00

kvark requested changes Dec 9, 2019

View reviewed changes

yanchith force-pushed the validate-bind-group-layouts-by-value branch from 20bda02 to 77b5a86 Compare December 12, 2019 14:54

kvark requested changes Dec 12, 2019

View reviewed changes

kvark approved these changes Dec 16, 2019

View reviewed changes

yanchith force-pushed the validate-bind-group-layouts-by-value branch from 8d96012 to 659abe7 Compare December 17, 2019 11:50

yanchith commented Dec 17, 2019

View reviewed changes

yanchith force-pushed the validate-bind-group-layouts-by-value branch from 659abe7 to 6da253d Compare December 17, 2019 12:08

kvark requested changes Dec 17, 2019

View reviewed changes

Deduplicate Bind Group Layouts on creation

e9545c9

To make later bind group layout compatibility checks simpler (and cheaper), deduplicate them on creation. If two bind group layouts with same descriptors are requested, only one is created and the same id is returned for both.

yanchith force-pushed the validate-bind-group-layouts-by-value branch from 6da253d to e9545c9 Compare December 17, 2019 17:34

kvark approved these changes Dec 17, 2019

View reviewed changes

bors bot merged commit e9545c9 into gfx-rs:master Dec 17, 2019

yanchith deleted the validate-bind-group-layouts-by-value branch December 17, 2019 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate bind group layouts by value #400

Validate bind group layouts by value #400

yanchith commented Dec 6, 2019 •

edited

Loading

kvark left a comment

kvark Dec 9, 2019

yanchith Dec 9, 2019

kvark Dec 9, 2019

yanchith commented Dec 12, 2019

kvark Dec 12, 2019

kvark Dec 12, 2019

kvark Dec 12, 2019

yanchith Dec 12, 2019

kvark left a comment

kvark commented Dec 16, 2019

kvark commented Dec 16, 2019

bors bot commented Dec 16, 2019

kvark commented Dec 16, 2019

yanchith commented Dec 17, 2019 •

edited

Loading

kvark commented Dec 17, 2019

yanchith commented Dec 17, 2019

yanchith Dec 17, 2019

kvark left a comment

kvark Dec 17, 2019

kvark left a comment

bors bot commented Dec 17, 2019

kvark commented Dec 17, 2019

yanchith commented Dec 17, 2019

Validate bind group layouts by value #400

Validate bind group layouts by value #400

Conversation

yanchith commented Dec 6, 2019 • edited Loading

kvark 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

yanchith commented Dec 12, 2019

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 left a comment

Choose a reason for hiding this comment

kvark commented Dec 16, 2019

kvark commented Dec 16, 2019

bors bot commented Dec 16, 2019

Canceled

kvark commented Dec 16, 2019

yanchith commented Dec 17, 2019 • edited Loading

kvark commented Dec 17, 2019

yanchith commented Dec 17, 2019

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

bors bot commented Dec 17, 2019

Build succeeded

kvark commented Dec 17, 2019

yanchith commented Dec 17, 2019

yanchith commented Dec 6, 2019 •

edited

Loading

yanchith commented Dec 17, 2019 •

edited

Loading