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

Revert using a separate draw thread #4656

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Feb 18, 2024

Progresses #4654

Checklist:

  • Tests included. <- currently manual testing to check all platforms
    • macOS arm
    • macOS Intel
    • Windows
    • Linux X11
    • Linux Wayland
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Any breaking changes have a deprecation path or have been discussed.

Notice that on macOS the UI will stop drawing if we resize. This is the reason draw thread was added and we will need to find another solution...

@andydotxyz andydotxyz mentioned this pull request Feb 18, 2024
16 tasks
@coveralls
Copy link

coveralls commented Feb 20, 2024

Coverage Status

coverage: 59.949% (-0.03%) from 59.974%
when pulling c890d2e on andydotxyz:fix/mergethreads
into a1d708d on fyne-io:develop.

}
}
r.animationMutex.Lock()
r.animations = append(newList, r.pendingAnimations...)
Copy link
Contributor

Choose a reason for hiding this comment

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

The runner can get a whole lot cleaner once we assume the new threading model. While Start and Stop can still be called when invoking user code within a Tick, we will know they won't be called elsewhere, so we can do some simplification and cleanup around the synchronization between Start, Stop and Tick. That can be a follow-up PR though.

@andydotxyz
Copy link
Member Author

Putting this on hold until we begin work on 2.6

@andydotxyz andydotxyz marked this pull request as draft April 23, 2024 14:56
@andydotxyz andydotxyz added this to the "F" release, Late 2024 milestone May 23, 2024
@andydotxyz andydotxyz marked this pull request as ready for review December 9, 2024 21:40
@andydotxyz
Copy link
Member Author

Reworked on top of develop.

This seems to work smoothly in limited testing but we need to get it checked on all OS.
Lots more work is required to simplify but I didn't want to go too deep yet.

@andydotxyz
Copy link
Member Author

P.S. in a follow up PR where I merge the event thread in as well I have hello and fyne_demo completely without race errors!

Keeping them separate as this PR works smoothly but the event management change has significant performance impact!

@Jacalz Jacalz self-requested a review December 9, 2024 22:27
@andydotxyz
Copy link
Member Author

All Mac tests passing. There is a performance impact but it is actually clear where some latency is coming from - hovering events are triggering multiple event updates etc. This change may help with hunting down other performance bugs!

@Jacalz
Copy link
Member

Jacalz commented Dec 10, 2024

Nice. That should help us decrease power consumption and unnecessary cpu usage :)

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Well done. Performance seems okay and everything appears to be working here on Linux. Just left one minor comment line but I'll approve before hand :)

drawFuncQueue <- drawData{f: f, win: w, done: done}
<-done
func runOnMainWithContext(w *window, f func()) {
runOnMain(func() { w.RunWithContext(f) }) // TODO remove this completely
Copy link
Member

Choose a reason for hiding this comment

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

Is this a TODO for an upcoming PR or should it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is part of the future refactoring. Lots more to be done as mentioned above and in the other tickets.

There are 3 different ways to queue onto what is essentially the same thread but I didn't want to rip out too much code until the direction is established :)

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Just wanted to check. Still approved from my end :)

@andydotxyz andydotxyz merged commit 5ee9979 into fyne-io:develop Dec 12, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants