Arcanization of wgpu core resources by gents83 · Pull Request #3626 · gfx-rs/wgpu

gents83 · 2023-03-30T18:14:45Z

Connections

Closes #2710
Closes #4139
Closes #3193
Closes #4686

Steps done:

Removed Token and LifeTime related management
Removed RefCount and MultiRefCount in favour of using only Arc internal reference count
Removing mut from resources and added instead internal members locks on demand or atomics operations
Resources now implement Drop and destroy stuff when last Arc resources is released
Resources hold an Arc in order to be able to implement Drop
Resources have an utility to retrieve the id of the resource itself
Remove all guards and just retrive the Arc needed on-demand to unlock registry of resources asap
Verify correct resources release when unused or not needed
Check Web and Metal compliation (thanks to @niklaskorz)
Fix tests on all platforms
Test a multithreaded scenario
Storage is now holding only user-land resources, but Arc is keeping refcount for resources
When user unregister a resource, it's not dropped if still in use due to refcount inside wgpu
IdentityManager is now unique and free is called on resource drop instead of storage unregister
Identity changes due to Arcanization and Registry being just the user reference
Added MemLeaks test and fixing mem leaks

STATUS:

Ready to be REVIEWED and INTEGRATED 👍

Bump cargo-run-wasm from 0.3.0 to 0.3.2 (gfx-rs#3629)

string::new doesn't take any arguments

Update outdated notice of GL being unsupported (gfx-rs#3638)

Elabajaba · 2023-04-02T20:42:27Z

Not sure if you're aware, but I believe staging buffers aren't being released currently (run bunnymark, spawn some bunnies and watch the GPU's shared memory usage skyrocket (on Vulkan it spams Memory block wasn't deallocated in the terminal, on DX12 it eventually crashes)).

gents83 · 2023-04-02T22:02:38Z

@Elabajaba it should be fixed now :)

Integrating last master changes

wgpu-core/src/track/stateless.rs

niklaskorz · 2023-04-07T09:34:29Z

I've fixed the compiler errors for the Metal backend (should I open a pull request on your branch @gents83?) but there's a final issue to solve: when shutting down a wgpu application using the Metal backend, the following errors occur:

[2023-04-07T09:42:44Z ERROR wgpu_hal::metal::device] No active command buffers for fence value 294
thread 'main' panicked at 'Error in Device::drop: Validation Error

Caused by:
    Parent device is lost
', wgpu/src/backend/direct.rs:1411:34
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
   2: wgpu::backend::direct::Context::handle_error_fatal
             at ./src/backend/direct.rs:312:9
   3: <wgpu::backend::direct::Context as wgpu::context::Context>::device_drop
             at ./src/backend/direct.rs:1411:29
   4: <T as wgpu::context::DynContext>::device_drop
             at ./src/context.rs:2285:9
   5: <wgpu::Device as core::ops::drop::Drop>::drop
             at ./src/lib.rs:2290:13
   6: core::ptr::drop_in_place<wgpu::Device>
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ptr/mod.rs:487:1
   7: core::ptr::drop_in_place<water::framework::start<water::Example>::{{closure}}>
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ptr/mod.rs:487:1
   8: core::ptr::drop_in_place<core::cell::UnsafeCell<dyn core::ops::function::FnMut<(winit::event::Event<()>,&winit::event_loop::EventLoopWindowTarget<()>,&mut winit::event_loop::ControlFlow)>+Output = ()>>
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ptr/mod.rs:487:1
   9: core::ptr::drop_in_place<core::cell::RefCell<dyn core::ops::function::FnMut<(winit::event::Event<()>,&winit::event_loop::EventLoopWindowTarget<()>,&mut winit::event_loop::ControlFlow)>+Output = ()>>
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ptr/mod.rs:487:1
  10: <alloc::rc::Rc<T> as core::ops::drop::Drop>::drop
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/alloc/src/rc.rs:1559:17
  11: core::ptr::drop_in_place<alloc::rc::Rc<core::cell::RefCell<dyn core::ops::function::FnMut<(winit::event::Event<()>,&winit::event_loop::EventLoopWindowTarget<()>,&mut winit::event_loop::ControlFlow)>+Output = ()>>>
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ptr/mod.rs:487:1
  12: core::ptr::drop_in_place<core::option::Option<alloc::rc::Rc<core::cell::RefCell<dyn core::ops::function::FnMut<(winit::event::Event<()>,&winit::event_loop::EventLoopWindowTarget<()>,&mut winit::event_loop::ControlFlow)>+Output = ()>>>>
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ptr/mod.rs:487:1
  13: core::mem::drop
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/mem/mod.rs:988:24
  14: winit::platform_impl::platform::event_loop::EventLoop<T>::run_return
             at /Users/niklaskorz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.27.5/src/platform_impl/macos/event_loop.rs:230:9
  15: winit::platform_impl::platform::event_loop::EventLoop<T>::run
             at /Users/niklaskorz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.27.5/src/platform_impl/macos/event_loop.rs:191:25
  16: winit::event_loop::EventLoop<T>::run
             at /Users/niklaskorz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.27.5/src/event_loop.rs:278:9
  17: water::framework::start
             at ./examples/water/../framework.rs:289:5
  18: water::framework::run
             at ./examples/water/../framework.rs:441:5
  19: water::main
             at ./examples/water/main.rs:820:5
  20: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
[2023-04-07T09:42:44Z ERROR wgpu_hal::metal::device] No active command buffers for fence value 294
[2023-04-07T09:42:44Z ERROR wgpu_core::device::resource] failed to wait for the device: Lost

I have checked the current trunk/master branch of upstream wgpu and can confirm these errors do not occur there.

gents83 · 2023-04-07T09:52:35Z

I've fixed the compiler errors for the Metal backend (should I open a pull request on your branch @gents83?)

Lovely! Sure - let's do that, I'll review it and merge it if possible 👍

but there's a final issue to solve: when shutting down a wgpu application using the Metal backend, the following errors occur:

Ah! Oki - it's possible that there is some missing Arc between fence and command buffer from the message and the Command Buffer is getting released before the fence (or maybe it's just a wrong destruction order).
Is it something that you can verify?

niklaskorz · 2023-04-07T10:24:46Z

Ah! Oki - it's possible that there is some missing Arc between fence and command buffer from the message and the Command Buffer is getting released before the fence (or maybe it's just a wrong destruction order). Is it something that you can verify?

Trying to debug it right now, and actually a command buffer associated with the wait value (or higher) is never added to the fence. The last command buffer to be added always has a value of wait_value - 1.

On upstream, the last (value, command_buffer) tuple in the fence always has value == wait_value.

Edit 1:

Maybe I'm missing something, but I've debug-logged all increments to device.active_submission_index and I can't seem to find where it is increased to the final value used by the failing wait...

[wgpu-core/src/device/queue.rs:556] device.active_submission_index.fetch_add(1, Ordering::Relaxed) = 224
[wgpu-core/src/device/queue.rs:1087] device.active_submission_index.fetch_add(1, Ordering::Relaxed) = 225
[2023-04-07T10:33:15Z INFO  wgpu_hal::metal] Adding command buffer with value 225 to fence
[wgpu-core/src/device/queue.rs:556] device.active_submission_index.fetch_add(1, Ordering::Relaxed) = 226
[wgpu-core/src/device/queue.rs:1087] device.active_submission_index.fetch_add(1, Ordering::Relaxed) = 227
[2023-04-07T10:33:15Z INFO  wgpu_hal::metal] Adding command buffer with value 227 to fence
[2023-04-07T10:33:15Z ERROR wgpu_hal::metal::device] No active command buffers for fence value 228

Edit 2:

From https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU64.html#method.fetch_add:

Adds to the current value, returning the previous value.

Am I right to assume that all calls to .fetch_add(1, Ordering::Relaxed) should be replaced by .fetch_add(1, Ordering::Relaxed) + 1 then?

Edit 2:

Yup, this seems to fix it! I'll open another PR on your branch so you can review @gents83

From https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU64.html#method.fetch_add: > Adds to the current value, returning the previous value.

niklaskorz · 2023-04-07T14:54:54Z

Tested on my own project: with my proposed changes this PR nows builds on all platforms: https://github.com/niklaskorz/linon/actions/runs/4638085286

Unfortunatly, wgpu still does not run on the web, but that is tracked in rust-lang/cargo#3430.

Fix compiler errors for Metal backend

Fix usage of active_submission_index.fetch_add

Fix release build

SludgePhD · 2023-11-18T17:53:12Z

I ran into some deadlocks that happen on both trunk and this branch in #4686, so I submitted a fix against this branch here: gents83#17

cwfitzgerald · 2023-11-18T21:11:12Z

@SludgePhD Nice! The plan is to merge this on monday, so I suspect we should wait post merge to fix a pre-existing issue.

@SludgePhD

Shorten lock durations to avoid deadlocks - thanks a lot @SludgePhD :)

nical · 2023-11-20T07:41:08Z

Alright, brace yourselves folks! 🥁

teoxoy · 2023-11-20T14:27:56Z

@gents83 were the changes to tests/tests/zero_init_texture_after_discard.rs required for arcanization to work?

We seem to no longer test multiple command encoder submissions.

Also, assert_buffers_are_zero was already waiting.

wgpu/tests/src/image.rs

Line 574 in 16ec5b0

device.poll(Maintain::Wait);

gents83 · 2023-11-20T15:48:01Z

@gents83 were the changes to tests/tests/zero_init_texture_after_discard.rs required for arcanization to work?

We seem to no longer test multiple command encoder submissions.

Also, assert_buffers_are_zero was already waiting.

wgpu/tests/src/image.rs

Line 574 in 16ec5b0

device.poll(Maintain::Wait);

@teoxoy with some latest changes from trunk I had that test in particular failing if separated in different sumbissions.
But it's not really a need for Arcanization itself

…6978ae2d251. r=webgpu-reviewers,supply-chain-reviewers,ErichDonGubler,teoxoy Note: This revision contains the arcanization work # Changelog * #4702 Add `WasmNotSendSync` By daxpedda in gfx-rs/wgpu#4702 * #4707 Add more metal keywords By fornwall in gfx-rs/wgpu#4707 * #4706 [naga] remove `span` and `validate` features By teoxoy in gfx-rs/wgpu#4706 * #4709 [dx12] filter out haswell iGPUs By teoxoy in gfx-rs/wgpu#4709 * #4712 Fix typo in pull request template. By jimblandy in gfx-rs/wgpu#4712 * #4598 Add more lints By daxpedda in gfx-rs/wgpu#4598 * #4713 [naga wgsl-in] Include base when printing pointer and array types. By jimblandy in gfx-rs/wgpu#4713 * #4718 [vk] check that adapters are Vulkan compliant By teoxoy in gfx-rs/wgpu#4718 * #4719 [naga] Let constant evaluation of `As` preserve `Splat` expressions. By jimblandy in gfx-rs/wgpu#4719 * #4725 Corrects typo in examples FrameCounter By cantudo in gfx-rs/wgpu#4725 * #3626 Arcanization of wgpu core resources By gents83 in gfx-rs/wgpu#3626 Differential Revision: https://phabricator.services.mozilla.com/D194048

…6978ae2d251. r=webgpu-reviewers,supply-chain-reviewers,ErichDonGubler,teoxoy Note: This revision contains the arcanization work # Changelog * #4702 Add `WasmNotSendSync` By daxpedda in gfx-rs/wgpu#4702 * #4707 Add more metal keywords By fornwall in gfx-rs/wgpu#4707 * #4706 [naga] remove `span` and `validate` features By teoxoy in gfx-rs/wgpu#4706 * #4709 [dx12] filter out haswell iGPUs By teoxoy in gfx-rs/wgpu#4709 * #4712 Fix typo in pull request template. By jimblandy in gfx-rs/wgpu#4712 * #4598 Add more lints By daxpedda in gfx-rs/wgpu#4598 * #4713 [naga wgsl-in] Include base when printing pointer and array types. By jimblandy in gfx-rs/wgpu#4713 * #4718 [vk] check that adapters are Vulkan compliant By teoxoy in gfx-rs/wgpu#4718 * #4719 [naga] Let constant evaluation of `As` preserve `Splat` expressions. By jimblandy in gfx-rs/wgpu#4719 * #4725 Corrects typo in examples FrameCounter By cantudo in gfx-rs/wgpu#4725 * #3626 Arcanization of wgpu core resources By gents83 in gfx-rs/wgpu#3626 Differential Revision: https://phabricator.services.mozilla.com/D194048 UltraBlame original commit: eb839abb42abde594497e419517260997fa1fc4d

xiaopengli89 · 2023-11-29T06:46:27Z

@gents83 Surface::as_hal has break change, we can't modify surface anymore, such as present_with_transaction for ui apps, is there any other way we can set this option?

gents83 · 2023-11-29T07:05:01Z

@gents83 Surface::as_hal has break change, we can't modify surface anymore, such as present_with_transaction for ui apps, is there any other way we can set this option?

Oh! I am guessing that is because I removed the surface_as_hal_mut()? Is it the only case where it's needed?
Otherwise I think it would be preferrable to have a config parameter at initialization time and manage it internally, no?
Looking at some past discussions I do not know exactly how it ended up to be exposed as mut:
#2711 (comment)

xiaopengli89 · 2023-11-29T10:27:39Z

Oh! I am guessing that is because I removed the surface_as_hal_mut()? Is it the only case where it's needed? Otherwise I think it would be preferrable to have a config parameter at initialization time and manage it internally, no? Looking at some past discussions I do not know exactly how it ended up to be exposed as mut: #2711 (comment)

As it's a platform feature, I exposed it as mut: #3123, I don't know if it is appropriate to add surface initialization parameter.

…6978ae2d251. r=webgpu-reviewers,supply-chain-reviewers,ErichDonGubler,teoxoy Note: This revision contains the arcanization work # Changelog * #4702 Add `WasmNotSendSync` By daxpedda in gfx-rs/wgpu#4702 * #4707 Add more metal keywords By fornwall in gfx-rs/wgpu#4707 * #4706 [naga] remove `span` and `validate` features By teoxoy in gfx-rs/wgpu#4706 * #4709 [dx12] filter out haswell iGPUs By teoxoy in gfx-rs/wgpu#4709 * #4712 Fix typo in pull request template. By jimblandy in gfx-rs/wgpu#4712 * #4598 Add more lints By daxpedda in gfx-rs/wgpu#4598 * #4713 [naga wgsl-in] Include base when printing pointer and array types. By jimblandy in gfx-rs/wgpu#4713 * #4718 [vk] check that adapters are Vulkan compliant By teoxoy in gfx-rs/wgpu#4718 * #4719 [naga] Let constant evaluation of `As` preserve `Splat` expressions. By jimblandy in gfx-rs/wgpu#4719 * #4725 Corrects typo in examples FrameCounter By cantudo in gfx-rs/wgpu#4725 * #3626 Arcanization of wgpu core resources By gents83 in gfx-rs/wgpu#3626 Differential Revision: https://phabricator.services.mozilla.com/D194048

gents83 and others added 5 commits March 30, 2023 19:48

First Arcanization no refcount, no lifetime

548cb51

Keeping Arc only outside and not inside resource

d685726

Merge pull request #1 from gfx-rs/master

ec28fc3

Bump cargo-run-wasm from 0.3.0 to 0.3.2 (gfx-rs#3629)

Fixing clippy errors

44413ca

Merge branch 'master' of https://github.com/gents83/wgpu

95141e7

gents83 requested a review from crowlKats as a code owner April 1, 2023 16:09

gents83 and others added 6 commits April 1, 2023 18:13

Removing unneeded replay feature

a9b3bcb

string::new doesn't take any arguments

17b8e71

Changed doc to align to moved files

c7e1c54

Merge pull request #2 from Elabajaba/arc-release-mode-fix

60d3070

string::new doesn't take any arguments

Correct texture view creation for render pass

ecd0cc2

Merge pull request #3 from gfx-rs/master

a1b6b81

Update outdated notice of GL being unsupported (gfx-rs#3638)

gents83 and others added 3 commits April 4, 2023 19:26

Fixing staging buffer issue

a4b83a5

Merge branch 'master' of https://github.com/gents83/wgpu

4961a49

Merge pull request #4 from gfx-rs/master

aa4424e

Integrating last master changes

gents83 marked this pull request as draft April 4, 2023 17:32

Elabajaba reviewed Apr 5, 2023

View reviewed changes

wgpu-core/src/track/stateless.rs Outdated Show resolved Hide resolved

niklaskorz added a commit to niklaskorz/linon that referenced this pull request Apr 7, 2023

Test PR gfx-rs/wgpu#3626

7518257

Fix compiler errors for Metal backend

e1c9b91

niklaskorz added 2 commits April 7, 2023 12:53

Fix usage of active_submission_index.fetch_add

9077b43

From https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU64.html#method.fetch_add: > Adds to the current value, returning the previous value.

Fix release build

f72ff9d

gents83 added 3 commits April 7, 2023 17:50

Merge pull request #5 from niklaskorz/master

669636a

Fix compiler errors for Metal backend

Merge pull request #6 from niklaskorz/atomic-fetch-add

c7b08db

Fix usage of active_submission_index.fetch_add

Merge pull request #7 from niklaskorz/debug-assertions

c935ea2

Fix release build

nical mentioned this pull request Nov 17, 2023

Fix max_vertex_buffers validation #4708

Merged

3 tasks

cwfitzgerald mentioned this pull request Nov 17, 2023

Add more lints #4598

Merged

gents83 added 2 commits November 18, 2023 09:46

Merge branch 'trunk' of https://github.com/gfx-rs/wgpu into wgpu/master

24393a4

Updated changelog

0e437be

Wumpf mentioned this pull request Nov 18, 2023

Deadlock on AMD/Mesa/vk #4686

Closed

Shorten lock durations to avoid deadlocks

724fc1a

Merge pull request #17 from SludgePhD/arcanization

e4cec38

Shorten lock durations to avoid deadlocks - thanks a lot @SludgePhD :)

nical merged commit 6e21f7a into gfx-rs:trunk Nov 20, 2023

cwfitzgerald mentioned this pull request Nov 20, 2023

Re-enable zero-init-workgroup-memory test #4728

Closed

teoxoy mentioned this pull request Nov 21, 2023

Revert changes to zero_init_texture_after_discard #4739

Closed

Friz64 mentioned this pull request Nov 22, 2023

Resize error on DX12 backend in wgpu 0.17 #3967

Closed

jonmmease mentioned this pull request Apr 15, 2024

Buffers Leaked when Device Handle is Dropped Before Buffers #5529

Closed

delan mentioned this pull request May 31, 2024

mach doc hangs and eventually crashes on wgpu-core(doc) servo/servo#32413

Closed

timtom-rs mentioned this pull request Jun 17, 2024

Surface lags behind on window resize #3756

Open

teoxoy mentioned this pull request Jul 16, 2024

[tests] delete outdated comment on DEVICE_DESTROY_THEN_MORE test #5967

Merged

teoxoy mentioned this pull request Jul 30, 2024

Dropped devices leak memory on Windows dx12 #3498

Closed

Conversation

gents83 commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Elabajaba commented Apr 2, 2023

Uh oh!

gents83 commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

niklaskorz commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gents83 commented Apr 7, 2023

Uh oh!

niklaskorz commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niklaskorz commented Apr 7, 2023

Uh oh!

SludgePhD commented Nov 18, 2023

Uh oh!

cwfitzgerald commented Nov 18, 2023

Uh oh!

nical commented Nov 20, 2023

Uh oh!

teoxoy commented Nov 20, 2023

Uh oh!

gents83 commented Nov 20, 2023

Uh oh!

xiaopengli89 commented Nov 29, 2023

Uh oh!

gents83 commented Nov 29, 2023

Uh oh!

xiaopengli89 commented Nov 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

19 participants

gents83 commented Mar 30, 2023 •

edited

Loading

gents83 commented Apr 2, 2023 •

edited

Loading

niklaskorz commented Apr 7, 2023 •

edited

Loading

niklaskorz commented Apr 7, 2023 •

edited

Loading