[Bug 59921] Use g_timeout_add_seconds to reduce wakeups

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=59921

--- Comment #2 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Comment on attachment 73715
  --> https://bugs.freedesktop.org/attachment.cgi?id=73715
Use g_timeout_add_seconds wherever possible

Review of attachment 73715:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=59921&attachment=73715)
-----------------------------------------------------------------

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.