Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

add option for "simple" main loop iteration #628

Closed
wants to merge 10 commits into from
Closed

add option for "simple" main loop iteration #628

wants to merge 10 commits into from

Conversation

jwahlstrand
Copy link
Contributor

I hesitated to post this because it seems too good to be true. This PR creates a "simple" main loop option (disabled by default) that replaces the call to gtk_main() with a Timer that iterates the GLib main loop every 5 ms in a non-blocking way. It also skips the call to __init__gmainloop__(), completely omitting the creation of a GSource for libuv. This is enabled using GTK_SIMPLE_LOOP = true.

When I run Julia 1.7 with GTK_SIMPLE_LOOP=true GTK_AUTO_IDLE=false JULIA_NUM_THREADS=4, I don't see a slowdown in the multithread tests posted at the beginning of #503. All Gtk.jl tests pass for me locally on Linux and Windows. ProfileView, ImageView and Winston seem to work just fine.

Am I missing something? It's very possible I just haven't managed to create the situations that led to the tight integration of the libuv loop with the GLib loop. On the other hand, that code is ~8 years old and maybe isn't needed in recent versions of Julia?

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #628 (1bbb88c) into master (d76faaa) will decrease coverage by 3.12%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
- Coverage   65.69%   62.57%   -3.13%     
==========================================
  Files          32       32              
  Lines        2720     2739      +19     
==========================================
- Hits         1787     1714      -73     
- Misses        933     1025      +92     
Impacted Files Coverage Δ
src/GLib/signals.jl 47.56% <66.66%> (-28.26%) ⬇️
src/Gtk.jl 74.39% <75.86%> (-17.68%) ⬇️
src/base.jl 89.55% <100.00%> (-2.12%) ⬇️
src/cairo.jl 79.45% <100.00%> (+0.88%) ⬆️
src/events.jl 42.85% <0.00%> (-4.09%) ⬇️
src/GLib/gtype.jl 75.20% <0.00%> (-0.83%) ⬇️
src/GLib/gvalues.jl 88.59% <0.00%> (+0.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d76faaa...1bbb88c. Read the comment docs.

@tknopp
Copy link
Collaborator

tknopp commented Feb 23, 2022

So instead of running the gtk main loop on its own long-running task this just runs one iteration of the main loop in a dedicated task. I did not know that GLib offers that functionality and it seems to make more sense for Gtk.jl since it offers more fine-grained control.

@jwahlstrand
Copy link
Contributor Author

Yes, this would also let us suspend the loop momentarily without quitting it. But critically this doesn't seem to disturb multithreading at all. My concern is that this skips >100 lines of code in signals.jl. Is that really not needed? It would be good for others to test this.

@jwahlstrand
Copy link
Contributor Author

Two of the three failures in the previous CI run were caused by a canvas widget not being realized in time on macos for a call to getgc(). Modiying a test to force an iteration of the loop solves that. The third was a segfault and that makes me nervous... I will continue to putter around with this idea.

@tknopp
Copy link
Collaborator

tknopp commented Feb 24, 2022

I am not 100% sure but probably the while loop should move into getgc?

@jwahlstrand
Copy link
Contributor Author

Yeah, maybe one could iterate the loop if the widget isn't realized in getgc.

Skipping the call to __init__gmainloop__ disables all the g_yield machinery, which is used by @sigatom. That seems important. However, in the docs at the bottom of https://juliagraphics.github.io/Gtk.jl/latest/manual/signals/, there's a discussion of why and when you need @sigatom, followed by, "These restrictions should be fixed once JuliaLang/julia#13099 is merged." That happened a long time ago, which fits the hypothesis that it's no longer necessary, but I'm definitely in over my head here.

When I have time I'll refine this PR. We'll see if that random segfault reappears.

@tknopp
Copy link
Collaborator

tknopp commented Feb 25, 2022

Did you still see segfaults?

@tknopp
Copy link
Collaborator

tknopp commented Feb 25, 2022

And yes after JuliaLang/julia#13099 was merged I was able to remove all the @sigatoms from my (larger) Gtk application. IMHO all this can be removed.

@jwahlstrand
Copy link
Contributor Author

No, that's 4 or 5 in a row with no segfaults. But I just noticed that I broke autoidle. Once that is fixed I think this is ready to go -- I don't have time to work on it now. Do you think this should be an option for now or the new default main loop behavior?

@tknopp
Copy link
Collaborator

tknopp commented Feb 25, 2022

If it works better, I would make it the default.

@tknopp
Copy link
Collaborator

tknopp commented Feb 25, 2022

Whether to remove the old code, I am not entire sure. Probably it would be better to make the cleanup in the Gtk4.jl project?

@jwahlstrand
Copy link
Contributor Author

I tried this out on Windows this evening and, despite all tests passing, it's a disaster. Resizing windows and interacting with the GUI is very sluggish. It works fine on Linux...

@tknopp
Copy link
Collaborator

tknopp commented Mar 1, 2022

The windows issue is of course very unfortunate. Is this completely solved by disabling the client side decorations?

Probably the best would be to keep both possibilities (as you do now) and still cleanup the old path (@sigatom...).

@jwahlstrand
Copy link
Contributor Author

Disabling client side decorations on Windows largely solves the responsivity issues. The only visible effect I've noticed is windows look more like Windows windows than GTK windows. From what I've read, there might be problems if someone tries to use GtkHeaderBar, but that widget isn't supported currently anyway. Redrawing in response to GUI adjustments is still a little slower than before, which might be related to this: https://gitlab.gnome.org/GNOME/gtk/-/issues/801 . I think it's acceptable.

I did some basic "human testing" on a Mac just now and it seems to work well. I'd say redraw performance is a bit worse than before, so there might be a general performance hit from iterating the main loop in a non-blocking way. But then on Linux I don't notice any difference.

OK, do you think we should clean up the @sigatom stuff here? I could at least remove the mention in the docs. I need to add some words to the docs about how to switch between this and auto idle, but after that I think it's ready to go.

@tknopp
Copy link
Collaborator

tknopp commented Mar 2, 2022

Any performance degradation worries me. The experience on OSX is not so great. But as long as we have the switch, its fine I think.

IMHO we should look forward to the upcoming Gtk4 experience and put most energy there.

OK, do you think we should clean up the @sigatom stuff here? I could at least remove the mention in the docs. I need to add some words to the docs about how to switch between this and auto idle, but after that I think it's ready to go.

I don't care if you still remove it here or at the Gtk4 repository. My comment was more targeting the later: I.e. that the Gtk4 code base can clean this up. So probably the best is to leave it here as is, so that we can merge the PR and then you can carry over a cleaned up version to Gtk4.

@jwahlstrand jwahlstrand marked this pull request as ready for review March 3, 2022 01:53
@jwahlstrand
Copy link
Contributor Author

With this last commit, the performance issues on Mac and Windows are fixed. Disabling client side decorations was still necessary on Windows because without that there is a constant spew of Gdk-WARNING **: gdk-frame-clock: layout continuously requested, giving up after 4 tries. I think that we should go with #630, however, because it's a more elegant solution.

@tknopp
Copy link
Collaborator

tknopp commented Mar 3, 2022

what should we do with this? Keep the infrastructure for the future to remain flexible?

@jwahlstrand jwahlstrand closed this Mar 3, 2022
@jwahlstrand
Copy link
Contributor Author

Nah, this is sort of a mess.

@jwahlstrand jwahlstrand deleted the loop branch March 3, 2022 12:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants