GNOME Bugzilla – Bug 399012
Rhythmbox wakes up too much
Last modified: 2013-01-11 09:03:01 UTC
Rhythmbox wakes up a lot even when not doing anything. Ideally it would not wake up at all when the user hasn't done anything and it's not playing. When in a paused state, the following things wake it up, when combined means we're waking up about 10 times a second (which is BAD for laptops on battery). tick_timeout (200ms), rhythmdb_idle_poll_events (300ms), rb_audioscrobbler_timeout_cb (15s), idle_save_playlist_manager (10s), rb_shell_clipboard_idle_poll_deletions (300ms), purge_useless_threads (30s). For rhythmdb_idle_poll_events(), we should only add the timeout when pushing an event on the queue, and stop it running when there are none left. A bit complicated due to the threading. I'm not sure why we have rb_shell_clipboard_idle_poll_deletions(), rather than just removing the entries in the entry-delete callback. Perhaps we could replace the tick timeout with a GstClock thing. Or just disable it when paused.
*** Bug 355491 has been marked as a duplicate of this bug. ***
I was also reporting about activity while playing.
Created attachment 82520 [details] [review] fix clipboard and not-playing tick wakeups This patch removed the clipboard deletion one, and also removes the rb-player-gst tick callback while we're not playing and paused. > I was also reporting about activity while playing. This is probably less of a concern than waking up while doing nothing, since 1) we'll be waking up a reasonable amount for playback related things, and 2) most of the non-playback wakeups are the ones reported above that are done when not playing.
Patch has been committed to trunk.
I am using RB 0.9.6 and playing a MP3 file takes 28% of CPU (G4 1GHz). Even when RB is minimized to tray, notification off. I am attaching an partial strace output. It shows many poll on the same fds with micro timeouts, even 0, always < 100. Only 1 poll out of 147 succeeds.
Created attachment 85968 [details] strace output
Created attachment 88153 [details] [review] kill rhythmdb polling This adds a GSource that dispatches events from an async queue. This means we need to call g_main_context_wakeup any time we push something onto the event queue.
Looks good to me and seems to work fine. However I then notice wakeups from the action thread. read_queue() [rhythmdb.c] wakes up every now because of the timeout on popping from the action queue, to check for cancellation. Instead of doing that, we should probably use an untimed pop and when we cancel push a dummy event onto the action queue to force it to return. I've also just noticed that when the cancel flag is true, we leak the thing we just popped off the stack (but given that we're about to quit, it probably doesn't make that much difference).
We should also look at using g_timeout_add_seconds where possible for the longer timeouts (glib 2.14+) and perhaps changing some of the periods so they group better.
Created attachment 88203 [details] [review] fix a few more things This patch adds what I mentioned above to the previous rhythmdb polling patch, and also stops rhythmdb_process_changed_files getting called every 2 seconds when there are no waiting files.
I've committed the patch to svn, and now RB now wakes up every few seconds for me when not playing. Making the playlist manager not save when playlists haven't changed shouldn't be too hard.
Created attachment 88516 [details] [review] use g_timeout_add_seconds for some things I haven't actually tested this with a glib that has g_timeout_add_seconds, but it should help a little. It won't make any difference at all with an older glib.
Slightly off topic, but running RB with PowerTop might be interesting - I'm sure these recent code changes will have improved the situation: http://www.linuxpowertop.org/
(In reply to comment #13) > Slightly off topic, but running RB with PowerTop might be interesting - I'm > sure these recent code changes will have improved the situation: > > http://www.linuxpowertop.org/ See mailing list thread here http://mail.gnome.org/archives/rhythmbox-devel/2007-May/msg00020.html
*** Bug 441385 has been marked as a duplicate of this bug. ***
The patch to use g_timeout_add_seconds looks good to me.
*** Bug 437180 has been marked as a duplicate of this bug. ***
Related to this, I've noticed with powertop that when rhythmbox is pasued, the sound card causes about 50 interrupts a second, even though rhythmbox itself does not appear to be waking up. As soon as playback is *stopped* (instead of paused), the interrupts cease. rhythmbox-0.11.1-1.fc8 gstreamer-0.10.14-2.fc8
Does powertop indicate what (if it's not Rhythmbox itself) is waking up 50 times a second?
(In reply to comment #19) > Does powertop indicate what (if it's not Rhythmbox itself) is waking up 50 > times a second? It's Rhythmbox keeping the sound device opened when paused, and because the sound system sucks a bit, the hardware keeps generating interrupts even though it's not being used. Lennart tells me this will be fixed by using PulseAudio in the future. See also bug 465095.
*** Bug 512412 has been marked as a duplicate of this bug. ***
Bug 512412 reports rhythmbox waking up 20 times/sec while idle. I'm only seeing 10 times/sec here, due to a timer added by gst-python, apparently to handle ctrl-c keypresses. Seems a bit unnecessary in our case..
After rebuilding gst-python without the timer, an idle rhythmbox is back down to 0.3 wakeups/sec.
ok with latest svn I also see about 0.5 wakeups/sec when pausing play which is fine with me ;) well done for taking it down to this nice level.
I opened bug 512916 (already fixed) about the timer in gst-python, in case anyone's playing along at home.
Patch needs updating, as we now rely on a new enough glib to avoid the ifdefs.
I've removed the ifdefs and a few of the one-shot timers and committed this. I'm not sure how much more there is to do on this.
Created attachment 116222 [details] [review] Update patch to current svn
Created attachment 116223 [details] [review] Change more g_timeout_add to g_timeout_add_seconds This patch applies on top of the previous one and changes some more g_timeout_add uses to g_timeout_add_seconds. I haven't included it in the previous patch to make it easier to see which places were changed compared to the patch provided by Jonathan
Ooops, didn't see Jonathan's comment before adding those patches :-/
Created attachment 116281 [details] [review] Update patch #116223 to current svn
Looks OK to me.
I totally had forgotten this patch, it's in now.
Closing as per last comment