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

Refactor MacOS and provide a coroutine-driven runloop #237

Closed
wants to merge 24 commits into from

Conversation

willglynn
Copy link

This is a major MacOS refactoring, per discussion in #219, and among other things it closes #219.

Summary

This PR refactors the MacOS event loop such that it occurs in a Runloop. EventsLoop calls Runloop::work(timeout: Timeout), which performs a cycle of the event loop (blocking for up to the specified timeout), delivering zero or more events to a queue, and returns.

This PR also adds a second Runloop implementation, available via --features context, which runs in a coroutine. MacOS X event handling can enter inner loops (see this comment in particular and #219 in general), and the only way to connect winit's polling-centric API with the MacOS event loop while preserving the contracts of both is to run the MacOS event loop in a coroutine.

Comparative behavior

Consider a user with code like this:

loop {
    events_loop.poll_events(|event| {
        // handle event
    });
    
    // draw frame
}

Now let's look at what happens on MacOS during a window resize. During a resize, forward_event_to_cocoa() enters an internal loop where Cocoa callbacks can generate Resized events, but where forward_event_to_cocoa() can not return.

In master, poll_events() stores the user's callback in a place where Cocoa callbacks can reach it. This lets the Resized events get delivered immediately, but the user can't do anything with them, since poll_events() continues to block until the user stops resizing the window. No frames get drawn.

In this PR without --features context, the Cocoa callback creates Resized events and puts them in the queue immediately, but they can't be delivered because the forward_event_to_cocoa() is stuck in an internal loop. This means the user's code does not see the events immediately, but the net effect is the same -- poll_events() blocks for exactly the same amount of time, and the next frame is delayed exactly as long.

In this PR with --features context, the Resized events land in the Shared event queue immediately, and the runloop coroutine yields immediately, even though it's now inside another event loop. poll_events() returns immediately, the user can draw a frame, and the next call to poll_events() resumes that interior loop where it left off.

Likewise, in master and in this PR without --features context, Proxy::wakeup() events are deferred until after a resize. In this PR with --features context, wakeup events are delivered promptly from any state.

Structure

The basic Runloop does everything in Runloop. The context-enabled version moves those functions to InnerRunloop, and adds a Runloop with the same public interface whose purpose is to start an InnerRunloop coroutine and context switch into it as needed.

Entering the coroutine

After initialization in Runloop::new(), Runloop::work() is the only place where the main thread context switches into the InnerRunloop coroutine.

Runloop::work() is called only by EventLoop::get_event(). Whatever invariants about the main thread are true at that point remain true for the entire duration of the inner runloop. For example, we know that the main thread is not holding locks inside Shared, so the coroutine can acquire and release locks on Shared without deadlocking on the main thread.

Runloop::work() checks the NSThread's identity to ensure that the coroutine can only be resumed from the main thread.

Moving data into the coroutine

The initial call into the coroutine entrypoint needs to bring an InnerRunloop, and all subsequent calls into the coroutine bring a Timeout. I made this happen by combining three properties:

  • context can carry a data: usize along during a context switch.
  • When execution is transferred into a coroutine, the caller stops running until execution transfers back.
  • Option::take() moves a value out of the Option.

Put together, this means the caller can declare a local Option, pass a &mut of it into the coroutine, and the coroutine can safely .take() its value. Again, there's no concurrency at work -- everything executes sequentially -- so we can guarantee that there's only one mutable borrow to the caller's Option.

Actually doing this via a usize requires &mut as *mut as usize on the way down, and usize as *mut transmute &mut on the way up. One could transmute straight from usize to &mut, but I kept *mut for symmetry.

Calls into the coroutine look like:

// caller
let mut input: Option<Foo> = Some(Foo);
context.resume(&mut input as *mut Option<Foo> as usize);

                // coroutine's resume() returns, holding the &mut Option
                let t: context::Transfer = context.resume();
                let input: *mut Option<Foo> = t.data as *mut Option<Foo>;
                let input: &mut Option<Foo> = mem::transmute(input);
                let input: Foo = input.take().unwrap();
                // input is now moved to the coroutine
                // coroutine eventually returns to the caller
                t.context.resume();

// caller's context.resume() returns
// input = None, since the value was taken by the coroutine

Inside the coroutine

yield_to_caller() is the place where the coroutine context switches back to Runloop::work(), and it is therefore also the place where the coroutine resumes.

yield_to_caller() sets a thread local cell containing the caller's context when execution switched into the coroutine, and it moves the context out of that cell before it switches back. If that cell is full, then we are currently inside the coroutine; if that cell is empty, then we are not.

yield_to_caller() also sets a thread local cell containing the caller's Timeout, which can be retrieved by fn current_timeout() -> Option<Timeout>. The inner runloop uses this when asking Cocoa to receive an event.

The coroutine's InnerRunloop looks very much like the normal blocking Runloop. It tries to receive an event from Cocoa, forwards it back to Cocoa, translates it into zero or more Events, and posts them to the queue.

Exiting the coroutine

Shared::enqueue_event() enqueues the event and then tries to wake the runloop, and the coroutine version of Runloop:::wake() calls yield_to_caller(). This means that if we enqueue an event from inside the coroutine -- for example, from the normal inner runloop or because a Cocoa callback posted an event -- then execution immediately returns to EventLoop::get_event(), which checks Shared's event queue, finds an event, and returns it to its caller.

If Runloop::wake() finds that its caller is not inside the coroutine -- for example, because it's on a different thread calling Proxy::wakeup() -- it calls CFRunLoopWakeUp(CFRunLoopGetMain()) in hopes of stirring the main runloop. The main CFRunLoop inside the coroutine wakes up, regardless of whether that's the normal loop or whether it's that an inner loop caused by a resize. This wakeup triggers a CFRunLoopObserver, which calls yield_to_caller(). That gets us back to the main thread even if we're stuck inside someone else's loop.

In addition to the runloop observer, the coroutine has a timer whose callback calls yield_to_caller(). This causes needless context switches, but it also sets an upper bound on how long the coroutine can run continuously without yielding, which significantly improves worst-case latency.

@jwilm
Copy link
Contributor

jwilm commented Jul 18, 2017

I'm really excited about the work you're doing here. The stack management on winit's side seems good, and not running user code in the coroutine is a good choice. One nagging concern I've had is how safe it is to suspend execution of the Cocoa APIs? For instance, what happens when there are multiple windows being managed? It seems that both windows could be suspended in a coroutine at the same time. What happens if there's shared state in the Cocoa side?

One situation where this could show up is with a modal dialog. The main window may have already processed some events and suspended the Cocoa event loop, opened the modal window, and started processing events there.

By the way, my question is coming from a place of ignorance about how Cocoa works in general, and this may just be a non-issue.

@willglynn
Copy link
Author

One nagging concern I've had is how safe it is to suspend execution of the Cocoa APIs? For instance, what happens when there are multiple windows being managed? It seems that both windows could be suspended in a coroutine at the same time. What happens if there's shared state in the Cocoa side?

To start: it's important that there's no preemption of Cocoa. The Runloop coroutine can yield back to the user from the deepest depths of a Cocoa call stack, but even in that case, it's returning to the user from within a Cocoa callback. In other words, writing this code with --features context:

loop {
    events_loop.poll_events(|event| {
        // handle event
    });
    
    draw_frame();
}

…lets the user draw_frame() at the same point as the user can right now:

events_loop.run_forever(|event| {
    // handle event
    if it_is_time_to_draw_a_frame {
      draw_frame();
    }
});

Both allow the user to interact with Cocoa from the main thread in the middle of a Cocoa callback. Cocoa's state is identical in both cases; the coroutine approach just swizzles around the stack pointer so that the user can do this outsidewinit instead of doing it from a callback inside winit.

As for multiple windows, examples/multiwindow.rs uses a single event loop (and thus a single Runloop of either kind) to serve multiple windows. That's the normal, expected, and sensible way to do things. Working that single loop will provide events for all the windows.

I'm not sure what would happen if you tried creating multiple windows with unrelated event loops, and you're correct, that could give you multiple coroutines suspended at different points. However, given that Cocoa actually provides only one event loop, the NSThread check won't switch into any runloop from the wrong thread, and the runloops only yield at defined points, I don't think there's any UB potential.

I think the worst case is that you could construct something like:

events_loop_1.run_forever(|event| {
    events_loop_2.run_forever(|event| {
        // ¯\_(ツ)_/¯
    });
});

The new runloops follow the same pattern as the old ones, so such a construction should succeed or fail in much the same way that it would now.

@mitchmindtree
Copy link
Contributor

Just wanted to mention that I'm also quite excited for this PR! That said, I haven't had a close look yet - I'll try to get around to testing and reviewing this soon.

@willglynn
Copy link
Author

FYI: I will have intermittent internet connectivity starting tomorrow for about a week. That's partly I pushed to write all this up and open the PR today. I expect that proper review of this changeset will take time; I wanted to get that process started before I left, and I wanted to provide enough information that it could proceed without requiring me to be immediately available.

Having said that, I will still have a computer with a Rust compiler and a winit checkout :-) If you have a question, or want a change, or find a problem, let me know and I'll do what I can.

@willglynn
Copy link
Author

I found and fixed a regression versus master without --features context. Specifically, EventsLoopProxy::wakeup() + run_forever() would not deliver Awakened events until some other event arrived.

The old approach was for EventsLoopProxy::wakeup() to post an NSEvent of type NSApplicationDefined to an OS event queue, which was translated into an Event::Awakened on receipt. The new approach in this PR is for EventsLoopProxy::wakeup() to post an Event::Awakened to a winit event queue, and for posting events to imply calling Runloop::wake(), which is supposed to do something to reduce the time spent between "event is in the queue" and "user gets the event".

The coroutine runloop implementation of Runloop::wake() causes a context switch. If we're trying to wake from inside the coroutine, it can context switch back directly. Otherwise, Runloop::wake() assumes the runloop is asleep on a different thread, so it tells the OS to wake the runloop. Waking the runloop triggers the runloop observer, and the runloop observer callback causes a context switch. The net effect in both cases is that the main thread checks the event queue and returns to the user.

The blocking runloop tries to wake itself by asking the OS, but the runloop was missing logic to check for queued events, which meant it would sit in receive_event_from_cocoa() until an event got delivered. This is fine when called via poll_events(), but run_forever() blocks forever, so the next check could be arbitrarily far in the future.

I addressed this by making the non-coroutine Runloop::wake() post an NSApplicationDefined event just like before. Unlike before, this event does nothing -- it does not translate into an Awakened event, since that event is already in the queue. Its function is to allow receive_event_from_cocoa() to return so that the runloop can check Shared::has_queued_events() and return as needed.


All this prompted me to write a tool to measure wakeup latency, which I stuck in examples/wakeup_latency.rs. It spawns a thread to call EventsLoopProxy::wakeup() 60 times per second, uses run_forever() in the main thread to receive winit::Event::Awakened 60 times per second, and calculates how long the wakeup took to arrive. Every 300 events (=~ 5 seconds), it calculates the mean along with various percentiles and prints them.

Typical wakeup latency

Baseline from current master:

wakeup() -> Event::Awakened latency (all times in µs)
mean    max     99%     95%     50%     5%      1%      min
1219    90248   35209   434     330     148     115     102
312     11997   441     427     310     100     78      64
288     600     448     428     291     153     137     124
308     842     596     433     317     184     143     108
301     446     436     430     321     151     131     127
329     445     441     433     334     209     171     135
329     440     438     432     333     217     162     130
327     465     441     432     332     199     155     129
313     446     439     432     323     186     142     97
311     1272    441     429     320     167     132     118
259     1024    519     428     231     138     113     92
^C

This PR without --features context now fares comparably:

wakeup() -> Event::Awakened latency (all times in µs)
mean	max	99%	95%	50%	5%	1%	min
902	72556	15382	406	301	140	112	78
301	429	413	404	316	154	137	128
314	505	415	405	319	204	183	144
297	418	413	405	315	153	136	130
266	417	409	403	246	147	127	106
318	422	411	403	321	208	199	196
313	455	415	405	321	197	150	137
298	5631	414	405	294	159	114	87
267	1549	409	403	227	176	137	96
305	630	418	406	316	194	144	79
^C

This PR with --features context runs significantly faster, with median latencies lower than the minimum latencies previously seen:

wakeup() -> Event::Awakened latency (all times in µs)
mean	max	99%	95%	50%	5%	1%	min
491	40276	20302	159	51	17	4	2
67	178	164	156	57	17	5	2
82	207	199	160	76	25	13	8
80	234	199	159	75	23	12	3
79	228	180	161	70	25	19	5
74	216	171	158	64	25	20	5
75	194	183	159	71	24	11	3
74	199	173	158	72	25	18	9
68	199	160	154	64	20	10	4
65	208	168	154	62	22	13	3
^C

Resizing wakeup latency

Here I tried to grab the window for resizing the moment it printed the first summary line, then hold it for ~6 seconds.

Current master shows extreme latencies, since the NSApplicationDefined events (which translate into Awakened events) do not get delivered until the resize is complete:

wakeup() -> Event::Awakened latency (all times in µs)
mean	max	99%	95%	50%	5%	1%	min
1499	113548	58438	212	107	72	65	63
3127967	6012464	5959120	5751020	3154140	181	128	104
31976	586795	530781	310085	165	137	112	99
^C

This PR without --features context fares no better, since like master its runloop gets stuck in forward_event_to_cocoa():

wakeup() -> Event::Awakened latency (all times in µs)
mean	max	99%	95%	50%	5%	1%	min
1157	87584	34583	406	303	137	112	82
2931622	5919778	5860272	5640558	3056384	204	188	140
137012	1219426	1165654	951234	304	135	110	67
^C

This PR with --features context suffers versus the not-resizing case, since the main thread still becomes busy doing other things during a resize, but it actually does deliver Awakened events promptly:

wakeup() -> Event::Awakened latency (all times in µs)
mean	max	99%	95%	50%	5%	1%	min
332	32771	6514	130	37	11	3	2
876	30898	28488	232	38	10	3	3
41	158	97	78	44	6	3	3
^C

@willglynn
Copy link
Author

I rebased this PR (dff60d3 => 6ea5d45).

Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Ok, I finally got the chance to read through this and test the examples tonight. Sorry for the delay!

Overall I'm really happy with the changes.

The refactored code is quite a bit tidier and more readable than it was before. I found the runloop_context code surprisingly readable with concise comments. I wonder if we could even include the more detailed overview (from the top comment of this PR) at the top of the module? I think it provides a valuable overview of the reasoning and workings behind the context switching which future contributors could benefit from.

I was able to run all of the examples with and without --features "context" successfully. The proxy example actually works properly for the first time on macos with --features "context" which is really nice. By this I mean the user can actually receive Awakened events during a resize (not currently working on master). I ran the wakeup_latency example with and without --features "context" and saw very similar results to what you show in your previous comment.

One regression that I noticed when running the examples with --features "context" is that the CPU usage no longer drops to 0% when they are completely unfocused and idle - instead they seem to hover at about 4%. @willglynn do you have any idea what might be causing this? I ran it through the Instruments Time Profiler and most of the time seems to be spent in __CFRunLoopRun and in particular __CFRunLoopDoTimers, __CFRunLoopServiceMachPort and __CFRunLoopDoObservers. I originally noticed this when running the window example with and without --features "context" and watching Activity Monitor. I'd be curious to hear if you see the same behaviour.

In summary I'm in favour of this PR, however I'd like for us to get to the bottom of the idle CPU usage before going ahead with it. Other than that, I think the complexity of the additional context switching is well commented/documented, neatly written and easily worth the benefits. These benefits bring macos behaviour on par with the behaviour of the other OSes and remove the need for some really frustrating workarounds currently required by users who desire properly supporting macos.

Also as a side note, if we decide to go ahead with this PR I'd prefer that we remove the --feature "context" flag and just make this the default behaviour (however admittedly it is really useful to have while reviewing and testing the changes!).

In the meantime the more macos eyes the merrier - @RobSaunders any chance you'd be interested in reviewing/testing this too? Any other macos users welcome to test/review - even it it's just testing the branch with the examples or your own existing projects.


// Turn this into a mutable borrow, then move the inner runloop into the coroutine's stack
let mut inner: InnerRunloop =
unsafe { mem::transmute::<*mut Option<_>, &mut Option<_>>(inner) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be slightly nicer / less scary as:

inner.as_mut().unwrap()

although I guess it's fine as we know that inner will never be null!

Copy link
Author

Choose a reason for hiding this comment

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

I chose to see this as a chain of &mut -> *mut -> usize -> *mut -> &mut casts. The thread always goes straight down that line, executing everything top to bottom, and I opted to reinforce that idea by fearlessly reaching for transmute.

transmute is a bold statement, but so is "these things are guaranteed to happen in this order, so I know for a fact that this usize is actually a &mut Option". My opinion was that this particular construction improves clarity here, especially when the alternative is as_mut() -> Option<&mut Option<T>> in a confusing context where we've already got &muts and Options in play.

Having said all that, if you want me to change it, let me know :-)

@willglynn
Copy link
Author

I wonder if we could even include the more detailed overview (from the top comment of this PR) at the top of the module? I think it provides a valuable overview of the reasoning and workings behind the context switching which future contributors could benefit from.

Sure, I'll work it in.

One regression that I noticed when running the examples with --features "context" is that the CPU usage no longer drops to 0% when they are completely unfocused and idle - instead they seem to hover at about 4%. @willglynn do you have any idea what might be causing this?

That's almost certainly the timer. I added it to contain the worst case latency, but there's no reason at all for it to run when the application is backgrounded. It's probably enough to activate the timer only during a resize, bounded by windowWillStartLiveResize/windowDidEndLiveResize callbacks on NSWindowDelegate, though perhaps it would be better to use it as an alarm clock to unconditionally wake us out of an event send.

I also had the thought that the runloop observer could possibly be replaced with CFRunLoopPerformBlock. Instead of context switching each time the runloop sleeps/wakes in case the user wants their thread back, we can enqueue a block that performs a context switch only when the user actually indicates they want their thread via wakeup().

My previous measurements were subjective and visual, which was sufficient to address the problems I observed in master but is not a great basis for making efficient tradeoffs. Now that --example wakeup_latency spits out concrete numbers, I'll run some experiments and see what I can do.

@willglynn
Copy link
Author

I rebased again (6ea5d45 => 8c2aaee), this time fixing conflicts from #229.

@willglynn
Copy link
Author

Trading out the unconditional runloop observer for an as-needed queued block shows worse steady state timings, but they're still better than master, resizing still works fine, and it avoids some spurious callbacks:

wakeup() -> Event::Awakened latency (all times in µs)
mean	max	99%	95%	50%	5%	1%	min
521	44335	12039	189	82	24	12	5
78	260	196	163	74	20	8	5
89	223	194	150	81	36	24	16
86	225	190	146	81	27	12	2
94	266	207	155	83	40	24	6
89	256	198	159	81	43	24	4
87	196	187	147	79	34	23	15
86	215	184	139	77	43	27	15
80	249	183	142	74	30	12	7
^C

I also found that core-foundation-sys doesn't provide CFRunLoopPerformBlock so I pulled in the dispatch crate to say something like dispatch_async_f(dispatch_get_main_queue(), …) instead. Point is, another thread can somehow gets the main thread to call yield_to_caller() on demand directly.

@willglynn
Copy link
Author

willglynn commented Aug 3, 2017

I ripped out the unconditional timer, and added a check -- if you enter the coroutine with Timeout::Now, it'll use dispatch_after() to enqueue a block for 2 ms after entering the runloop.

This puts a slightly lower bound on the amount of time the runloop can block the main thread -- 2ms from runloop entry vs 2.5ms average waiting for a 5ms timer -- and it's consistent to boot. It also does nothing in the Timeout::Forever case, so the examples' CPU usage stats are better.

Coupled with the previous change, this cuts Activity Viewer's "idle wakeups" on --example wakeup_latency from ~250/s (the timer, runloop observer, and the helper thread sending wakeups) to ~60/s (just the helper thread).

poll_events() and run_forever() now delegate event production to get_event(),
a new function whose job it is to either a) produce Some(Event) by a Timeout
or b) return None. This dramatically simplifies the entire event loop.

MacOS in particular has multiple ways that Events can be produced: wakeup(),
AppDelegate callbacks, and a Cocoa event receive -> send -> translate cycle.
The first two are handled by an event queue; wakeup() now posts an
Event::Awakened to the queue, just like the callbacks.

The CocoaEvent cycle is handled either via the usual blocking
[NSApp sendEvent:] call or via a coroutine-based event dispatcher. The
coroutine yields back to get_event() on a regular basis, even while it's in
the middle of sending events. This allows get_event() to return an event to
the calling application even if it is still in the middle of a sendEvent:
call.
events_loop::runloop is now responsible for the entire Cocoa runloop cycle.
It's a simple blocking implementation (which fails during a resize), but
it's an API that maps cleanly to a coroutine-based implementation.

NSEvent concerns are now separated into events_loop::nsevent.
This puts an upper bound on how long the coroutine can execute without
yielding back to the main thread. The runloop observer fires before going to
sleep, which is useful if `forward_event_to_cocoa()` has triggered an inner
event loop.

The timer could be used to wake the event loop from a different thread, but
in practice that has a few milliseconds of latency, and it also requires
finding a reference to the timer from `Runloop::wake()`. Instead, the timer
is configured to fire every few milliseconds all the time, again in the
pursuit of lower latency.
Posting an event causes receive_event_from_cocoa() to return, which gives
the runloop an opportunity to check Shared::has_queued_events(). This ensures
that wakeup events can be delivered promptly in the absence of other event
traffic.
@Diggsey
Copy link

Diggsey commented Nov 28, 2017

What about simply making coroutine a default feature? Clients will still be able to disable it if they don't care about the resizing behaviour on osx.

@willglynn
Copy link
Author

@tomaka I think there will be more than two implementations, which is why I refactored everything in favor of minimizing the feature-dependent code. I chose context for a second implementation because it let me fix #219 in stable Rust. Judging by upstream activity, I expect we'll be eventually able to write a third implementation in nightly Rust, which will eventually make its way into stable Rust.

Implementation Resizing works Stable Rust Extra dependencies
runloop.rs None
runloop_context.rs context
Native Rust coroutines ❌ => ✅ None

Until it's possible to do the same thing in stable Rust – on a version of stable Rust that you're willing to require – I think it's appropriate to have multiple implementations despite the cost.

@Diggsey
Copy link

Diggsey commented Dec 13, 2017

I'd really like to see this get merged 😉
@willglynn does my resolution of the merge conflicts look correct to you? If so, could you update your PR?

@mcoffin mcoffin mentioned this pull request Dec 27, 2017
5 tasks
@bzm3r
Copy link

bzm3r commented Jan 4, 2018

Where are we on this?

@willglynn
Copy link
Author

As far as I know, there's no outstanding changes needed, this just… never got merged. I haven't been following along with winit/master changes since roughly cb5df71. If all this needs is another rebase, I'll take another look, but I'm not eager to invest more time into this PR if it's just going to sit here again.

@Diggsey
Copy link

Diggsey commented Jan 4, 2018

@willglynn it was waiting on you to review (and apply) my resolution of the merge conflicts on this PR: master...Diggsey:coroutines

Unfortunately there are new merge conflicts since then. I will resolve them and update my branch.

@willglynn
Copy link
Author

@Diggsey: Sure, I know this PR has conflicts now. I haven't reviewed either winit/master for context or your fork of this PR for correctness. My status comment is more general: this PR was mergeable for quite a while, there were zero outstanding issues that I knew about, and the PR was neither accepted nor rejected.

I know about one outstanding issue now (merge conflicts), and if that's all that needs to happen to get this merged, I'm happy to fix it. On the other hand, @tomaka recently expressed hesitation about this PR, and viewed in light of his general aversion to this concept in #219, I'm not inclined to hop onto the rebase treadmill without concrete guidance from the project about what's needed to make this happen.

@Diggsey
Copy link

Diggsey commented Jan 4, 2018

@willglynn well tomaka said:

I guess we could merge this after conflicts are resolved.

I've resolved the merge conflicts on my fork: master...Diggsey:coroutines

@jwilm
Copy link
Contributor

jwilm commented Feb 21, 2018

We are really excited to have this for Alacritty. @tomaka, do you feel your concerns have been adequately addressed?

@tomaka
Copy link
Contributor

tomaka commented Feb 22, 2018

@tomaka, do you feel your concerns have been adequately addressed?

I'd be up for merging if the conflicts are solved.

@jrmuizel
Copy link
Contributor

@willglynn, given @tomaka's willingness to merge do you think you'll be able to do a rebase soon?

@tomaka
Copy link
Contributor

tomaka commented Mar 28, 2018

The problem is that the OSX backend has received some behaviour changes since then. I'm not sure if it's a good idea to have two different implementations of the same backend.

@francesca64
Copy link
Member

@willglynn I'm trying to understand how to proceed with this, and I'm a bit confused on why there needs to be two implementations. What reason would there be for not using the coroutine feature?

@willglynn
Copy link
Author

I need to catch up on what happened in master since this PR developed conflicts, rebase this PR, test it again, make it work, and give everyone a thumbs up again. I've been traveling and haven't sat down to do it yet.

There's two implementations mostly because of discussion in #219 that made me think that a coroutines-only approach wouldn't be welcome in winit. Making coroutines an opt-in feature seemed like it might be more palatable.

I'm not terribly concerned about having two runloop implementations. On the one hand, it is a form of duplication, but on the other, the simple runloop is pretty simple, and if there is a problem with the coroutine-driven runloop it could be nice to have a way to turn it off. Having said that, I think this situation is worse in another way: coroutines solve real problems on MacOS platform, which ultimately means adding bug fixes behind a feature flag.

Another thought rolling around in my head is #231, especially this comment and following. This PR proposes adding coroutines to connect MacOS to winit's current API, but it seems a lot like we should be looking at changing winit's API instead, in which case we wouldn't need to do any of this.

@francesca64
Copy link
Member

it seems a lot like we should be looking at changing winit's API instead, in which case we wouldn't need to do any of this.

That's something I've been actively investigating lately; I'm increasingly getting the impression that we'll inevitably have to invert the control flow. I just need to make sure I understand things well enough to be able to make a strong case for it.

It seems that broadly speaking, such an API change would lead to the backends becoming more idiomatic (wrt the design of the underlying platform APIs) and less complex. Even without considering functionality improvements, that would be a pretty big boon for contributors and maintainers.

@francesca64 francesca64 added this to the EventsLoop 2.0 milestone May 6, 2018
@cztomsik
Copy link

I know I'm late but the alternative api provided here is exactly what I want (and I'm considering either forking winit or just using glfw because it's possible to keep the control that way)

Events Loop 2.0 #459 seems to go in the opposite direction :-/

To give a bit more context, we run inside of node.js event loop which is on the main thread and we need to return if there were no events so the other work can be done (async I/O), we also want to switch between polling and blocking depending on whether we are animating or not to save power

Yes, it's a bit specific use-case but at the same time, I believe this would be the same for python and maybe even other platforms

@mitchmindtree
Copy link
Contributor

@cztomsik the ability to return from the main loop in EventLoop 2.0 is also something I'm interested in - it might be worth opening a new issue specifically for this.

@elinorbgr
Copy link
Contributor

Event Loop 2.0 does introduce a run_return method as an extension trait for platforms that can support returning from the event loop though :

https://github.com/tomaka/winit/blob/eventloop-2.0/src/platform/desktop.rs

@cztomsik
Copy link

cztomsik commented Apr 1, 2019

run_return is very different thing - you're given the control back, but after the loop is already destroyed (or at least it was the case in the macos code I've seen before but I can't find it right now)

we will probably just migrate to glfw, because it's better fit for us (more explicit api in overall and for some reason even much less cpu usage), I only wanted to give you feedback about some specific but real-world usage

@Osspial
Copy link
Contributor

Osspial commented Jun 13, 2019

I'm closing this because of EventLoop 2.0.

@Osspial Osspial closed this Jun 13, 2019
madsmtm added a commit to madsmtm/winit that referenced this pull request Jun 11, 2022
* refactor(windows): `begin_resize_drag` now similar to gtk's (rust-windowing#200)

* refactor(windows): `begin_resize_drag` now similart to gtk's

* fix

* feat(linux): skipping taskbar will now also skip pager (rust-windowing#198)

* refactor(linux): clean dummy device_id (rust-windowing#195)

* refactor(linux): clean dummy device_id

* fmt

* feat(linux): allow resizing undecorated window using touch (rust-windowing#199)

* refactor(windows): only skip taskbar if needed when `set_visible` is called (rust-windowing#196)

* fix: increase borderless resizing inset (rust-windowing#202)

* fix: increase borderless resizing inset

* update some comments

* Replace winapi with windows crate bindings shared with WRY (rust-windowing#206)

* fix(deps): update rust crate libayatana-appindicator to 0.1.6 (rust-windowing#190)

Co-authored-by: Renovate Bot <[email protected]>

* Add Windows crate and webview2-com-sys bindings

* Initial port to webview2-com-sys

* Finish conversion and remove winapi

* Fix renamed lint warning

* Fix all match arms referencing const variables

* Put back the assert instead of expect

* Point to the published version of webview2-com-sys

* Cleanup slightly weird BOOL handling

* Replace mem::zeroed with Default::default

* Add a summary in .changes

* Remove extra projects not in config.json

* Fix clippy warnings

* Update to 32-bit compatible webview2-com-sys

* Better fix for merge conflict

* Fix clippy errors on Windows

* Use path prefix to prevent variable shadowing

* Fix Windows clippy warnings with nightly toolchain

* Fix Linux nightly/stable clippy warnings

* Fix macOS nightly/stable clippy warnings

* Put back public *mut libc::c_void for consistency

* Re-run cargo fmt

* Move call_default_window_proc to util mod

* Remove unnecessary util::to_wstring calls

* Don't repeat LRESULT expression in match arms

* Replace bitwise operations with util functions

* Cleanup more bit mask & shift with util fns

* Prefer from conversions instead of as cast

* Implement get_xbutton_wparam

* Use *mut libc::c_void for return types

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>

* fix(keyboard): add mapping for space key on Windows (rust-windowing#209)

* fix(keyboard): add mapping for space key on Windows

* change file

* feat: impl Clone for EventLoopWindowTarget (rust-windowing#211)

* chore: add `on_issue_closed.yml` (rust-windowing#214)

* Update tray dependency version (rust-windowing#217)

* Delete on_issue_closed.yml (rust-windowing#221)

* refactor(linux): event loop (rust-windowing#233)

* Use crossbeam::channel

* Fix crossbeam channel import

* Add check on poll event

* Fix deadlock when unregistering shortcut on Linux (rust-windowing#230)

* Add fullscreen monitor selection support on Linux (rust-windowing#235)

* Add fullscreen monitor support on Linux

* Add change file

* Remove todo on videomode

* Fix clippy

* Update to 2021 edition (rust-windowing#236)

* Update to 2021 edition

* Fix clippy

* Add run_return on Linux (rust-windowing#237)

* Add run_return on Linux

* Add main context

* Add run_return trait on Linux (rust-windowing#238)

* Fix: rust-windowing#239 Update webview2-com and windows crates (rust-windowing#240)

* Replace webivew2-com-sys with prebuilt windows

* Use windows utility instead of direct GetLastError

* Bump windows version and add changelog

* Run cargo fmt

* Restore inverted matches macro

* Scope constants in match arms

* Fix inverted null check

* Update src/platform_impl/windows/util.rs

Co-authored-by: Amr Bashir <[email protected]>

* Use env_logger instead of simple_logger (rust-windowing#241)

* Use env_logger instead of simple_logger

* Make clippy happy

* Cherry pick commits to next (rust-windowing#244)

* feat(macos): Add `unhide_application` method, closes rust-windowing#182 (rust-windowing#231)

* feat(macos): Add `unhide_application` method

* Update src/platform/macos.rs

Co-authored-by: Amr Bashir <[email protected]>

* Reanme to `show_application()`

* Remove broken doc link

Co-authored-by: Amr Bashir <[email protected]>

* feat: Allow more strings to parse to keycode (rust-windowing#229)

* feat: support accelerator key strings `,` `-` `.` `Space` `Tab` and `F13`-`F24` (rust-windowing#228)

* feat(macOS): support more accelerator key strings

* Move function keys together

* Add `,` `-` `.` `Space` `F20-F24` for Windows

* Remove support for accelerators not found in `winapi`

* Add `,` `-` `.` `Space` `F13-F24` for Linux

* Update .changes

* Add the rest for Windows

* Add the rest for Linux

* Add the rest on macOS

* Update accelerator-strings.md

* Fix git comments

Co-authored-by: Kasper <[email protected]>
Co-authored-by: Amr Bashir <[email protected]>

* Add redraw events on Linux (rust-windowing#245)

* Add redraw events on Linux

* Update doc of RequestRedraw Event

* Add change file

* Fix missing menu bar on borderless window (rust-windowing#247)

Credit goes to irh's work on winit commit f2de847

* refactor: improve `set_skip_taskbar` impl on Windows (rust-windowing#250)

* fix: emit errors on parsing an invalid accelerator for string, closes rust-windowing#135 (rust-windowing#252)

* chore: update comment

* fix(linux): fix focus events not firing properly (rust-windowing#253)

* fix(linux): fix focus events not firing properly

* add changelog

* chore: update focus events error message

* chore: fmt

* fix: revert windows-rs 0.28 version bump

* fix(linux): fix native menu items (rust-windowing#256)

* chore: remove examples commited by accident

* Update `ReceivedImeText` (rust-windowing#251)

* Allow receiving text without Ime on Windows

* Avoid panic todo

* Receive text without ime on mac

* Fix CursorMoved event on Linux

* Add ReceivedImeText on Linux

This only add Simple IME from GTK for now. We should add the actual IME
from system in the future.

* Fix redraw event that causes inifinite loop (rust-windowing#260)

* Fix redraw event that causes inifinite loop

* Refactor event loop

* Remove unused function

* Add doc comment on linux's run_return

* Ignore doc test on run_return

* Add non blocking iteration on Linux (rust-windowing#261)

* Docs: SystemTrayExtWindows::remove() is gone (rust-windowing#262)

Fix docs following rust-windowing#153

* Fix busy loop on Linux (rust-windowing#265)

* Update windows crate to 0.29.0 (rust-windowing#266)

* Update to windows 0.29.0

* Add change description

* Remove clippy check (rust-windowing#267)

* refactor(windows): align util function with win32 names

* chore: update PR template

* fix(linux): fire resized & moved events on min/maximize, closes rust-windowing#219 (rust-windowing#254)

* feat(linux): implement `raw_window_handle()` (rust-windowing#269)

* chore(deps): update to raw-window-handle 0.4

* add linux raw-window-handle support

* update macos/ios/android

* fix ios

* Fix core-video-sys dependency (rust-windowing#274)

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#275)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

* Fix some invalid msg_send! usage (rust-windowing#276)

* Revert "Fix some invalid msg_send! usage (rust-windowing#276)" (rust-windowing#277)

This reverts commit a3a2e0cfc49ddfa8cdf65cf9870fb8e3d45b4bc0.

* Revert "The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#275)" (rust-windowing#279)

This reverts commit 6f9c468f26ddb60e29be2139397bfaf3b30eab1e.

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#280)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

Co-authored-by: madsmtm <[email protected]>

* Fix some invalid msg_send! usage (rust-windowing#278)

Co-authored-by: madsmtm <[email protected]>

* Add exit code to ControlFlow::Exit (rust-windowing#281)

* Add exit code to ControlFlow::Exit

* Cargo fmt

* Add change files

Co-authored-by:  multisn8 <[email protected]>

* Add new_any_thread to Unix event loop (rust-windowing#282)

* Update windows crate to 0.30.0 (rust-windowing#283)

* Update windows crate to 0.30.0

* Simplify new-type usage

* Fix boxing in GWL_USERDATA

* Make sure everyone is using Get/SetWindowLongPtrW

* build the system_tray module when "ayatana" feature is enabled (rust-windowing#285)

Without those cfg feature checks, the "ayatana" feature does
actually not enable anything.

* Fix click events missing whe tray has menu (rust-windowing#291)

* Fix click events missing whe tray has menu

* Add change file

* Fix crash when tray has no menu (rust-windowing#294)

* chore: update pull request commit exmple

* fix(windows): send correct position for system tray events, closes rust-windowing#295 (rust-windowing#300)

* fix(windows): revert maximized state handling to winit impl, closes rust-windowing#193 (rust-windowing#299)

* fix(windows): revet maximized state handling to winit impl, closes rust-windowing#193

* add chanefile [skip ci]

* fix: `MenuItem::Quit` on Windows (rust-windowing#303)

* fix: `MenuItem::Close` on Windows

* use `PostQuitMessage` instead

Co-authored-by: amrbashir <[email protected]>

* feat: v1 audit by Radically Open Security (rust-windowing#304)

* Update to gtk 0.15 (rust-windowing#288)

* Update to gtk 0.15

* Fix picky none on set_geometry_hint

* Fix CursorMoved position

Co-authored-by: Amr Bashir <[email protected]>
Co-authored-by: Bill Avery <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Co-authored-by: Kasper <[email protected]>
Co-authored-by: amrbashir <[email protected]>
Co-authored-by: Jay Oster <[email protected]>
Co-authored-by: madsmtm <[email protected]>
Co-authored-by: multisn8 <[email protected]>
Co-authored-by: Aurélien Jacobs <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

MacOS can't wake up during resize, breaks timeout schemes