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

🍏 iOS threading, initial work #1644

Closed

Conversation

Jasper-Bekkers
Copy link
Contributor

@Jasper-Bekkers Jasper-Bekkers commented Aug 5, 2020

The way our event loop works on iOS is that we call run and process all the windowing events on another thread (we send them over an mpsc channel). However, on iOS a lot of things need to happen on the main thread as documented / asserted in winit.

This follows along with some of the macOS code and uses Grand Central Dispatch to move the execution of some of these code blocks back to the main thread.

This doesn't remove all the asserts, nor does it fix all the cases, however I've moved over enough for our internal use-case.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@Jasper-Bekkers Jasper-Bekkers marked this pull request as ready for review August 7, 2020 13:20
@Jasper-Bekkers
Copy link
Contributor Author

@Osspial since you did the initial implementation, could you review this PR or redirect me somebody that can?

@francesca64
Copy link
Member

francesca64 commented Aug 11, 2020

@Jasper-Bekkers I wrote the macOS EL2 backend, original GCD implementation included. The iOS EL2 backend was written by my very close co-worker @mtak-, so I'm the girl you're looking for!

Thanks so much for making this PR! It's fantastic to see more love being given to winit's mobile story. I'll probably find time to review this at some point this week (I've been a bit busier than usual).

Always feel free to ping me for anything iOS or Android related! I'm kinda sorta supposed to keep these things nice as part of my job. I don't watch this repo for my own sanity, but I'll show up if I'm mentioned.

@francesca64 francesca64 self-requested a review August 11, 2020 20:52
@francesca64 francesca64 added DS - ios C - waiting on maintainer A maintainer must review this code labels Aug 11, 2020
@Jasper-Bekkers
Copy link
Contributor Author

Will do @francesca64, good to know I can ping you about this since I'll probably have more patches incoming over time as our use and experience with iOS continues.

@francesca64
Copy link
Member

This'll need a CHANGELOG entry, since I think it's a cool improvement to announce.

I hadn't noticed that the GCD usage in the macOS backend was refactored a while ago; to clarify, I wrote the raw FFI code that was there before that. I don't want it to seem like I took credit for something I didn't do!

Anyway, the code quality looks good! However, when outer_size calls screen_frame I'm hitting EXC_BREAKPOINT at the dispatch_sync_f within exec_sync's implementation. Are you able to reproduce this? The app I'm testing with does all windowing on the main thread, if that makes a difference.

Also, I'm curious about the choice to use exec_sync over exec_async, which is used in the macOS backend.

@Jasper-Bekkers
Copy link
Contributor Author

This'll need a CHANGELOG entry, since I think it's a cool improvement to announce.

Will do.

Anyway, the code quality looks good! However, when outer_size calls screen_frame I'm hitting EXC_BREAKPOINT at the dispatch_sync_f within exec_sync's implementation. Are you able to reproduce this? The app I'm testing with does all windowing on the main thread, if that makes a difference.

I don't have an app like that so I have missed that during testing, I can set up a small repro to take a look at that when I get back to this next week.

Also, I'm curious about the choice to use exec_sync over exec_async, which is used in the macOS backend.

exec_async doesn't allow for return values

@francesca64 francesca64 added C - waiting on author Waiting for a response or another PR and removed C - waiting on maintainer A maintainer must review this code labels Sep 3, 2020
unsafe fn screen_frame(&self) -> CGRect {
self.to_screen_space(msg_send![self.window, bounds])
let window = self.window;
Queue::main().exec_sync(move || {
Copy link
Contributor

@michaelkirk michaelkirk Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way our event loop works on iOS is that we call run and process all the windowing events on another thread (we send them over an mpsc channel). However, on iOS a lot of things need to happen on the main thread as documented / asserted in winit.

It seems like this would deadlock for anyone calling these methods who is already on the main thread. Is it reasonable to say that users of this lib can't call these methods from the main thread? That is surprising to me.

A better solution for your situation might be to hoist these dispatches to your client code.

Or am I misunderstanding?

@Jasper-Bekkers
Copy link
Contributor Author

I've left this open for a while hoping I'd get back to it, but over the last 2 years I didn't find the time to get back to this. Closing to cleanup my open PR lists.

If anyone drives by and is willing to pick this up feel free, but for me it's so low on the prio list I won't be able to get to it any time soon. When I do, I'll either re-open this or start a new PR entirely since it's probably diverged way too much.

@madsmtm
Copy link
Member

madsmtm commented Jan 26, 2023

I hadn't seen this issue, but I have plans to fix this myself in #2464 in the near-ish future, so don't worry about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on author Waiting for a response or another PR DS - ios
Development

Successfully merging this pull request may close these issues.

4 participants