Bug 59921 - Use g_timeout_add_seconds to reduce wakeups
Summary: Use g_timeout_add_seconds to reduce wakeups
Status: NEW
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
Whiteboard: review-
Keywords: patch
Depends on:
Reported: 2013-01-27 07:18 UTC by B.Prathibha
Modified: 2013-01-28 14:29 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

Use g_timeout_add_seconds wherever possible (4.07 KB, patch)
2013-01-27 07:22 UTC, B.Prathibha
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 B.Prathibha 2013-01-27 07:22:39 UTC
Created attachment 73715 [details] [review]
Use g_timeout_add_seconds wherever possible
Comment 2 Simon McVittie 2013-01-28 12:17:06 UTC
Comment on attachment 73715 [details] [review]
Use g_timeout_add_seconds wherever possible

Review of attachment 73715 [details] [review]:

Thanks for looking into this, but please don't blindly replace g_timeout_add (n * 1000, ...) with g_timeout_add_seconds (n, ...). Not every call to g_timeout_add causes extra wakeups, and not every call to g_timeout_add can safely be replaced by g_timeout_add_seconds.

::: examples/client/extended-client.c
@@ +307,4 @@
>    g_signal_connect (cm, "got-info",
>        G_CALLBACK (connection_manager_got_info), NULL);
> +  timer = g_timeout_add_seconds (5, time_out, NULL);

This is only example code anyway, but yes add_seconds is better here, and it's somewhat important to be "correct" in an example.

::: telepathy-glib/base-media-call-content.c
@@ +73,4 @@
>  #include "telepathy-glib/util.h"
>  #include "telepathy-glib/util-internal.h"
> +#define DTMF_PAUSE_MS (3)

MS in the name denotes milliseconds, to avoid confusion between the various common time measurements (s/ms/µs/ns): if this change was acceptable, then the constant's name should change to DTMF_PAUSE_S or DTMF_PAUSE_SEC.

However, I don't think this change is really suitable anyway:

@@ +1123,4 @@
>                      self->priv->current_dtmf_state);
>                  break;
>                case DTMF_CHAR_CLASS_PAUSE:
> +                self->priv->tones_pause_timeout_id = g_timeout_add_seconds (

DTMF (aka "touch-tone" and various other names) is a way to communicate machine-readable information to, for instance, switchboards ("to speak to the Sales department, press 1").

If a switchboard/dial-string contains "p", which means "pause for 3 seconds", then it should pause for 3 seconds, not for 2.25 or 3.9 (both of which are entirely possible in GLib's current implementation of g_timeout_set_expiration).

This code only executes during VoIP calls, so we can't really reduce wakeups here: we'll be waking up frequently anyway, to process new audio data. So I think g_timeout_add is really the correct thing here.

::: telepathy-glib/run.c
@@ +101,4 @@
>      }
>  }
> +#define DIE_TIME 5

I'd prefer a rename to DIE_TIME_SEC, while you're changing the affected lines anyway.

The changes in this file make sense, although they basically won't save any wakeups: this timeout happens once during startup, and once every time Gabble finds that it has no active IM connections. That's something like 2 wakeups per IM session - pretty insignificant.

::: tests/dbus/account-manager.c
@@ +121,4 @@
>      guint timeout)
>  {
>    script_append_action (test, quit_action, NULL);
> +  test->timeout_id = g_timeout_add_seconds (timeout, test_timed_out, test);

These changes are in a regression test which is constantly active, so you won't avoid any wakeups. If the timeout actually goes off, the test fails.

If you want to change this file at all, please change it by removing this timeout completely, and changing script_start_with_deadline (Test *, guint) to just script_start (Test *).

The file has a global 10-second timeout, which is enough to stop it getting stuck on failure.

bug/show.html.tmpl processed on Oct 22, 2016 at 16:03:45.
(provided by the Example extension).