Implement more efficient run loop

[Jacalz]

Description:

This uses glfw.WaitEvents() which blocks until either an event occurs or when an empty event is posted. With the new threading model, this was a simple as just posting an empty event from animations when there are animations running. This mostly works well. System tray clicks don't appear until mouse is moved into the window and the same with changing settings within fyne_settings.

Fixes #2506

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[Jacalz]

@dweymouth Feel free to test this out and help out getting it to the finish line if you'd like. There are some problems remaining but it is working out quite well. With no animations on screen, we will wait for events to occur.

[Jacalz]

This might not even be relevant any more with threading changes? It seems to me like CPU usage is quite unchanged between the two, even for completely idle windows with no animations to run.

[dweymouth]

I quickly tried out something that was closer to the idea I was thinking of - removing the time.Ticker entirely that currently drives the event loop (the <-t.C case in the select became a default). With that, CPU does truly go down to 0 on idle, though it would still need some more work to handle the animation case then. My idea would be to switch to a loop being driven by the Ticker and pollEvents while there exist animations to run, and switching to waitEvents with no Ticker running for idle. Also, fyne.Do will need to post an empty event as well to wake up the main thread. (And during the transitional period, maybe canvas.Refresh as well in case people call it directly or indirectly (via Refresh on a widget) without using fyne.Do)

[dweymouth]

also, looking at this code again, I think I discovered an existing bug. Here: https://github.com/fyne-io/fyne/pull/5422/files#diff-c4f15824f3446f8e7e44764227aa45dd5f43c4981c462270d40dfaccef9b2198L142

The TickAnimations and drawSingleFrame calls should be outside the per-window loop, not within it, as far as I can tell.

[Jacalz]

Thanks for the notes and ideas. To be fair, fyne.Do already does post an empty event in this version (through runOnMain) but it did not seem to have much effect.

I might have a look at this in the coming days but I just want to say that you are absolutely welcome to commit to this branch of you want to collaborate :)

[Jacalz]

The TickAnimations and drawSingleFrame calls should be outside the per-window loop, not within it, as far as I can tell.

Cool. Feel free to open a PR if you'd like :)

[dweymouth]

Pushed updates to switch between sleeping (waitEvents) and polling at 60fps depending on whether there are animations to run. This brings CPU down to 0 and no idle wakeups showing, according to Mac activity monitor, when we have no animations to tick. I'm seeing the window fail to exit when closed for some reason in fyne_demo but I was also seeing that before my changes, so something that still needs to be addressed.

The logic of the loop.go updates was to move the select body out of the ticker and into its own "runSingleFrame" func. Also we are checking all the channels (done, funcs, settings) per frame now instead of just selecting one of them.

[Jacalz]

Well done. I will look through the code more but I can verify that CPU usage is at 0% with no animations here as well.

[andydotxyz]

Should we use this PR to resolve the "freezes on resize" issue as well - the solution may be the same code change.
It may already be fixed - I have not run this code yet.

[dweymouth]

Should we use this PR to resolve the "freezes on resize" issue as well - the solution may be the same code change.
It may already be fixed - I have not run this code yet.

This doesn't fix the "freezes on resize" issue - that seems to be caused by GLFW as both waitEvents and pollEvents block while the window is being resized. I'm actually not sure we can fix it in pure Go code (or if it's even fixable?).

[dweymouth]

I ran this again on M1 Mac and it is not freezing on window exiting. I think I had something wrong in my local state last time since I applied the patch manually to my branch, vs now I'm running it straight from cloning jacalz/fyne and it seems to work fine, no bugs that I can tell clicking around in fyne_demo. (Except it doesn't fix the aforementioned freeze-on-resize)

[andydotxyz]

Should we use this PR to resolve the "freezes on resize" issue as well - the solution may be the same code change.
It may already be fixed - I have not run this code yet.

This doesn't fix the "freezes on resize" issue - that seems to be caused by GLFW as both waitEvents and pollEvents block while the window is being resized. I'm actually not sure we can fix it in pure Go code (or if it's even fixable?).

There must be something we can do, as it was working before falling back to one thread ;)

[dweymouth]

This may be relevant (for the freeze-on-resize, which is orthogonal to this PR):

https://stackoverflow.com/questions/45880238/how-to-draw-while-resizing-glfw-window

[Jacalz]

Can we not post an empty event when doing a resize?

[dweymouth]

From the StackOverflow, it sounds like we may need to hook into the GLFW window_refresh_callback (and/or the framebuffer_size_callback) and maybe just post an empty event from within those callbacks, or maybe call our runSingleFrame directly from there. Anyhow, I think that should be a follow-up, not done in this PR, since this PR achieves the CPU improvement without changing the behavior at all of the draw-on-resize issue

[Jacalz]

Yeah, I agree

[Jacalz]

This PR is almost working perfectly on my end. Changing the theme from fyne_setting or the dark/light theme control on Gnome does not update until I move my mouse to the window when there are no animations running.

Also: Do note that I think animations for objects that are not visible are still running? I notice that if I go to the progressbar tab and then move away from there, there are still animations ticking in the backround making the window use the less efficient path.

[Jacalz]

Do you think it would be worth it to add some check if we are running in glfw.WaitEvents() mode and not post an empty event in that case? I assume we want to do as few CGo calls as possible but I'm not sure it is worth it?

[dweymouth]

Do you think it would be worth it to add some check if we are running in glfw.WaitEvents() mode and not post an empty event in that case? I assume we want to do as few CGo calls as possible but I'm not sure it is worth it?

Probably not, or at the least it would be premature optimization. It's the waitEvents case where we do have to post an event to wake up main, and that's the default mode. Only if there are actively running animations do we use pollEvents where we could theoretically avoid posting an empty event. (I suppose also we could avoid posting an empty event from fyne.Do if we know there are already queued funcs that haven't run yet, now that we run them all in one go, but that would require complex signaling and locking I think)

[dweymouth]

Also: Do note that I think animations for objects that are not visible are still running? I notice that if I go to the progressbar tab and then move away from there, there are still animations ticking in the background making the window use the less efficient path.

This has always been the case and I'm not sure what we can do about it -- we don't currently have a system for tracking when a widget first becomes invisible. Fyne_demo could use the OnChanged hook from the tabs to call .Stop() on the infinite progress bar maybe? Or we just don't care since it's just fyne_demo :)

[Jacalz]

Good point. I think we likely want to create a good example and hide it when changing tabs :)

[dweymouth]

Settings change working now for me. Had to add a new API to the settings (keeping it internal for now with anonymous interface casting) to invoke listeners via a func instead of a channel, since a func allows us to post empty event to wake up main. Attempting to post empty event directly from the settings didn't work because of timing with the postEmptyEvent

[dweymouth]

Oh I noticed tables don't scroll smoothly with the Mac trackpad. I think it sends more than 60 events per second perhaps and table is doing too much per scroll... may need to look into combining events now and rate limiting drawing somehow

[dweymouth]

Alternatively, we could switch back to 60fps polling when we receive an event, and go back to idle mode only when we receive no events within 1/60 sec. That way we should be invoking widget code similarly to before with max 60fps events (except maybe for that first one that wakes the loop out of idle mode)

[dweymouth]

Do you think it would be worth it to add some check if we are running in glfw.WaitEvents() mode and not post an empty event in that case? I assume we want to do as few CGo calls as possible but I'm not sure it is worth it?

Returning to this, I think it may be worth it if we move to a custom slice+lock-based func queue, to avoid posting the empty event if there are multiple funcs in the queue (since we know the driver will be awake processing them then). We cannot do it with a simple asleep atomic.Bool, since if the main thread were to set asleep.Store(true) and then call glfw.WaitEvents() there is still the unavoidable race that: 1) goroutine checks asleep and sees it's false, 2) main thread sets asleep = true, then calls glfw.WaitEvents to go to sleep`, 3) goroutine adds func to the queue and doesn't post empty event since it thinks main is awake.

But I believe it could be done with the custom func queue, because pulling the head from the queue, as well as adding a func to the queue and posting an event if needed, can each be made atomic operations

[dweymouth]

@Jacalz if you have some time could you rebase this to the latest develop? I want to see how much Andy's scroller PR may have smoothed things up with resizing the Table/Tree in fyne_demo