After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 399012 - Rhythmbox wakes up too much
Rhythmbox wakes up too much
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: general
HEAD
Other Linux
: Normal normal
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
: 355491 437180 441385 512412 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2007-01-21 13:11 UTC by James "Doc" Livingston
Modified: 2013-01-11 09:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix clipboard and not-playing tick wakeups (5.92 KB, patch)
2007-02-14 08:32 UTC, James "Doc" Livingston
committed Details | Review
strace output (58.65 KB, text/plain)
2007-04-07 21:59 UTC, Benoît Dejean
  Details
kill rhythmdb polling (16.11 KB, patch)
2007-05-14 12:25 UTC, Jonathan Matthew
none Details | Review
fix a few more things (20.95 KB, patch)
2007-05-15 13:18 UTC, James "Doc" Livingston
committed Details | Review
use g_timeout_add_seconds for some things (6.71 KB, patch)
2007-05-20 23:24 UTC, Jonathan Matthew
committed Details | Review
Update patch to current svn (6.21 KB, patch)
2008-08-09 11:54 UTC, Christophe Fergeau
none Details | Review
Change more g_timeout_add to g_timeout_add_seconds (4.52 KB, patch)
2008-08-09 11:55 UTC, Christophe Fergeau
none Details | Review
Update patch #116223 to current svn (4.12 KB, patch)
2008-08-10 10:17 UTC, Christophe Fergeau
committed Details | Review

Description James "Doc" Livingston 2007-01-21 13:11:24 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.
Comment 1 Jonathan Matthew 2007-02-07 11:44:28 UTC
*** Bug 355491 has been marked as a duplicate of this bug. ***
Comment 2 Benoît Dejean 2007-02-07 14:14:29 UTC
I was also reporting about activity while playing.
Comment 3 James "Doc" Livingston 2007-02-14 08:32:26 UTC
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.
Comment 4 James "Doc" Livingston 2007-02-26 08:16:48 UTC
Patch has been committed to trunk.
Comment 5 Benoît Dejean 2007-04-07 21:58:47 UTC
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.
Comment 6 Benoît Dejean 2007-04-07 21:59:36 UTC
Created attachment 85968 [details]
strace output
Comment 7 Jonathan Matthew 2007-05-14 12:25:07 UTC
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.
Comment 8 James "Doc" Livingston 2007-05-14 13:16:09 UTC
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).
Comment 9 Jonathan Matthew 2007-05-14 23:37:20 UTC
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.
Comment 10 James "Doc" Livingston 2007-05-15 13:18:26 UTC
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.
Comment 11 James "Doc" Livingston 2007-05-20 08:37:09 UTC
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.
Comment 12 Jonathan Matthew 2007-05-20 23:24:12 UTC
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.
Comment 13 Peter 2007-05-21 08:32:40 UTC
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/
Comment 14 Peter Robinson 2007-05-21 08:34:40 UTC
(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
Comment 15 Jonathan Matthew 2007-05-26 10:15:50 UTC
*** Bug 441385 has been marked as a duplicate of this bug. ***
Comment 16 James "Doc" Livingston 2007-06-02 03:58:02 UTC
The patch to use g_timeout_add_seconds looks good to me.
Comment 17 Jonathan Matthew 2007-06-15 10:54:11 UTC
*** Bug 437180 has been marked as a duplicate of this bug. ***
Comment 18 Bill Nottingham 2007-08-09 16:24:59 UTC
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
Comment 19 James "Doc" Livingston 2007-08-12 13:32:59 UTC
Does powertop indicate what (if it's not Rhythmbox itself) is waking up 50 times a second?
Comment 20 Bastien Nocera 2007-08-13 11:16:22 UTC
(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.
Comment 21 Jonathan Matthew 2008-01-27 22:10:25 UTC
*** Bug 512412 has been marked as a duplicate of this bug. ***
Comment 22 Jonathan Matthew 2008-01-28 06:52:29 UTC
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..
Comment 23 Jonathan Matthew 2008-01-28 07:41:58 UTC
After rebuilding gst-python without the timer, an idle rhythmbox is back down to 0.3 wakeups/sec.
Comment 24 Christian Kirbach 2008-01-29 22:15:44 UTC
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.
Comment 25 Jonathan Matthew 2008-01-30 12:50:58 UTC
I opened bug 512916 (already fixed) about the timer in gst-python, in case anyone's playing along at home.
Comment 26 Bastien Nocera 2008-08-07 21:20:16 UTC
Patch needs updating, as we now rely on a new enough glib to avoid the ifdefs.
Comment 27 Jonathan Matthew 2008-08-09 11:41:27 UTC
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.
Comment 28 Christophe Fergeau 2008-08-09 11:54:00 UTC
Created attachment 116222 [details] [review]
Update patch to current svn
Comment 29 Christophe Fergeau 2008-08-09 11:55:44 UTC
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
Comment 30 Christophe Fergeau 2008-08-09 13:55:55 UTC
Ooops, didn't see Jonathan's comment before adding those patches :-/
Comment 31 Christophe Fergeau 2008-08-10 10:17:14 UTC
Created attachment 116281 [details] [review]
Update patch #116223 to current svn
Comment 32 Jonathan Matthew 2008-08-12 13:31:47 UTC
Looks OK to me.
Comment 33 Christophe Fergeau 2008-11-13 12:56:11 UTC
I totally had forgotten this patch, it's in now.
Comment 34 Christophe Fergeau 2013-01-11 09:03:01 UTC
Closing as per last comment