Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-threaded #205

Open
lylythechosenone opened this issue Mar 12, 2024 · 16 comments · May be fixed by #217
Open

Multi-threaded #205

lylythechosenone opened this issue Mar 12, 2024 · 16 comments · May be fixed by #217

Comments

@lylythechosenone
Copy link

Currently, Context and Surface are neither Send nor Sync. What is the reason for this decision? Is it possible to make them at least Send, so they could be made Sync with a Mutex?

@daxpedda
Copy link
Member

daxpedda commented Mar 13, 2024

I can't speak for other backends, but at least for Web this isn't possible (without a custom wire implementation which comes with significant overhead).

This introduces the problem of cross-platform compatibility, either all backends have to be Send or none can.

@lylythechosenone
Copy link
Author

Hmm... In wgpu, there is actually a different concept—all platforms require Send + Sync except web. I think if we could implement Send + Sync for everything except web, that would be completely fine.

However, I can see why it would be undesirable to have different trait implementations on different platforms.

@daxpedda
Copy link
Member

As a maintainer of the Web backend I'm fine with going down that route as well.

@notgull
Copy link
Member

notgull commented Mar 13, 2024

For Windows and macOS this would require a significant reworking of how the backends work. winit makes it work on these platforms by sending messages across threads; since softbuffer deals with large buffers of data this is impractical.

(In a future release Window probably won't be Send + Sync, JFYI)

@lylythechosenone
Copy link
Author

You mean winit::Window? I'll relay this to the wgpu team and see if they have similar plans.

@lylythechosenone
Copy link
Author

@notgull so to be fully clear, this is a platform limitation, and will likely not be changed by the softbuffer team?

@notgull
Copy link
Member

notgull commented Mar 16, 2024

@notgull so to be fully clear, this is a platform limitation, and will likely not be changed by the softbuffer team?

Raymond Chen confirms that device contexts can't be sent between threads. To quote:

The thread that calls functions such as GetDC must also be the one that calls ReleaseDC, but as with window handles, during the lifetime of the DC, any thread can use it. If you choose to use a DC in a multi-threaded manner, it’s your responsibility to coordinate the consumers of that device context so that only one thread uses it at a time

Essentially, a DC is Send in that other threads can use it (at least when given an &mut reference), but it is !Send in that it has to be dropped on the same thread that created it. Therefore Surface on Win32 is pretty unavoidably !Send.

A similar conundrum affects window handles in winit. However winit can get around this by sending a message to the main thread that then destroys the window on the thread that created it. Even if we had the ability to access this mechanism from softbuffer, it would add a significant amount of synchronization overhead to this crate.

I think something similar happens with macOS, but we should check with @madsmtm about that.

X11 is perfectly thread safe, by the way, and could be Send if it wanted to. But for consistency with other platforms it is kept !Send.

@madsmtm
Copy link
Member

madsmtm commented Mar 17, 2024

On macOS, the window handle is basically an &Arc<NSView> that can be cloned and passed between threads safely, but you can only ever access anything from the main thread, including creating a surface / rendering context and attaching that to it. So ideally, we should restrict surface (and context) creation to the main thread.

The rendering can be done from another thread though, i.e. I'm fairly sure the Surface (inner: CALayer) is thread safe, and could be marked Send + Sync (you often need to do a bit of synchronization to not get tearing and other artifacts, so I can't comment on whether it would be a good idea or not).

@lylythechosenone
Copy link
Author

I'd be alright with Send and not Sync, because then one could use a Mutex to get both. That is something I am willing to do. However, because it has neither, even Mutex currently does not work.

@lylythechosenone
Copy link
Author

lylythechosenone commented Mar 17, 2024

Even if we had the ability to access this mechanism from softbuffer, it would add a significant amount of synchronization overhead to this crate.

@notgull what makes it impossible? Also, would that overhead exist even if only that singular message is required? I.e. only Send, and not Sync.

@notgull
Copy link
Member

notgull commented Mar 17, 2024

@notgull what makes it impossible?

Unlike macOS, Windows doesn't have a singular "run this function on the GUI thread" function. The intended solution is to post a message to the window which will then be processed on the window's main thread. The problem is that the message is intended to be handled by the window's WndProc (window procedure), which is controlled by the windowing framework.

The intended solution to this is subclassing, where a second WndProc is uploaded to the window and used as an "additional" window procedure. The issue with this is that the subclassing itself is not thread safe, so we would have to be careful about how we do it.

@notgull
Copy link
Member

notgull commented Mar 17, 2024

Actually, this would be 100% safe if it were guaranteed that the HWND wasn't allowed to leak outside of the origin thread. I've filed rust-windowing/winit#3593 to fix this.

@lylythechosenone
Copy link
Author

Very nice.

@AustinMReppert
Copy link

Thanks!

@lylythechosenone
Copy link
Author

@notgull now that that's merged, are we any further to multi-threaded support?

notgull added a commit that referenced this issue May 8, 2024
Previously, types in softbuffer were !Send and !Sync. However it would
be nice if types could be shared across threads. Therefore I've made the
following changes:

- Context<D> is Send+Sync iff D is Send+Sync
- Surface<D, W> is Send iff D is Send+Sync and W is Send
- Buffer<'x, D, W> is Send iff D if Send+Sync and W is Send

Materially, I've made the following changes across the backends:

- X11, Wayland and KMS use Arc for their displays instead of Rc.
- MacOS uses MainThreadBound to secure windowing resources. This
  restriction was already implicitly enforced anyhow.
- Windows uses a subclassing strategy to manage creating and destroying
  resources. This subclass is applied to windows that are registered
  into softbuffer, and is then used to manage resources.

Closes #205

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull linked a pull request May 8, 2024 that will close this issue
@notgull
Copy link
Member

notgull commented May 8, 2024

@notgull now that that's merged, are we any further to multi-threaded support?

I've filed a PR, but it needs more testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants