GNOME Bugzilla – Bug 753891
animation: Use Tweener to animate instead of a timeout
Last modified: 2016-01-19 08:49:27 UTC
aruiz pointed out on IRC that gdm's gnome-shell was constantly burning a bit of CPU. As far as I can tell (using gdb) this is was the only timeout causing mainloop wakeups. In particular it was the js/gdm/authPrompt.js instance because this._spinner.stop() is itself called from a Tweener onComplete() handler which isn't reached before the VT switch to the user session and thus never happens (actually it does if you VT switch back to gdm).
Created attachment 309755 [details] [review] animation: Use Tweener to animate instead of a timeout This way, if the clutter master clock is frozen, perhaps because we're VT switched away, we won't needlessly run the animation.
Review of attachment 309755 [details] [review]: So i don't think the below is going to work generally. Two possible ideas: 1) use imports.misc.loginManager to detect vt switch and stop the spinner 2) add a signal to clutter master clock to emit when it's freezing and stop the spinner then ::: js/ui/animation.js @@ +45,3 @@ + _tween: function() { + this._lastFrameTime = GLib.get_monotonic_time() / 1000.0; + Tweener.addTween(this.actor, this is going to break spinners in a VM (or remote connection) where animations are disabled.
or maybe 2) could be generalized to complete all pending tweens before freezing the clock ?
(In reply to Ray Strode [halfline] from comment #2) > ::: js/ui/animation.js > @@ +45,3 @@ > + _tween: function() { > + this._lastFrameTime = GLib.get_monotonic_time() / 1000.0; > + Tweener.addTween(this.actor, > > this is going to break spinners in a VM (or remote connection) where > animations are disabled. And this matches what GTK+ does as well. We want the "disable animations" button to disable animations.
so there's no frontend api to ClutterMasterClock, so the signal can actually just go in mutter right before we pause it, here: static void• session_pause (void)• {• clutter_evdev_release_devices ();• clutter_egl_freeze_master_clock ();• }•
no, spinners shouldn't get disabled when animations are disabled. it's a bug that that happens in gtk.
Are you sure about that? I thought it was intentional that we disabled animations when the "disable animations" button was pressed. Anyway, there is a front-end API to ClutterMasterClock -- it's ClutterTimeline. You can use that to tick independently.
Yea, I really don't think we should be disabling "functional" animations. Only big animations that cause a lot of screen flicker and are primarily provided for transition aesthetics. You can drop the latter without taking functionality away. ClutterTimeline isn't going to help us here, unless we adopt it more completely (see the code in comment 5, it's using master clock backend api).
to give a concrete example of what i mean...right now the timed login animation uses tweener, so if you turn on timed login in a vm, it logs you in immediately. That animation shouldn't get disabled.
we could also be really heavy handed and make session_pause() block until sd_login_monitor says the session is active again.
Created attachment 309764 [details] [review] native: block main loop if on another VT If we're not visible, there's little reason to dispatch events. We try to avoid drawing by freezing the master clock, but it's easy for a stray g_timeout_add to keep running anyway. This commit makes us block the main loop wholesale, which is closer to how we behave on X11 (which blocks the main loop waiting on the glx lock).
so attachment 309764 [details] [review] comes with a few caveats: 1) I haven't tested it (yet) 2) the code could be cleaned up more (we're still freezing the master clock, when we probably don't need to anymore, we're monitoring for vt switches in two different ways, etc) 3) i'm not sure if doing this heavy handed of an approach is a good idea or not. i'm mainly posting it for feedback, at this point.
Review of attachment 309764 [details] [review]: ::: src/backends/native/meta-launcher.c @@ +150,3 @@ + } + + is_active = ret; this needs a if (is_active) break;
Review of attachment 309764 [details] [review]: This is absolutely broken. We can't do this.
> This is absolutely broken. We can't do this. so you think it's a bad idea, and I'm on the fence, but it's not absolutely broken. it's pretty equivalent to how X11 behaves, which was status quo for a very long time.
Wayland clients might crash if the compositor is blocked and doesn't respond in time. Xwayland crashing would take the session down. We used to do something similar (run a blocking nested main loop when switched away) until being VT switched away for a minute meant Xwayland wouldn't get a response, crash, and then mutter responds by taking down its session as well.
(In reply to Jasper St. Pierre from comment #16) > We used to do something similar (run a blocking nested main loop when > switched away) until being VT switched away for a minute meant Xwayland > wouldn't get a response, crash, and then mutter responds by taking down its > session as well. did Xwayland get fixed to not crash?
Nope, but any client can crash when the client buffer is full. We need to make sure to always be parsing client messages, even when VT switched away.
okay you've convinced me, unless we blocked all the clients too, blocking just the compositor isn't a good idea. let's just abandon that idea.
so maybe we're going about this the wrong way... instead of stopping the spinner in response to the vt change, why not stop the spinner before telling gdm to change vts? Solving the more general problems related to the master clock are fine, but it's a bigger task and I guess it isn't really necessary for solving this bug.
so I found one problem in the code that I think fixing, might help with this problem. Unfortunately, when I try to reproduce the problem I can't... my login screen gnome-shell is stuck at 0% cpu. if i strace it, it wakes up once every several seconds, not several times every second. hmmm will post the patch anyway, but we probably shouldn't commit it until we can get a better handle on the situation.
Created attachment 309821 [details] [review] authPrompt: stop spinner after its hidden The code previously tried to stop spinner after it was hidden, but due to an incorrect check was only stoppig it after it was shown. Also, it was only stopping after hiding due to an animation, and failing to stop it in the non-animated case. This left the spinner hidden and running while VT switched away from the login screen, only stopping when the auth prompt was reset when switching back.
(no idea if attachment 309821 [details] [review] is sufficient to fix the problem)
Review of attachment 309821 [details] [review]: This makes sense in itself but doesn't fix the original issue of the spinner not stopping after a successful log in. Adding a this.stopSpinning() call to _onVerificationComplete() seems to fix it and seems like the right thing to do since all other userVerifier signal handlers except this one end up calling reset() or clear() which does stopSpinning()
Do we have a committable fix here ?
yup, i'll just push the two changes talked about here (well s/stopSpinning/setDefaultActorInButtonWell(null)/)
Created attachment 310378 [details] [review] authPrompt: hide/stop spinner after verfiication completes When the user successfully types their password, we should hide the spinner from the button well right away, so it doesn't consume resources until reset (which may happen significantly later if the user is vt switched away)
Attachment 309821 [details] pushed as 030a22d - authPrompt: stop spinner after its hidden Attachment 310378 [details] pushed as f2d4aa0 - authPrompt: hide/stop spinner after verfiication completes
Comment on attachment 309755 [details] [review] animation: Use Tweener to animate instead of a timeout This isn't needed
I think we're done here.
Hello, this may be causing bug 754814 which somehow pauses logging in until the user presses Escape key on his/her keyboard.
Hi. I think this isn't really fixed. I have to ctrl+alt+f1 then ctrl+alt+f2 back to my session to stop the gdm version of gnome-shell from constantly eating 20% CPU. This is very rough on my core2duo 2.9GHz processor with nvidia GPU. I am working around this by using autologin. It would be however nice if I didn't have to.
Probably because of bug 754814 comment 66 so we're not getting verification-complete signal anymore at login time so attachment 310378 [details] [review] isn't effective.
I need to see if i can reproduce grawity's crasher and then add the gdm patch back.
(In reply to Ray Strode [halfline] from comment #34) > I need to see if i can reproduce grawity's crasher and then add the gdm > patch back. I just reapplied the patch to 3.18.2 and restarted gdm. No crashes on unlock with normal logon. I tried both unlocking from ctrl+alt+f1 (greeter session) and ctrl+alt+f2 (my session). Then re-enabled auto-login and still unlocking works so this covers all cases. But I am using Nvidia. Maybe it now crashes on non-nvidia for grawity haha.
And more importantly, it got rid of the constant 20% CPU on gdm's version of gnome-shell.